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
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@addaleax
Member

addaleax commented Nov 29, 2016

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

buffer: fix transcode for single-byte enc to ucs2
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

@addaleax addaleax added the buffer label Nov 29, 2016

@bnoordhuis

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);

This comment has been minimized.

@bnoordhuis

bnoordhuis Nov 29, 2016

Member

destbuf[0]?

@bnoordhuis

bnoordhuis Nov 29, 2016

Member

destbuf[0]?

This comment has been minimized.

@addaleax

addaleax Dec 5, 2016

Member

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

addaleax Dec 5, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
Member

addaleax commented Dec 5, 2016

@jasnell

jasnell approved these changes Dec 5, 2016

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 6, 2016

Member

Landed in 69b1a76

Member

addaleax commented Dec 6, 2016

Landed in 69b1a76

@addaleax addaleax closed this Dec 6, 2016

@addaleax addaleax deleted the addaleax:buffer-fix-transcode-to-ucs2 branch Dec 6, 2016

addaleax added a commit that referenced this pull request Dec 6, 2016

buffer: fix transcode for single-byte enc to ucs2
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

buffer: fix transcode for single-byte enc to ucs2
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

2016-12-06, Version 7.2.1 (Current)
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

2016-12-06, Version 7.2.1 (Current)
    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 added a commit to jmdarling/node that referenced this pull request Dec 8, 2016

buffer: fix transcode for single-byte enc to ucs2
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

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Dec 20, 2016

Member

@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?

Member

MylesBorins commented Dec 20, 2016

@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 addaleax referenced this pull request Dec 21, 2016

Closed

buffer: add buffer.transcode #9038

4 of 4 tasks complete
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 21, 2016

Member

@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 :)

Member

addaleax commented Dec 21, 2016

@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