buffer: let WriteFloatGeneric silently drop values by pmq20 · Pull Request #3767 · nodejs/node · GitHub

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.

cc @trevnorris

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. :)

@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!

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

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

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

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

@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

@trevnorris done

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

@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.

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

buffer: let WriteFloatGeneric silently drop values by pmq20 · Pull Request #3767 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: let WriteFloatGeneric silently drop values - #3767

buffer: let WriteFloatGeneric silently drop values#3767pmq20 wants to merge 1 commit intonodejs:masternodejs/node:masterfrom pmq20:master-fix-3766pmq20/node:master-fix-3766Copy head branch name to clipboard

Conversation

pmq20 commented Nov 11, 2015

Uh oh!

thefourtheye commented Nov 11, 2015

Uh oh!

trevnorris Nov 11, 2015

Choose a reason for hiding this comment

Uh oh!

pmq20 Nov 11, 2015

Choose a reason for hiding this comment

Uh oh!

trevnorris Nov 11, 2015

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Nov 11, 2015

Uh oh!

jasnell commented Nov 12, 2015

Uh oh!

trevnorris commented Nov 12, 2015

Uh oh!

pmq20 commented Nov 13, 2015

Uh oh!

thefourtheye Nov 13, 2015

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Nov 13, 2015

Uh oh!

pmq20 commented Nov 16, 2015

Uh oh!

trevnorris commented Nov 16, 2015

Uh oh!

pmq20 commented Nov 17, 2015

Uh oh!

trevnorris commented Nov 17, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

buffer: let WriteFloatGeneric silently drop values#3767
pmq20 wants to merge 1 commit into
nodejs:masterfrom
pmq20:master-fix-3766