Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

writeFloat* and writeDouble* do no longer throw range error when out of bounds #5587

Closed
ghost opened this issue Mar 7, 2016 · 8 comments
Closed
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@ghost
Copy link

ghost commented Mar 7, 2016

I'l let the code speak for itself.

"use strict";

let buffer;

buffer = new Buffer(8);
buffer.fill(1);
console.log(buffer.writeFloatLE(0, 4), buffer, buffer.length);
// Node.js v4.3.1: "8 <Buffer 01 01 01 01 00 00 00 00> 8"
// Node.js v5.7.1: "8 <Buffer 01 01 01 01 00 00 00 00> 8"

buffer = new Buffer(8);
buffer.fill(1);
try {
    console.log(buffer.writeFloatLE(0, 5), buffer, buffer.length);
    // Node.js v5.7.1: "9 <Buffer 01 01 01 01 01 00 00 00> 8"
} catch (error) {
    console.log(error);
    // Node.js v4.3.1: "[RangeError: index out of range]"
}

buffer = new Buffer(16);
buffer.fill(1);
console.log(buffer.writeDoubleLE(0, 8), buffer, buffer.length);
// Node.js v4.3.1: "16 <Buffer 01 01 01 01 01 01 01 01 00 00 00 00 00 00 00 00> 16"
// Node.js v5.7.1: "16 <Buffer 01 01 01 01 01 01 01 01 00 00 00 00 00 00 00 00> 16"

buffer = new Buffer(16);
buffer.fill(1);
try {
    console.log(buffer.writeDoubleLE(0, 9), buffer, buffer.length);
    // Node.js v5.7.1: "17 <Buffer 01 01 01 01 01 01 01 01 01 00 00 00 00 00 00 00> 16"
} catch (error) {
    console.log(error);
    // Node.js v4.3.1: "[RangeError: index out of range]"
}

System 1

  • Version: v4.3.1
  • Platform: Windows Server 2012 R2 64-bit
  • Subsystem: Buffer

System 2

  • Version: v5.7.1
  • Platform: Windows 7 Pro SP1 64-bit
  • Subsystem: Buffer
@silverwind silverwind added the buffer Issues and PRs related to the buffer subsystem. label Mar 7, 2016
@princejwesley
Copy link
Contributor

this commit introduces silent drop behaviour.

@ghost
Copy link
Author

ghost commented Mar 7, 2016

I do not pass noAssert.

According to the docs of writeDouble*:

...Set noAssert to true to skip validation of value and offset. This means that value may be too large for the specific function and offset may be beyond the end of the Buffer leading to the values being silently dropped. This should not be used unless you are certain of correctness....

After reading this, i expect it to not silently drop because i did not pass noAssert so it defaults to false.

@Fishrock123
Copy link
Contributor

cc @trevnorris

@princejwesley
Copy link
Contributor

@Ravoltz Setting noAssert to true is to skip validation. Negative sentence hurts.

@targos targos added the confirmed-bug Issues with confirmed bugs. label Mar 7, 2016
@targos
Copy link
Member

targos commented Mar 7, 2016

The regression appeared between v5.1.1 and v5.2.0

@targos
Copy link
Member

targos commented Mar 7, 2016

I suspect fcf0e8e

@targos
Copy link
Member

targos commented Mar 7, 2016

Confirmed with git bisect.
cc @matthewloring

@matthewloring
Copy link

The commit linked by @princejwesley introduces the behavior where, if there would be overflow, we instead copy as much of the value as possible without overflowing. The check to see whether range errors should be thrown should occur above this failsafe logic. I'll put together a PR with this change.

Fishrock123 pushed a commit that referenced this issue Mar 22, 2016
The check to determine whether `noAssert` was set to true and thus
whether RangeErrors should be thrown was happening after the write was
truncated to the available size of the buffer. These checks now occur in
the correct order.

Fixes: #5587
PR-URL: #5605
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

No branches or pull requests

5 participants