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.from(string, 'hex') needs a documentation update (was: Buffer.from(x, hex).toString(hex) does not return x for certain values of x) #29786

Closed
iamrenejr opened this issue Oct 1, 2019 · 6 comments
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.

Comments

@iamrenejr
Copy link

iamrenejr commented Oct 1, 2019

  • Version: 10.16.3
  • Platform: Darwin Renes-MacBook-Pro.local 18.6.0 Darwin Kernel Version 18.6.0: Thu Apr 25 23:16:27 PDT 2019; root:xnu-4903.261.4~2/RELEASE_X86_64 x86_64
  • Subsystem: Buffer

Run these in the REPL:

 Buffer.from('0', 'hex').toString('hex')               // returns ''
 Buffer.from('08', 'hex').toString('hex')              // returns '08'
 Buffer.from('088', 'hex').toString('hex')             // returns '08'
 Buffer.from('0888', 'hex').toString('hex')            // returns '0888'
 Buffer.from('08888', 'hex').toString('hex')           // returns '0888'
 Buffer.from('088888', 'hex').toString('hex')          // returns '088888'

I expected .toString('hex') to be the inverse operation of Buffer.from(x, 'hex'), but it doesn't seem to be the case. Why not?

I was able to catch this error using fast-check and it found a few other strings that behave this way:

  • 8501eb788
  • eb788
  • 788
@Trott
Copy link
Member

Trott commented Oct 1, 2019

All of your problematic examples have an odd number of characters. Buffer.from() can't know how your hex-encoded string should be treated if it has an odd number of characters. Hex encoding means each byte is encoded as two hexadecimal characters. If you only provide one character instead, I imagine behavior is undefined, although I don't actually know. Surprisingly, I can't find mention of this in the documentation, so there may be a documentation pull request to be done here. A case can be made that this ought to throw an error but I wouldn't want to make that case before updating the docs.

People file issues like this from time to time which suggests that a doc update is in order. Here's a previous example: #24491

@Trott Trott added buffer Issues and PRs related to the buffer subsystem. Hacktoberfest good first issue Issues that are suitable for first-time contributors. labels Oct 1, 2019
@Trott Trott changed the title Buffer.from(x, hex).toString(hex) does not return x for certain values of x Buffer.from(string, 'hex') needs a documentation update (was: Buffer.from(x, hex).toString(hex) does not return x for certain values of x) Oct 1, 2019
@Trott
Copy link
Member

Trott commented Oct 1, 2019

Added hacktoberfest and good first issue labels in case someone wants to try their hand at updating the relevant documentation.

@Trott Trott added the doc Issues and PRs related to the documentations. label Oct 1, 2019
@tr0id tr0id mentioned this issue Oct 1, 2019
2 tasks
@AdityaSrivast
Copy link
Contributor

@Trott I would like to work on this issue

@Trott
Copy link
Member

Trott commented Oct 9, 2019

@Trott I would like to work on this issue

Go for it.

@Trott Trott removed Hacktoberfest good first issue Issues that are suitable for first-time contributors. labels Oct 9, 2019
HarshithaKP added a commit to HarshithaKP/node that referenced this issue Jan 14, 2020
@Trott Trott closed this as completed in e43ee37 Jan 17, 2020
codebytere pushed a commit that referenced this issue Feb 17, 2020
fixes: #29786
refs: #29792
refs: #24491

PR-URL: #31352
Fixes: #29786
Refs: #29792
Refs: #24491
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 14, 2020
fixes: #29786
refs: #29792
refs: #24491

PR-URL: #31352
Fixes: #29786
Refs: #29792
Refs: #24491
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 17, 2020
fixes: #29786
refs: #29792
refs: #24491

PR-URL: #31352
Fixes: #29786
Refs: #29792
Refs: #24491
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@reneklootwijk
Copy link

All of your problematic examples have an odd number of characters. Buffer.from() can't know how your hex-encoded string should be treated if it has an odd number of characters. Hex encoding means each byte is encoded as two hexadecimal characters. If you only provide one character instead, I imagine behavior is undefined, although I don't actually know. Surprisingly, I can't find mention of this in the documentation, so there may be a documentation pull request to be done here. A case can be made that this ought to throw an error but I wouldn't want to make that case before updating the docs.

People file issues like this from time to time which suggests that a doc update is in order. Here's a previous example: #24491

Then it is very inconsistent:
let n = 1000
console.log(n.toString(16)) => results in 3e8
console.log(Buffer.from(n.toString(16), 'hex')) => results in <Buffer 3e>

@Trott
Copy link
Member

Trott commented Sep 26, 2020

Then it is very inconsistent:
let n = 1000
console.log(n.toString(16)) => results in 3e8
console.log(Buffer.from(n.toString(16), 'hex')) => results in <Buffer 3e>

I suppose that suggests another possibility: add a leading 0 if needed so that '3e8' is treated like '03e8'. In addition to being a breaking change, that gets challenging for edge cases and may end up affecting performance. You wouldn't want to pad '3ez' but you would want to pad '3e8'. Things probably get weird fast there. @nodejs/buffer

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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants