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

buffer: fix transcode for single-byte enc to ucs2 #9838

Closed

Conversation

addaleax
Copy link
Member

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

Fix buffer.transcode() for transcoding from single-byte character
encodings to UCS2.

These would previously perform out-of-bounds reads due to an
incorrect sizeof() expression.

Fixes: #9834

/cc @nodejs/buffer

Fix `buffer.transcode()` for transcoding from single-byte character
encodings to UCS2.

These would previously perform out-of-bounds reads due to an
incorrect `sizeof()` expression.

Fixes: nodejs#9834
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 29, 2016
@addaleax addaleax added the buffer Issues and PRs related to the buffer subsystem. label Nov 29, 2016
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

@@ -179,7 +179,7 @@ MaybeLocal<Object> TranscodeToUcs2(Isolate* isolate,
MaybeLocal<Object> ret;
MaybeStackBuffer<UChar> destbuf(source_length);
Converter from(fromEncoding);
const size_t length_in_chars = source_length * sizeof(*destbuf);
const size_t length_in_chars = source_length * sizeof(UChar);
Copy link
Member

Choose a reason for hiding this comment

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

destbuf[0]?

Copy link
Member Author

Choose a reason for hiding this comment

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

The other instances of sizeof in this file use **destbuf or UChar. I’m fine with using destbuf[0] everywhere but right now I wouldn’t want to add more confusion here.

@addaleax
Copy link
Member Author

addaleax commented Dec 5, 2016

@addaleax
Copy link
Member Author

addaleax commented Dec 6, 2016

Landed in 69b1a76

@addaleax addaleax closed this Dec 6, 2016
@addaleax addaleax deleted the buffer-fix-transcode-to-ucs2 branch December 6, 2016 00:47
addaleax added a commit that referenced this pull request Dec 6, 2016
Fix `buffer.transcode()` for transcoding from single-byte character
encodings to UCS2.

These would previously perform out-of-bounds reads due to an
incorrect `sizeof()` expression.

Fixes: #9834
PR-URL: #9838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Dec 6, 2016
Fix `buffer.transcode()` for transcoding from single-byte character
encodings to UCS2.

These would previously perform out-of-bounds reads due to an
incorrect `sizeof()` expression.

Fixes: #9834
PR-URL: #9838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 added a commit that referenced this pull request Dec 6, 2016
Notable changes:

* buffer:
  - Reverted the runtime deprecation of calling `Buffer()` without
`new`. (Anna Henningsen) #9529
  - Fixed `buffer.transcode()` for single-byte character
encodings to `UCS2`. (Anna Henningsen)
#9838
* promise: `--trace-warnings` now produces useful stacktraces for
Promise warnings. (Anna Henningsen)
#9525
* repl: Fixed a bug preventing correct parsing of generator functions.
(Teddy Katz) #9852
* V8: Fixed a significant `instanceof` performance regression.
(Franziska Hinkelmann) #9730

PR-URL: #10127
imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 7, 2016
    Notable changes:

    * buffer:
      - Reverted the runtime deprecation of calling `Buffer()` without
    `new`. (Anna Henningsen) nodejs/node#9529
      - Fixed `buffer.transcode()` for single-byte character
    encodings to `UCS2`. (Anna Henningsen)
    nodejs/node#9838
    * promise: `--trace-warnings` now produces useful stacktraces for
    Promise warnings. (Anna Henningsen)
    nodejs/node#9525
    * repl: Fixed a bug preventing correct parsing of generator functions.
    (Teddy Katz) nodejs/node#9852
    * V8: Fixed a significant `instanceof` performance regression.
    (Franziska Hinkelmann) nodejs/node#9730

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
Fix `buffer.transcode()` for transcoding from single-byte character
encodings to UCS2.

These would previously perform out-of-bounds reads due to an
incorrect `sizeof()` expression.

Fixes: nodejs#9834
PR-URL: nodejs#9838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@addaleax this is not landing cleanly on v6.x or v4.x

There are some other commits that need to get pulled for this to land cleanly, should we backport?

@addaleax
Copy link
Member Author

@thealphanerd This affects a feature added in #9038, which exists only in v7. I’m removing the lts-watch labels here and left a comment over there, let me know if you need anything else :)

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. 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.

6 participants