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: remove error for malformatted hex string #12012

Closed
wants to merge 1 commit into
base: master
from

Conversation

@Trott
Member

Trott commented Mar 23, 2017

Improve accuracy of the error message when a hex string of an incorrect
length is send to .write() or .fill().

Fixes: #3770

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

@Trott Trott added this to the 8.0.0 milestone Mar 23, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 23, 2017

Member

Rather than attempting to adjust the behavior of our code to match the error message (as in #3773) this takes the approach of adjusting the error message to more accurately report the issue that is found. (Of course, we can always implement more complete and robust error checking like in #3773 at a later date if deemed desirable and performant.)

Member

Trott commented Mar 23, 2017

Rather than attempting to adjust the behavior of our code to match the error message (as in #3773) this takes the approach of adjusting the error message to more accurately report the issue that is found. (Of course, we can always implement more complete and robust error checking like in #3773 at a later date if deemed desirable and performant.)

@Trott

This comment has been minimized.

Show comment
Hide comment
@targos

targos approved these changes Mar 24, 2017

Maybe a RangeError is appropriate?

Show outdated Hide outdated test/parallel/test-buffer-alloc.js Outdated
Show outdated Hide outdated src/node_buffer.cc Outdated
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 24, 2017

Member

Maybe a RangeError is appropriate?

If I got a RangeError, I would expect it to be something like the string contained characters that were invalid hex digits (/[^a-fA-F0-9]/). This error we're talking about here seems more like "invalid formatting" than "out of acceptable range" to me...

Member

Trott commented Mar 24, 2017

Maybe a RangeError is appropriate?

If I got a RangeError, I would expect it to be something like the string contained characters that were invalid hex digits (/[^a-fA-F0-9]/). This error we're talking about here seems more like "invalid formatting" than "out of acceptable range" to me...

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 24, 2017

Member

Reprinting an above comment down here for more visibility:

I'm starting to think this check isn't really worthwhile and the right thing to do is remove it. This error is still misleading in that if you get it, one would reasonably expect there's more validation going on for hex strings than just a length check. One would reasonably expect that we check for valid hex chars. But we don't. We happily allow out-of-range characters just as long as the string length is divisible by two.

The upshot is puzzling behavior like this:

> Buffer.from('xx', 'hex')
<Buffer >
> Buffer.from('x', 'hex')
TypeError: Invalid hex string
    at Buffer.write (buffer.js:769:21)
    at fromString (buffer.js:213:18)
    at Function.Buffer.from (buffer.js:105:12)
    at repl:1:8
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at REPLServer.defaultEval (repl.js:339:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)
> Buffer.from('abxx', 'hex')
<Buffer ab>
> Buffer.from('abx', 'hex')
TypeError: Invalid hex string
    at Buffer.write (buffer.js:769:21)
    at fromString (buffer.js:213:18)
    at Function.Buffer.from (buffer.js:105:12)
    at repl:1:8
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at REPLServer.defaultEval (repl.js:339:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)
> 

It would be a whole lot less puzzling if this happened:

> Buffer.from('xx', 'hex')
<Buffer >
> Buffer.from('x', 'hex')
<Buffer >
> Buffer.from('abxx', 'hex')
<Buffer ab>
> Buffer.from('abx', 'hex')
<Buffer ab>
>

Principle Of Least Astonishment to me says we either accept anything and do the best we can or else we do reasonably rigorous error checking.

Member

Trott commented Mar 24, 2017

Reprinting an above comment down here for more visibility:

I'm starting to think this check isn't really worthwhile and the right thing to do is remove it. This error is still misleading in that if you get it, one would reasonably expect there's more validation going on for hex strings than just a length check. One would reasonably expect that we check for valid hex chars. But we don't. We happily allow out-of-range characters just as long as the string length is divisible by two.

The upshot is puzzling behavior like this:

> Buffer.from('xx', 'hex')
<Buffer >
> Buffer.from('x', 'hex')
TypeError: Invalid hex string
    at Buffer.write (buffer.js:769:21)
    at fromString (buffer.js:213:18)
    at Function.Buffer.from (buffer.js:105:12)
    at repl:1:8
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at REPLServer.defaultEval (repl.js:339:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)
> Buffer.from('abxx', 'hex')
<Buffer ab>
> Buffer.from('abx', 'hex')
TypeError: Invalid hex string
    at Buffer.write (buffer.js:769:21)
    at fromString (buffer.js:213:18)
    at Function.Buffer.from (buffer.js:105:12)
    at repl:1:8
    at ContextifyScript.Script.runInThisContext (vm.js:23:33)
    at REPLServer.defaultEval (repl.js:339:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:101:20)
> 

It would be a whole lot less puzzling if this happened:

> Buffer.from('xx', 'hex')
<Buffer >
> Buffer.from('x', 'hex')
<Buffer >
> Buffer.from('abxx', 'hex')
<Buffer ab>
> Buffer.from('abx', 'hex')
<Buffer ab>
>

Principle Of Least Astonishment to me says we either accept anything and do the best we can or else we do reasonably rigorous error checking.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 24, 2017

Member

accept anything and do the best we can

That’s mostly what we do for base64, so doing it for hex too sounds good to me

Member

addaleax commented Mar 24, 2017

accept anything and do the best we can

That’s mostly what we do for base64, so doing it for hex too sounds good to me

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 25, 2017

Member

That’s mostly what we do for base64, so doing it for hex too sounds good to me

Done! PTAL

Member

Trott commented Mar 25, 2017

That’s mostly what we do for base64, so doing it for hex too sounds good to me

Done! PTAL

@Trott Trott changed the title from buffer: better error for malformated hex string to buffer: remove error for malformated hex string Mar 25, 2017

@Trott Trott changed the title from buffer: remove error for malformated hex string to buffer: remove error for malformatted hex string Mar 25, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 25, 2017

Member

@targos @cjihrig You probably want to re-review too now that this PR does something quite different :)

Member

addaleax commented Mar 25, 2017

@targos @cjihrig You probably want to re-review too now that this PR does something quite different :)

@targos

targos approved these changes Mar 25, 2017

// - https://github.com/nodejs/node/issues/6770
assert.throws(() => Buffer.from('A', 'hex'), TypeError);
// Test single hex character is discarded.
assert.strictEqual(Buffer.from('A', 'hex').length, 0);

This comment has been minimized.

@targos

targos Mar 25, 2017

Member

Can you add a test where the resulting length is >0 ?
Like Buffer.from('abx', 'hex')

@targos

targos Mar 25, 2017

Member

Can you add a test where the resulting length is >0 ?
Like Buffer.from('abx', 'hex')

This comment has been minimized.

@Trott

Trott Mar 25, 2017

Member

@targos Good idea. Added!

@Trott

Trott Mar 25, 2017

Member

@targos Good idea. Added!

@lpinca

lpinca approved these changes Mar 25, 2017

buffer: remove error for malformatted hex string
Remove error message when a hex string of an incorrect length is sent
to .write() or .fill().

Fixes: #3770
@jgeewax

This comment has been minimized.

Show comment
Hide comment
@jgeewax

jgeewax Mar 26, 2017

(Just chiming in as this relates to #4877)

It's clear that there's precedent here with base64 (it clearly just swallows any invalid characters):

> Buffer.from('not !**# base64 at all_... !!!!', 'base64').toString('base64')
'notbase64atall//'

Would it make sense to also add "strict mode" where invalid values are rejected?

In other words, I'd guess there is a not-insignificant group of folks out there who would prefer to be told when garbage goes into creating a new Buffer (since they're saying what the format should be). A strict mode wouldn't hurt those of us who just want the buffer to do "just do the best it can", and is super helpful to those of us that want to be notified when we accidentally send invalid values into the Buffer.

A few options that seem reasonable:

  • Flags like RegExp: Buffer.from(input, 'hex', 's')
  • Boolean for strict-mode: Buffer.from(input, 'base64', true)
  • A separate encoding type: Buffer.from(input, 'hex-strict')

Not sure if it matters also, but short-cut validation methods that check whether the Buffer.from(input, 'base64').toString('base64') == input could be useful (this might be out of scope of the Buffer class though...)

jgeewax commented Mar 26, 2017

(Just chiming in as this relates to #4877)

It's clear that there's precedent here with base64 (it clearly just swallows any invalid characters):

> Buffer.from('not !**# base64 at all_... !!!!', 'base64').toString('base64')
'notbase64atall//'

Would it make sense to also add "strict mode" where invalid values are rejected?

In other words, I'd guess there is a not-insignificant group of folks out there who would prefer to be told when garbage goes into creating a new Buffer (since they're saying what the format should be). A strict mode wouldn't hurt those of us who just want the buffer to do "just do the best it can", and is super helpful to those of us that want to be notified when we accidentally send invalid values into the Buffer.

A few options that seem reasonable:

  • Flags like RegExp: Buffer.from(input, 'hex', 's')
  • Boolean for strict-mode: Buffer.from(input, 'base64', true)
  • A separate encoding type: Buffer.from(input, 'hex-strict')

Not sure if it matters also, but short-cut validation methods that check whether the Buffer.from(input, 'base64').toString('base64') == input could be useful (this might be out of scope of the Buffer class though...)

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 26, 2017

Member

@jgeewax I definitely see that sort of error-checking safety as useful behavior and while I wish it was what Node.js did, it does seem like there's no reason that validating input like that can't be implemented as a userland module. EDIT: And if a userland module can show a way to do it in a performant way, that might even be a path into Node.js core. My sense right now is that most of the objections/hesitance about expanding the error checking is the performance hit incurred.

Member

Trott commented Mar 26, 2017

@jgeewax I definitely see that sort of error-checking safety as useful behavior and while I wish it was what Node.js did, it does seem like there's no reason that validating input like that can't be implemented as a userland module. EDIT: And if a userland module can show a way to do it in a performant way, that might even be a path into Node.js core. My sense right now is that most of the objections/hesitance about expanding the error checking is the performance hit incurred.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 26, 2017

Member

@jgeewax If we were designing Node from scratch, we’d probably implement one of your suggestions (which all make sense from an API perspective), but I think they all come with some kind of negative performance impact that we would want to avoid now.

Member

addaleax commented Mar 26, 2017

@jgeewax If we were designing Node from scratch, we’d probably implement one of your suggestions (which all make sense from an API perspective), but I think they all come with some kind of negative performance impact that we would want to avoid now.

@Trott

This comment has been minimized.

Show comment
Hide comment
Member

Trott commented Mar 28, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 28, 2017

Member

Landed in 682573c

Member

Trott commented Mar 28, 2017

Landed in 682573c

@Trott Trott closed this Mar 28, 2017

Trott added a commit to Trott/io.js that referenced this pull request Mar 28, 2017

buffer: remove error for malformatted hex string
Remove error message when a hex string of an incorrect length is sent
to .write() or .fill().

PR-URL: nodejs#12012
Fixes: nodejs#3770
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

leonardodino added a commit to leonardodino/basic-crypto that referenced this pull request Oct 31, 2017

fix build on node 8
buffer behaviour change
caused by: nodejs/node#12012
landed in: nodejs/node@682573c

@shuse2 shuse2 referenced this pull request Feb 28, 2018

Merged

Add multiple node.js version support - Closes #561 #562

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment