buffer: let WriteFloatGeneric silently drop values #3767

pmq20 · 2015-11-11T07:39:14Z

Quoted from buffer.markdown,

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.

Therefore I let node::Buffer::WriteFloatGeneric silently drop values instead of crashing.

thefourtheye · 2015-11-11T07:42:16Z

cc @trevnorris

trevnorris · 2015-11-11T07:52:55Z

src/node_buffer.cc

@@ -735,7 +735,9 @@ uint32_t WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {

  T val = args[1]->NumberValue();
  uint32_t offset = args[2]->Uint32Value();
-  CHECK_LE(offset + sizeof(T), ts_obj_length);
+  size_t memcpy_num = sizeof(T);


Just curious, why initialize memcpy_num to sizeof(T)?

Just a coding style thing, it is the same as,

if (offset + sizeof(T) > ts_obj_length) memcpy_num = ts_obj_length - offset; else memcpy_num = sizeof(T);

ah I see. was focused on that memcpy_num = sizeof(T) was set then immediately after there's a check sizeof(T) in the conditional. nm it then. :)

trevnorris · 2015-11-11T22:52:37Z

@pmq20 hm. possibly the index returned should be the end of the buffer and not just index + 8. don't really care though. Everything LGTM.

CI is down ATM. will queue this up when it becomes available. Thanks much for the fix!

jasnell · 2015-11-12T16:57:58Z

@trevnorris ... would this be a bug fix or a behavior change? Think this would be appropriate for LTS?

trevnorris · 2015-11-12T17:03:57Z

@jasnell Since the alternative is to flat out abort, I'd consider this a bug fix.

pmq20 · 2015-11-13T02:26:38Z

Thank you all for reviewing. Please let me know if I could do more.

thefourtheye · 2015-11-13T02:32:22Z

test/parallel/test-buffer-arraybuffer.js

@@ -44,3 +44,7 @@ assert.throws(function() {
  AB.prototype.__proto__ = ArrayBuffer.prototype;
  new Buffer(new AB());
 }, TypeError);
+
+// writeFloatBE with noAssert should not crash, cf. #3766


Comment says BE, but the code uses LE. May be add test for that also.

trevnorris · 2015-11-13T19:13:31Z

@pmq20 If you wouldn't mind adding similar tests to cover all the write{Double,Float}{LE,BE} variants (ping me when you do) I'll put this through CI and land it.

Fixes nodejs#3766

pmq20 · 2015-11-16T03:20:24Z

@trevnorris done

trevnorris · 2015-11-16T23:09:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/753/

pmq20 · 2015-11-17T02:58:31Z

@trevnorris The failures are on ARM:

  ...
not ok 132 test-crypto-dh.js
# TIMEOUT
  ---
  duration_ms: 120.25

not ok 129 test-crypto-binary-default.js
# TIMEOUT
  ---
  duration_ms: 120.101

They seem unrelated to this PR.

trevnorris · 2015-11-17T04:31:33Z

Landed in 0ed3a7c. Thanks much!

Documentation currently states that setting noAssert and passing a value larger than can fit in the Buffer will cause data to be silently dropped. Change implementation to match documented behavior. Fixes: #3766 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Notable changes: * buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) #3767. - This makes the behavior match what the docs suggest. * child_process: `child.send()` now properly returns a boolean like the docs suggest (Rich Trott) #3577. * doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) #3662. * http_parser: update http-parser to 2.6.0 from 2.5.0 (James M Snell) #3569. - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`, `REBIND`, `UNBIND`. - Also added ACL and IPv6 Zone ID support. * npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner) #3685. - See the release notes for https://github.com/npm/npm/releases/tag/v3.3.7, https://github.com/npm/npm/releases/tag/v3.3.8, https://github.com/npm/npm/releases/tag/v3.3.9, https://github.com/npm/npm/releases/tag/v3.3.10, https://github.com/npm/npm/releases/tag/v3.3.11, and https://github.com/npm/npm/releases/tag/v3.3.12 for more details. * repl: The REPL no longer crashes if the persistent history file cannot be opened (Evan Lucas) #3630. * tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) #3755. * v8: Added some more useful post-mortem data (Fedor Indutny) #3779. PR-URL: #3736

Notable changes: * buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767. - This makes the behavior match what the docs suggest. * child_process: `child.send()` now properly returns a boolean like the docs suggest (Rich Trott) nodejs/node#3577. * doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662. * http_parser: update http-parser to 2.6.0 from 2.5.0 (James M Snell) nodejs/node#3569. - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`, `REBIND`, `UNBIND`. - Also added ACL and IPv6 Zone ID support. * npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner) nodejs/node#3685. - See the release notes for https://github.com/npm/npm/releases/tag/v3.3.7, https://github.com/npm/npm/releases/tag/v3.3.8, https://github.com/npm/npm/releases/tag/v3.3.9, https://github.com/npm/npm/releases/tag/v3.3.10, https://github.com/npm/npm/releases/tag/v3.3.11, and https://github.com/npm/npm/releases/tag/v3.3.12 for more details. * repl: The REPL no longer crashes if the persistent history file cannot be opened (Evan Lucas) nodejs/node#3630. * tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755. * v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779. PR-URL: nodejs/node#3736

pmq20 force-pushed the master-fix-3766 branch from 7f1e7c7 to 960821a Compare November 11, 2015 07:40

thefourtheye added the buffer Issues and PRs related to the buffer subsystem. label Nov 11, 2015

trevnorris mentioned this pull request Nov 11, 2015

buffer: move checkFloat from lib into src #3763

Closed

trevnorris reviewed Nov 11, 2015
View reviewed changes

jasnell added the lts-watch-v4.x label Nov 12, 2015

trevnorris added the land-on-v5.x label Nov 12, 2015

trevnorris self-assigned this Nov 12, 2015

thefourtheye reviewed Nov 13, 2015
View reviewed changes

buffer: let WriteFloatGeneric silently drop values

f28cea5

Fixes nodejs#3766

pmq20 force-pushed the master-fix-3766 branch from 960821a to f28cea5 Compare November 16, 2015 02:59

trevnorris closed this Nov 17, 2015

MylesBorins added lts-landed-on-v4.x and removed lts-watch-v4.x labels Nov 30, 2015

MylesBorins added land-on-v4.x and removed lts-landed-on-v4.x labels Dec 17, 2015

MylesBorins added the land-on-v4.x label Dec 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: let WriteFloatGeneric silently drop values #3767

buffer: let WriteFloatGeneric silently drop values #3767

pmq20 commented Nov 11, 2015

thefourtheye commented Nov 11, 2015

trevnorris Nov 11, 2015

pmq20 Nov 11, 2015

trevnorris Nov 11, 2015

trevnorris commented Nov 11, 2015

jasnell commented Nov 12, 2015

trevnorris commented Nov 12, 2015

pmq20 commented Nov 13, 2015

thefourtheye Nov 13, 2015

trevnorris commented Nov 13, 2015

pmq20 commented Nov 16, 2015

trevnorris commented Nov 16, 2015

pmq20 commented Nov 17, 2015

trevnorris commented Nov 17, 2015

buffer: let WriteFloatGeneric silently drop values #3767

buffer: let WriteFloatGeneric silently drop values #3767

Conversation

pmq20 commented Nov 11, 2015

thefourtheye commented Nov 11, 2015

trevnorris Nov 11, 2015

Choose a reason for hiding this comment

pmq20 Nov 11, 2015

Choose a reason for hiding this comment

trevnorris Nov 11, 2015

Choose a reason for hiding this comment

trevnorris commented Nov 11, 2015

jasnell commented Nov 12, 2015

trevnorris commented Nov 12, 2015

pmq20 commented Nov 13, 2015

thefourtheye Nov 13, 2015

Choose a reason for hiding this comment

trevnorris commented Nov 13, 2015

pmq20 commented Nov 16, 2015

trevnorris commented Nov 16, 2015

pmq20 commented Nov 17, 2015

trevnorris commented Nov 17, 2015