v6.6.0 node::PBKDF2() Out of Memory #8571

scttcper · 2016-09-17T04:25:41Z

The following code crashes in v6.6.0 on OSX 10.11.6. v6.5.0 does not crash.

running

var crypto = require('crypto');
var salt = new Buffer('McWpw6FL29zJ6E97Le3hKQ==', 'base64');
crypto.pbkdf2('', salt, 1, 32, "sha256", function(error, saltedPassword) {
  console.log(error);
  console.log(saltedPassword);
});

results in

FATAL ERROR: node::PBKDF2() Out of Memory
 1: node::Abort() [/Users/scoope7/.nvm/versions/node/v6.6.0/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/scoope7/.nvm/versions/node/v6.6.0/bin/node]
 3: node::ClearFatalExceptionHandlers(node::Environment*) [/Users/scoope7/.nvm/versions/node/v6.6.0/bin/node]
 4: node::crypto::RandomBytesWork(uv_work_s*) [/Users/scoope7/.nvm/versions/node/v6.6.0/bin/node]
 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/scoope7/.nvm/versions/node/v6.6.0/bin/node]
 6: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>) [/Users/scoope7/.nvm/versions/node/v6.6.0/bin/node]
 7: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/scoope7/.nvm/versions/node/v6.6.0/bin/node]
 8: 0xe4335f092a7
[1]    45139 abort      node crash.js

This was code extracted out of https://github.com/neumino/rethinkdbdash that was crashing a project.

The text was updated successfully, but these errors were encountered:

not-an-aardvark · 2016-09-17T04:34:37Z

I was able to reproduce this on OSX 10.11.6.

MarkHerhold · 2016-09-17T04:34:47Z

@scttcper Thanks for reporting this! I just hit the same issue in my project which also uses rethinkdbdash, so I assume the issue is the same.

mscdex · 2016-09-17T04:41:13Z

Confirmed on Linux as well.

not-an-aardvark · 2016-09-17T04:44:05Z

It's possible a00ccb0 is the cause, I'll bisect to verify.

mscdex · 2016-09-17T04:50:40Z

I can confirm that reverting a00ccb0 fixes it.

/cc @mhdawson @addaleax @bnoordhuis

mscdex · 2016-09-17T04:57:47Z

I think we may need to add a length check for every nullptr check, like what was done in ed640ae? It seems like there are many uses of node::Malloc() without that extra check.

Trott · 2016-09-17T05:49:56Z

I think we may need to add a length check for every nullptr check, like what was done in ed640ae?

Sure seems like that change set needs to be gone over to find nullptr checks that might be affected.

As far as the specific issue here, test case and proposed fix at #8572

niieani · 2016-09-17T09:38:45Z

I'm also getting a crash when running tsc compilation (TypeScript) with 6.6.0. It works with 6.5.0. Not sure if it's the same error though.

not-an-aardvark · 2016-09-17T15:47:51Z

@niieani, would you mind posting the stack trace of the tsc crash (assuming there is a stack trace)? This will help us triage the issue to figure out if it's the same one, or create a new fix if not.

ghiscoding · 2016-09-17T18:53:42Z

I can also confirm that reverting back to NodeJS v6.5 fixes the problem (running on Win10). I will stick with 6.5 until resolution.

niieani · 2016-09-18T09:07:21Z

@not-an-aardvark No stack trace, unless I need to pass some parameter to force displaying it? tsc just ends abruptly without actually doing anything, while on 6.5.0 it works properly.

doomedramen · 2016-09-19T12:07:16Z

Error reproduced on macOS Sierra with node v6.6.0.

juicelink · 2016-09-19T15:38:48Z

Same error on official node 6 docker image that reference 6.6.0 version since a few days. Switched to a custom image ( no 6.5 official image available)

johanbrandhorst · 2016-09-20T08:10:15Z

@juicelink You don't happen to have a public vanilla 6.5 image on docker hub we can all peruse until this blows over?

Fixes: nodejs#8571

juicelink · 2016-09-21T11:14:30Z

juicelink/node image is the official one downgraded to 6.5.0

niieani · 2016-09-21T13:40:38Z

Since this came up now, something like this might also happen in the future. May we ask the maintainers of the docker images to keep tagging minor versions, not only major ones? I.e. have both the :6 tag, for the latest Node 6 and :6.0, :6.1, etc. for minor ones.

mormahr · 2016-09-21T14:32:55Z

The Docker Library still has the old versions :)
So "FROM node:6.5" should work

Change `Malloc()/Calloc()` so that size zero does not return a null pointer, consistent with prior behavior. Fixes: nodejs#8571

mofax · 2016-09-22T04:50:08Z

Just updated to v6.6.0 on ubuntu 16.04, and this showed up; everything crashed

Trott · 2016-09-22T05:51:32Z

This is fixed in d2eb7ce. That should be in the next release in the Node.js version 6.x line (which will be either 6.6.1 or 6.7.0). I believe it will also be in the next Node.js version 7 beta which should be out next week. (First beta came out today.)

pkese · 2016-09-23T13:45:06Z

I would assume, now that the bug is fixed, that a new version of nodejs would be pushed out with this fix applied ASAP, i.e. hopefully before next week.

Lots of people who followed the recommended install procedure from nodesource.com are now having this broken 6.6.0 version (.deb or .rpm) installed on their linux systems. It is a major pain to find workarounds (pinning old package versions or simply not updating their linux distros) just because of this bug.

MylesBorins · 2016-09-23T13:57:24Z

@pkese unfortunately we do not do Friday releases for a number of reasons. There will be a security update coming out next Tuesday, and I believe we intent to ship the above fix as part of it.

https://nodejs.org/en/blog/vulnerability/september-2016-security-releases/

Change `Malloc()/Calloc()` so that size zero does not return a null pointer, consistent with prior behavior. Fixes: #8571 PR-URL: #8572 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins · 2016-09-23T14:14:26Z

I've gone ahead and backported this to v6.x-staging, this will help to make sure it isn't missed

Change `Malloc()/Calloc()` so that size zero does not return a null pointer, consistent with prior behavior. Fixes: #8571 PR-URL: #8572 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins · 2016-09-28T04:27:38Z

This was released with v6.7.0

Change `Malloc()/Calloc()` so that size zero does not return a null pointer, consistent with prior behavior. Fixes: #8571 PR-URL: #8572 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Change `Malloc()/Calloc()` so that size zero does not return a null pointer, consistent with prior behavior. Fixes: nodejs#8571 PR-URL: nodejs#8572 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Change `Malloc()/Calloc()` so that size zero does not return a null pointer, consistent with prior behavior. Fixes: #8571 PR-URL: #8572 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Assuming that UncheckedMalloc(0) returns a non-nullptr is non-standard and we use other allocators as well (e.g., OPENSSL_malloc) that do not guarantee this behavior. It is the caller's responsibility to check that size != 0 implies UncheckedMalloc(size) != nullptr, and that's exactly what the checked variants (Malloc etc.) already do. The current behavior is also inconsistent with UncheckedRealloc(), which always returns a nullptr when the size is 0, and with the documentation in src/README.md. Refs: nodejs#8571 Refs: nodejs#8572

Assuming that UncheckedMalloc(0) returns a non-nullptr is non-standard and we use other allocators as well (e.g., OPENSSL_malloc) that do not guarantee this behavior. It is the caller's responsibility to check that size != 0 implies UncheckedMalloc(size) != nullptr, and that's exactly what the checked variants (Malloc etc.) already do. The current behavior is also inconsistent with UncheckedRealloc(), which always returns a nullptr when the size is 0, and with the documentation in src/README.md as well as with multiple comments in the source code. This changes UncheckedMalloc(), UncheckedCalloc(), and UncheckedRealloc() to always return a nullptr when the size is 0 instead of doing fake allocations in UncheckedMalloc() and UncheckedCalloc() while returning a nullptr from UncheckedRealloc(). This is consistent with existing documentation and comments. Refs: nodejs#8571 Refs: nodejs#8572

Assuming that UncheckedMalloc(0) returns a non-nullptr is non-standard and we use other allocators as well (e.g., OPENSSL_malloc) that do not guarantee this behavior. It is the caller's responsibility to check that size != 0 implies UncheckedMalloc(size) != nullptr, and that's exactly what the checked variants (Malloc etc.) already do. The current behavior is also inconsistent with UncheckedRealloc(), which always returns a nullptr when the size is 0, and with the documentation in src/README.md as well as with multiple comments in the source code. This changes UncheckedMalloc(), UncheckedCalloc(), and UncheckedRealloc() to always return a nullptr when the size is 0 instead of doing fake allocations in UncheckedMalloc() and UncheckedCalloc() while returning a nullptr from UncheckedRealloc(). This is consistent with existing documentation and comments. Refs: #8571 Refs: #8572 PR-URL: #44543 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: James M Snell <[email protected]>

scttcper mentioned this issue Sep 17, 2016

Node v6.6.0 PBKDF2() Out of Memory neumino/rethinkdbdash#293

Closed

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Sep 17, 2016

not-an-aardvark added the confirmed-bug Issues with confirmed bugs. label Sep 17, 2016

Trott mentioned this issue Sep 17, 2016

src: Malloc/Calloc size 0 returns non-null pointer #8572

Closed

3 tasks

davidbanham added a commit to Prismatik/dendritic that referenced this issue Sep 18, 2016

Pin to 6.5 until nodejs/node#8571 is resolved

379490a

mscdex mentioned this issue Sep 20, 2016

src: return static buffer on malloc(0) #8658

Closed

3 tasks

mscdex added a commit to mscdex/io.js that referenced this issue Sep 20, 2016

src: return static buffer on malloc(0)

070e10d

Fixes: nodejs#8571

fidian mentioned this issue Sep 21, 2016

Jasmine test helpers opentoken-io/opentoken#115

Merged

Trott added a commit to Trott/io.js that referenced this issue Sep 21, 2016

src: Malloc/Calloc size 0 returns non-null pointer

5a6e9db

Change `Malloc()/Calloc()` so that size zero does not return a null pointer, consistent with prior behavior. Fixes: nodejs#8571

daffl mentioned this issue Sep 21, 2016

Travis tests running out of memory when starting RethinkDB server feathersjs-ecosystem/feathers-rethinkdb#41

Closed

Trott closed this as completed in d2eb7ce Sep 22, 2016

tniessen mentioned this issue Sep 6, 2022

src: remove UncheckedMalloc(0) workaround #44543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v6.6.0 node::PBKDF2() Out of Memory #8571

v6.6.0 node::PBKDF2() Out of Memory #8571

scttcper commented Sep 17, 2016 •

edited

Loading

not-an-aardvark commented Sep 17, 2016

MarkHerhold commented Sep 17, 2016

mscdex commented Sep 17, 2016

not-an-aardvark commented Sep 17, 2016

mscdex commented Sep 17, 2016 •

edited

Loading

mscdex commented Sep 17, 2016 •

edited

Loading

Trott commented Sep 17, 2016

niieani commented Sep 17, 2016

not-an-aardvark commented Sep 17, 2016 •

edited

Loading

ghiscoding commented Sep 17, 2016

niieani commented Sep 18, 2016

doomedramen commented Sep 19, 2016

juicelink commented Sep 19, 2016

johanbrandhorst commented Sep 20, 2016

juicelink commented Sep 21, 2016

niieani commented Sep 21, 2016

mormahr commented Sep 21, 2016 •

edited

Loading

mofax commented Sep 22, 2016

Trott commented Sep 22, 2016

pkese commented Sep 23, 2016

MylesBorins commented Sep 23, 2016

MylesBorins commented Sep 23, 2016

MylesBorins commented Sep 28, 2016

v6.6.0 node::PBKDF2() Out of Memory #8571

v6.6.0 node::PBKDF2() Out of Memory #8571

Comments

scttcper commented Sep 17, 2016 • edited Loading

not-an-aardvark commented Sep 17, 2016

MarkHerhold commented Sep 17, 2016

mscdex commented Sep 17, 2016

not-an-aardvark commented Sep 17, 2016

mscdex commented Sep 17, 2016 • edited Loading

mscdex commented Sep 17, 2016 • edited Loading

Trott commented Sep 17, 2016

niieani commented Sep 17, 2016

not-an-aardvark commented Sep 17, 2016 • edited Loading

ghiscoding commented Sep 17, 2016

niieani commented Sep 18, 2016

doomedramen commented Sep 19, 2016

juicelink commented Sep 19, 2016

johanbrandhorst commented Sep 20, 2016

juicelink commented Sep 21, 2016

niieani commented Sep 21, 2016

mormahr commented Sep 21, 2016 • edited Loading

mofax commented Sep 22, 2016

Trott commented Sep 22, 2016

pkese commented Sep 23, 2016

MylesBorins commented Sep 23, 2016

MylesBorins commented Sep 23, 2016

MylesBorins commented Sep 28, 2016

scttcper commented Sep 17, 2016 •

edited

Loading

mscdex commented Sep 17, 2016 •

edited

Loading

mscdex commented Sep 17, 2016 •

edited

Loading

not-an-aardvark commented Sep 17, 2016 •

edited

Loading

mormahr commented Sep 21, 2016 •

edited

Loading