Skip to content
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

Crash when trying to create buffers with invalid base64. #3496

Closed
ggreer opened this issue Oct 23, 2015 · 7 comments
Closed

Crash when trying to create buffers with invalid base64. #3496

ggreer opened this issue Oct 23, 2015 · 7 comments
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@ggreer
Copy link

ggreer commented Oct 23, 2015

I've been having this problem intermittently in production, and (with the help of @kans) managed to create a reproducible test case. This is on Ubuntu 15.04 using node.js v4.2.1 (built from source):

ggreer@lithium:~% node
> new Buffer("=" + new Array(10000).join("A"), "base64");
node: ../src/node_buffer.cc:225: v8::MaybeLocal<v8::Object> node::Buffer::New(v8::Isolate*, v8::Local<v8::String>, node::encoding): Assertion `(data) != (nullptr)' failed.
zsh: abort (core dumped)  node
ggreer@lithium:~% 

In the Buffer constructor (https://github.com/nodejs/node/blob/master/src/node_buffer.cc#L224), it looks like StringBytes::Write() fails and returns zero. Then realloc() is called with a length of zero. On linux, this frees the memory and returns a null pointer. Then the null assertion fails and node crashes. realloc() behaves differently on OS X, so this won't crash on a mac.

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Oct 23, 2015
@evanlucas
Copy link
Contributor

/cc @trevnorris

@ChALkeR
Copy link
Member

ChALkeR commented Oct 23, 2015

I can reproduce this.

@btipling
Copy link

I can confirm that this also crashes on Amazon Linux AMI:

$ nvm use node
Now using node v4.1.2 (npm v2.14.4)
$ node
> new Buffer("=" + new Array(10000).join("A"), "base64");
node: ../src/node_buffer.cc:224: v8::MaybeLocal<v8::Object> node::Buffer::New(v8::Isolate*, v8::Local<v8::String>, node::encoding): Assertion `(data) != (nullptr)' failed.
Aborted
$ cat /etc/issue
Amazon Linux AMI release 2014.09
Kernel \r on an \m

It did not crash on OS X for me.

@ggreer
Copy link
Author

ggreer commented Oct 23, 2015

Just FYI, this also crashes on FreeBSD 10.1-RELEASE:

ggreer@calcium:~% node
> new Buffer("=" + new Array(10000).join("A"), "base64");
Assertion failed: ((data) != (nullptr)), function New, file ../src/node_buffer.cc, line 225.
zsh: abort (core dumped)  node
ggreer@calcium:~% 

That's using node.js v4.2.1 from ports.

@jhamhader
Copy link
Contributor

@bnoordhuis - your PR seem to solve the crash which happens when failing to parse a string with output > 4096 (base64 input > 5641).
Just wondering - what is the desired behavior when the parsing of base64 string fails? The Buffer constructor currently returns an empty buffer which is indistinguishable from successful parsing of an empty input buffer.
Shouldn't it somehow indicate that the parsing has failed?

@ggreer
Copy link
Author

ggreer commented Oct 23, 2015

It'd be nice to throw an exception if invalid base64 was passed into new Buffer(), but I think that'd change the Buffer API. Currently, the Buffer constructor only throws RangeError, and only if you try to make a huge (2GB?) buffer.

Right now though, I just want node to not crash.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Oct 23, 2015
malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
that the standard allows them to either return a unique pointer or a
nullptr for zero-sized allocation requests.  Normalize by always using
a nullptr.

Fixes: nodejs#3496
PR-URL: nodejs#3499
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis
Copy link
Member

Just wondering - what is the desired behavior when the parsing of base64 string fails?

@jhamhader The base64 decoder's behavior is backwards compatible (going back all the way to v0.1.x, IIRC.) It's allowed to pass in base64 data with trailing gunk and (some) interior gunk.

To wit, one of my first contributions to node was a better base64 decoder. One of my first bug fixes was for the regression it introduced because it was too strict. :-)

If you want to ensure that all input has been decoded, you would have to validate it yourself. Assuming valid base64 without whitespace, the decoded size should be (size / 4) * 3, where size is a multiple of 4.

bnoordhuis added a commit that referenced this issue Oct 26, 2015
malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
that the standard allows them to either return a unique pointer or a
nullptr for zero-sized allocation requests.  Normalize by always using
a nullptr.

Fixes: #3496
PR-URL: #3499
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis added a commit that referenced this issue Oct 26, 2015
malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
that the standard allows them to either return a unique pointer or a
nullptr for zero-sized allocation requests.  Normalize by always using
a nullptr.

Fixes: #3496
PR-URL: #3499
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis added a commit that referenced this issue Oct 29, 2015
malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
that the standard allows them to either return a unique pointer or a
nullptr for zero-sized allocation requests.  Normalize by always using
a nullptr.

Fixes: #3496
PR-URL: #3499
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

No branches or pull requests

8 participants