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

buffer: let WriteFloatGeneric silently drop values #3767

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@pmq20
Contributor

pmq20 commented Nov 11, 2015

Fixes #3766

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.

@pmq20 pmq20 force-pushed the pmq20:master-fix-3766 branch Nov 11, 2015

@thefourtheye thefourtheye added the buffer label Nov 11, 2015

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Nov 11, 2015

@@ -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);

This comment has been minimized.

@trevnorris

trevnorris Nov 11, 2015

Contributor

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

This comment has been minimized.

@pmq20

pmq20 Nov 11, 2015

Contributor

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);

This comment has been minimized.

@trevnorris

trevnorris Nov 11, 2015

Contributor

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

This comment has been minimized.

Contributor

trevnorris commented Nov 11, 2015

@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

This comment has been minimized.

Member

jasnell commented Nov 12, 2015

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

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Nov 12, 2015

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

@trevnorris trevnorris self-assigned this Nov 12, 2015

@pmq20

This comment has been minimized.

Contributor

pmq20 commented Nov 13, 2015

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

@thefourtheye

View changes

test/parallel/test-buffer-arraybuffer.js Outdated
@@ -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

This comment has been minimized.

@thefourtheye

thefourtheye Nov 13, 2015

Contributor

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

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Nov 13, 2015

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

@pmq20 pmq20 force-pushed the pmq20:master-fix-3766 branch to f28cea5 Nov 16, 2015

@pmq20

This comment has been minimized.

Contributor

pmq20 commented Nov 16, 2015

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Nov 16, 2015

@pmq20

This comment has been minimized.

Contributor

pmq20 commented Nov 17, 2015

@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

This comment has been minimized.

Contributor

trevnorris commented Nov 17, 2015

Landed in 0ed3a7c. Thanks much!

@trevnorris trevnorris closed this Nov 17, 2015

mscdex referenced this pull request Nov 17, 2015

buffer: let WriteFloatGeneric silently drop values
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>

Fishrock123 referenced this pull request Nov 17, 2015

buffer: let WriteFloatGeneric silently drop values
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>

Fishrock123 added a commit that referenced this pull request Nov 17, 2015

2015-11-17, Version 5.1.0 (Stable)
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

bbondy added a commit to brave/node that referenced this pull request Mar 13, 2016

2015-11-17, Version 5.1.0 (Stable)
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

bbondy added a commit to brave/node that referenced this pull request Mar 14, 2016

2015-11-17, Version 5.1.0 (Stable)
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

bbondy added a commit to brave/node that referenced this pull request Mar 14, 2016

2015-11-17, Version 5.1.0 (Stable)
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

bbondy added a commit to brave/node that referenced this pull request Mar 15, 2016

2015-11-17, Version 5.1.0 (Stable)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment