Skip to content

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.

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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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