buffer: ignore negative allocation lengths #7051

addaleax · 2016-05-29T18:17:41Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

buffer

Description of change

Treat negative length arguments to Buffer()/allocUnsafe() as if they were zero so the allocation does not affect the pool’s offset.

Ref: #7047

I decided to not throw an error here because there’s at least one existing test expecting Buffer(-1) to not throw, but I’d like to make a semver-major follow-up PR for adding a < 0 check to assertSize. (That would implement this suggestion by @thefourtheye, I think.)

/cc @nodejs/buffer

addaleax · 2016-05-29T18:20:58Z

CI: https://ci.nodejs.org/job/node-test-commit/3570/

thefourtheye · 2016-05-29T19:08:50Z

lib/buffer.js

@@ -173,6 +173,8 @@ Buffer.alloc = function(size, fill, encoding) {
 **/
 Buffer.allocUnsafe = function(size) {
  assertSize(size);
+  if (size <= 0)
+    return createBuffer(0);
  return allocate(size);


Would it be better if this check is in the allocate itself?

thefourtheye · 2016-05-29T19:18:53Z

LGTM. Let's hear from @jasnell and @trevnorris as well.

feross · 2016-05-30T01:08:57Z

LGTM

I believe this should get released soon because it may affect the security of user code. See #7047 (comment)

bnoordhuis · 2016-05-30T07:41:31Z

LGTM

bnoordhuis · 2016-05-30T09:10:54Z

Observation: Buffer.allocUnsafe(-1) returns an empty buffer, Buffer.allocUnsafeSlow(-1) throws an exception. Not very consistent.

thefourtheye · 2016-05-30T09:43:15Z

test/parallel/test-buffer.js

+assert.deepStrictEqual(Buffer.allocUnsafe(-100), Buffer.from(''));
+
+// Check pool offset after that by trying to write string into the pool.
+assert.doesNotThrow(() => Buffer.from('abc'));


It would be better if the negative allocation also done within this test itself. If the code is moved around later, it will not fail

Maybe wrap in a { } closure?

ChALkeR · 2016-05-30T10:30:26Z

LGTM

ChALkeR · 2016-05-30T10:34:22Z

@bnoordhuis Those should probably both throw, but changing that would be a semver-major.

Fishrock123 · 2016-05-30T14:06:40Z

@nodejs/ctc ptal, especially re: the above

addaleax · 2016-05-30T14:47:15Z

Updated with @Fishrock123’s suggestion of wrapping the test block in { }

trevnorris · 2016-05-30T14:53:05Z

Has the problematic commit been bisected? I fixed a similar issue a couple years ago and it should have been throwing (@bnoordhuis may remember this as one of my first contributions, finding how to read from random locations in memory).

I'd change this PR to throw. We already do if the range is to large, and in the case Ben already mentioned.

Edit: I see an existing test where it returned a zero length Buffer on negative number. I'll need to verify something.

addaleax · 2016-05-30T14:57:45Z

@trevnorris I’m pretty sure this comes from the introduction of Buffer.alloc*, see the this comment that’s also linked above in the description (also strongly supported by the fact that this bug popped up in v5.10.0).

I’d be happy to change this PR to throwing if there’s a clear majority here that thinks that would pass as a semver-patch; as mentioned, there’s at least one test expecting the old behaviour (from d157131).

trevnorris · 2016-05-30T15:09:49Z

Alright, seems the case I'm thinking of is in 498200b (thanks Ben for fixing that) and has to do with SlowBuffer.

For expediency, LGTM for the presented fix, but I'd also suggest we consider throwing in the next semver-major.

Now, i'm back off to my vacation. Thanks all for taking care of this.

trevnorris · 2016-05-30T15:13:38Z

@bnoordhuis

Observation: Buffer.allocUnsafe(-1) returns an empty buffer, Buffer.allocUnsafeSlow(-1) throws an exception. Not very consistent.

I'm sure you realize this, but for posterity, that's pretty much because allocUnsafeSlow() is basically an extension of SlowBuffer, which you made to throw on invalid size in the commit listed just above. I agree this is the behavior Buffer should follow, but also I'm sure there'll be plenty of debate about whether a breaking change should be allowed back into v6. So I say we introduce that behavior in v7, land this in v6 and call it a day.

addaleax · 2016-05-30T15:17:30Z

@trevnorris A semver-major follow-up is exactly the plan, yep. Have fun on your vacation!

EDIT: One more CI: https://ci.nodejs.org/job/node-test-commit/3587/

rvagg · 2016-05-31T05:49:23Z

LGTM, ignore that ARM failure, those are new machines and the error is very unlikely to be Node specific (same OOM errors showing up for other runs too). Also 👍 from me for the approach of following up with a semver-major @addaleax.

trevnorris · 2016-05-31T16:38:12Z

For posterity, the regression was introduced in c1534e7. Making, I think, a good case for why larger changes like that don't, at least in the short or medium term, get backported to LTS releases.

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: nodejs#7047 PR-URL: nodejs#7051 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

addaleax · 2016-05-31T18:00:08Z

Thanks for the reviews, everyone.

PR for throwing upon encountering a negative argument: #7079

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: #7047 PR-URL: #7051 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

@zkat

# Notable changes ## Notable changes * **buffer**: Ignore negative lengths in calls to `Buffer()` and `Buffer.allocUnsafe()`. This fixes a possible security concern (reported by Feross Aboukhadijeh) where user input is passed unchecked to the Buffer constructor or `allocUnsafe()` as it can expose parts of the memory slab used by other Buffers in the application. Note that negative lengths are not supported by the Buffer API and user input to the constructor should always be sanitised and type-checked. (Anna Henningsen) [#7051](nodejs/node#7051) * **npm**: Upgrade npm to 3.9.3 (Kat Marchán) [#7030](nodejs/node#7030) - [`npm/npm@42d71be`](npm/npm@42d71be) [npm/npm#12685](npm/npm#12685) When using `npm ls <pkg>` without a semver specifier, `npm ls` would skip any packages in your tree that matched by name, but had a prerelease version in their `package.json`. ([@zkat](https://github.com/zkat)) - [`npm/npm@f04e05`](npm/npm@df04e05) [npm/npm#10013](npm/npm#10013) `read-package-tree@5.1.4`: Fixes an issue where `npm install` would fail if your `node_modules` was symlinked. ([@iarna](https://github.com/iarna)) - [`b894413`](npm/npm@b894413) [#12372](npm/npm#12372) Changing a nested dependency in an `npm-shrinkwrap.json` and then running `npm install` would not get up the updated package. This corrects that. ([@misterbyrne](https://github.com/misterbyrne)) - This release includes `npm@3.9.0`, which is the result of our Windows testing push -- the test suite (should) pass on Windows now. We're working on getting AppVeyor to a place where we can just rely on it like Travis. * **tty**: Default to blocking mode for stdio on OS X. A bug fix in libuv 1.9.0, introduced in Node.js v6.0.0, exposed problems with Node's use of non-blocking stdio, particularly on OS X which has a small output buffer. This change should fix CLI applications that have been having problems with output since Node.js v6.0.0 on OS X. The core team is continuing to address stdio concerns that exist across supported platforms and progress can be tracked at <nodejs/node#6980>. (Jeremiah Senkpiel) [#6895](nodejs/node#6895) * **V8**: Upgrade to V8 5.0.71.52. This includes a fix that addresses problems experienced by users of node-inspector since Node.js v6.0.0, see <node-inspector/node-inspector#864> for details. (Michaël Zasso) [#6928](nodejs/node#6928)

ChALkeR · 2016-06-05T22:36:42Z

This should be backported to 5.x, perhaps — 5.x is still alive.

addaleax · 2016-06-05T22:39:46Z

fwiw, the commit here lands cleanly on v5.x, so the rest is mostly on @nodejs/release I think?

ChALkeR · 2016-06-05T23:08:47Z

@addaleax I'm going to file a PR to 5.x with several buffer-related backports in several minutes (after the tests pass).

ChALkeR · 2016-06-05T23:24:58Z

@addaleax Backport in #7169.

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: nodejs#7047 Refs: nodejs#7051 PR-URL: nodejs#7221 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: nodejs#7047 Refs: nodejs#7051 Refs: nodejs#7221 Refs: nodejs#7475 PR-URL: nodejs#7562 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

Treat negative length arguments to `Buffer()`/`allocUnsafe()` as if they were zero so the allocation does not affect the pool’s offset. Fixes: #7047 Refs: #7051 Refs: #7221 Refs: #7475 PR-URL: #7562 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label May 29, 2016

addaleax mentioned this pull request May 29, 2016

Inexplicable "RangeError: Attempt to write outside buffer bounds" #7047

Closed

thefourtheye reviewed May 29, 2016
View reviewed changes

addaleax force-pushed the buffer-negative-allocunsafe branch from 8196086 to 6796278 Compare May 29, 2016 19:13

thefourtheye reviewed May 30, 2016
View reviewed changes

Fishrock123 mentioned this pull request May 30, 2016

Propose v6.2.1 #6934

Closed

Fishrock123 added this to the 6.2.1 milestone May 30, 2016

addaleax force-pushed the buffer-negative-allocunsafe branch from 1902371 to ef9a8fa Compare May 31, 2016 17:50

addaleax merged commit ef9a8fa into nodejs:master May 31, 2016

addaleax deleted the buffer-negative-allocunsafe branch May 31, 2016 17:54

yorkie mentioned this pull request May 31, 2016

buffer: throw on negative .allocUnsafe() argument #7079

Closed

3 tasks

addaleax mentioned this pull request May 31, 2016

buffer: improve creation performance #6893

Closed

2 tasks

rvagg mentioned this pull request Jun 2, 2016

Release proposal: v6.2.1 #7108

Closed

chrjean mentioned this pull request Jun 4, 2016

Update NodeJs to 6.2.1 (security update) ScoopInstaller/Scoop#891

Merged

addaleax added the dont-land-on-v4.x label Jun 5, 2016

ChALkeR mentioned this pull request Jun 5, 2016

Backport important Buffer changes to v5.x #7169

Merged

ChALkeR mentioned this pull request Jun 8, 2016

Backport «buffer: ignore negative allocation lengths» to v5.x #7221

Merged

ChALkeR mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: ignore negative allocation lengths #7051

buffer: ignore negative allocation lengths #7051

addaleax commented May 29, 2016

addaleax commented May 29, 2016

thefourtheye May 29, 2016

addaleax May 29, 2016 •

edited

Loading

thefourtheye commented May 29, 2016

feross commented May 30, 2016 •

edited

Loading

bnoordhuis commented May 30, 2016

bnoordhuis commented May 30, 2016

thefourtheye May 30, 2016

Fishrock123 May 30, 2016

ChALkeR commented May 30, 2016

ChALkeR commented May 30, 2016 •

edited

Loading

Fishrock123 commented May 30, 2016

addaleax commented May 30, 2016

trevnorris commented May 30, 2016 •

edited

Loading

addaleax commented May 30, 2016

trevnorris commented May 30, 2016 •

edited

Loading

trevnorris commented May 30, 2016

addaleax commented May 30, 2016 •

edited

Loading

rvagg commented May 31, 2016

trevnorris commented May 31, 2016 •

edited

Loading

addaleax commented May 31, 2016

ChALkeR commented Jun 5, 2016

addaleax commented Jun 5, 2016

ChALkeR commented Jun 5, 2016 •

edited

Loading

ChALkeR commented Jun 5, 2016

buffer: ignore negative allocation lengths #7051

buffer: ignore negative allocation lengths #7051

Conversation

addaleax commented May 29, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax commented May 29, 2016

thefourtheye May 29, 2016

Choose a reason for hiding this comment

addaleax May 29, 2016 • edited Loading

Choose a reason for hiding this comment

thefourtheye commented May 29, 2016

feross commented May 30, 2016 • edited Loading

bnoordhuis commented May 30, 2016

bnoordhuis commented May 30, 2016

thefourtheye May 30, 2016

Choose a reason for hiding this comment

Fishrock123 May 30, 2016

Choose a reason for hiding this comment

ChALkeR commented May 30, 2016

ChALkeR commented May 30, 2016 • edited Loading

Fishrock123 commented May 30, 2016

addaleax commented May 30, 2016

trevnorris commented May 30, 2016 • edited Loading

addaleax commented May 30, 2016

trevnorris commented May 30, 2016 • edited Loading

trevnorris commented May 30, 2016

addaleax commented May 30, 2016 • edited Loading

rvagg commented May 31, 2016

trevnorris commented May 31, 2016 • edited Loading

addaleax commented May 31, 2016

ChALkeR commented Jun 5, 2016

addaleax commented Jun 5, 2016

ChALkeR commented Jun 5, 2016 • edited Loading

ChALkeR commented Jun 5, 2016

addaleax May 29, 2016 •

edited

Loading

feross commented May 30, 2016 •

edited

Loading

ChALkeR commented May 30, 2016 •

edited

Loading

trevnorris commented May 30, 2016 •

edited

Loading

trevnorris commented May 30, 2016 •

edited

Loading

addaleax commented May 30, 2016 •

edited

Loading

trevnorris commented May 31, 2016 •

edited

Loading

ChALkeR commented Jun 5, 2016 •

edited

Loading