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

src: fix 1-byte out-of-bounds write in TwoByteValue #6330

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Apr 21, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

src, buffer (Buffer.fill is the only place where TwoByteValue is being used)

Description of change

Plan 2 bytes instead of 1 byte for the final zero terminator for UTF-16. This is unlikely to cause real-world problems, but that ultimately depends on the malloc implementation.

The issue can be uncovered by running e.g.
valgrind node -e "Buffer(65536).fill('a'.repeat(4096), 'utf16le')".

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

@addaleax addaleax added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 21, 2016
@@ -47,7 +47,8 @@ TwoByteValue::TwoByteValue(Isolate* isolate, Local<Value> value)
return;

// Allocate enough space to include the null terminator
size_t len = StringBytes::StorageSize(isolate, string, UCS2) + 1;
size_t len = StringBytes::StorageSize(isolate, string, UCS2) +
sizeof(uint16_t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Break right after the '=' and indent the next line by four spaces. Otherwise LGTM and good catch.

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

LGTM

Plan 2 bytes instead of 1 byte for the final zero terminator
for UTF-16. This is unlikely to cause real-world problems,
but that ultimately depends on the `malloc` implementation.

The issue can be uncovered by running e.g.
`valgrind node -e "Buffer(65536).fill('a'.repeat(4096), 'utf16le')"`
@addaleax addaleax force-pushed the two-byte-value-off-by-one-write branch from c482574 to 5a322bc Compare April 21, 2016 19:59
@jasnell
Copy link
Member

jasnell commented Apr 22, 2016

@addaleax ... wanna get this one landed? I'm cutting v6 RC.4 shortly and would like to get this one in

@addaleax
Copy link
Member Author

Done, landed in a3b5b9c. :)

@addaleax addaleax closed this Apr 22, 2016
@addaleax addaleax deleted the two-byte-value-off-by-one-write branch April 22, 2016 18:30
addaleax added a commit that referenced this pull request Apr 22, 2016
Plan 2 bytes instead of 1 byte for the final zero terminator
for UTF-16. This is unlikely to cause real-world problems,
but that ultimately depends on the `malloc` implementation.

The issue can be uncovered by running e.g.
`valgrind node -e "Buffer(65536).fill('a'.repeat(4096), 'utf16le')"`

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #6330
@jasnell
Copy link
Member

jasnell commented Apr 22, 2016

Thank you!

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
Plan 2 bytes instead of 1 byte for the final zero terminator
for UTF-16. This is unlikely to cause real-world problems,
but that ultimately depends on the `malloc` implementation.

The issue can be uncovered by running e.g.
`valgrind node -e "Buffer(65536).fill('a'.repeat(4096), 'utf16le')"`

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#6330
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Plan 2 bytes instead of 1 byte for the final zero terminator
for UTF-16. This is unlikely to cause real-world problems,
but that ultimately depends on the `malloc` implementation.

The issue can be uncovered by running e.g.
`valgrind node -e "Buffer(65536).fill('a'.repeat(4096), 'utf16le')"`

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #6330
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants