Skip to content

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

Closed
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:two-byte-value-off-by-one-write
Closed

src: fix 1-byte out-of-bounds write in TwoByteValue#6330
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:two-byte-value-off-by-one-write

Conversation

@addaleax
Copy link
Copy Markdown
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
Comment thread src/util.cc Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: #6330
@jasnell
Copy link
Copy Markdown
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
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