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

Fix fromHex invalid hex behavior #303

Merged
merged 3 commits into from
Oct 11, 2023
Merged

Conversation

junderw
Copy link
Contributor

@junderw junderw commented Oct 16, 2021

In NodeJS, the behavior of Buffer.from(x, 'hex') differed slightly on its handling of bad hex parsing.

My fix also increased performance. I tried parsing a hex string '7c'.repeat(5e7) and it went from 2.8 s to ~800ms.

// node
> Buffer.from('abc def01','hex')
<Buffer ab>
// this library
> require('./index.js').Buffer.from('abc def01','hex')
<Buffer ab 0c de f0>

@junderw
Copy link
Contributor Author

junderw commented Oct 16, 2021

Note: This new implementation also mimics the code for NodeJS hex parser used in the from(x, 'hex') portion.

https://github.com/nodejs/node/blob/v14.18.1/src/string_bytes.cc#L246-L261

index.js Show resolved Hide resolved
@dcousens dcousens changed the title BugFix: NodeJS Buffer behavior fix + performance increase Fix fromHex invalid hex behavior (#303) Oct 11, 2023
@dcousens dcousens changed the title Fix fromHex invalid hex behavior (#303) Fix fromHex invalid hex behavior Oct 11, 2023
@dcousens dcousens merged commit 23aa85c into feross:master Oct 11, 2023
1 of 5 checks passed
@dcousens dcousens self-assigned this Oct 11, 2023
Comment on lines +21 to +22
assert.deepStrictEqual(buf, new Buffer([0xab]));
assert.strictEqual(buf.toString('hex'), 'ab');
Copy link
Contributor

Choose a reason for hiding this comment

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

buf here is a 4 bytes long buffer, so these asserts can never succeed? Did you try this locally? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

buf should NOT be a 4 byte buffer. That's the point.

If it is a 4 byte buffer, than the code is broken.

a non-hex character should end the hex parsing and only the 2-hex pairs up until the first non-hex is output as a Buffer.

in this case, [0xab]

Copy link
Contributor

Choose a reason for hiding this comment

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

But you are doing .write on an already existing Buffer that is 4 bytes long. write cannot truncate the length of the buffer.

With my updated tests it checks that write only wrote 1 byte, and that it left the three bytes after as 00 00 00.

@LinusU
Copy link
Contributor

LinusU commented Jan 23, 2024

This broke both the linting and the tests, I've submitted PRs to fix that: #340, #341

ping @dcousens

dcousens pushed a commit that referenced this pull request Jan 23, 2024
@dcousens
Copy link
Collaborator

With CI passing now, I'm slightly more confident with this going out (see #335)

@chjj
Copy link
Contributor

chjj commented Jan 23, 2024

@dcousens, please look at #343 before tagging a release. This PR (#303) added a massive slowdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants