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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Safe Buffer usage #790

Merged
merged 3 commits into from
May 23, 2017
Merged

Safe Buffer usage #790

merged 3 commits into from
May 23, 2017

Conversation

dcousens
Copy link
Contributor

@dcousens dcousens commented Apr 19, 2017 •
edited
Loading

@fanatid review would be appreciated 馃憤 .

Non-controversial, but, sensitive and I'd like at least 2 extra eyes since it touches the ECDSA module.

This is not a breaking change (we're already at a minimum of Node 4.5.0)

@dcousens dcousens requested a review from fanatid April 19, 2017 07:43
@dcousens dcousens self-assigned this Apr 19, 2017
@dcousens dcousens mentioned this pull request Apr 19, 2017
Closed
@fanatid
Copy link
Member

fanatid commented Apr 19, 2017

I like from/alloc/allocUnsafe, but this is not supported by node < 4.5.0 :(
do you sure about this?

@dcousens
Copy link
Contributor Author

dcousens commented Apr 19, 2017 •
edited
Loading

@fanatid we already bumped to 4.5.0 minimum with the 3.0.0 release whether we like it or not, as we use bs58check@2.0.0 ... we could revert that if necessary, but doesn't change the fact that this PR, right now, isn't a breaking change.

Personally, I'd rather patch the CHANGELOG to reflect #741

@dcousens
Copy link
Contributor Author

dcousens commented Apr 19, 2017 •
edited
Loading

So I guess this PR #750 was based on the wrong version number then... it should have been 4.5.0?

fanatid
fanatid reviewed Apr 19, 2017
View reviewed changes
src/script_number.js
if (b & 0x80) return -((b & ~0x80) * 0x100000000 + a)
return b * 0x100000000 + a
if (b & 0x80) return -(((b & ~0x80) * 0x100000000) + a)
return (b * 0x100000000) + a
Copy link
Member

Choose a reason for hiding this comment

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

extra parentheses for readable?

Copy link
Contributor Author

@dcousens dcousens Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

standard@latest warns on these, haven't bumped yet in repository though - indeed it is just readability

@dcousens
Copy link
Contributor Author

dcousens commented Apr 19, 2017 •
edited
Loading

Should we use .alloc in places without fixed length? (aka, potentially malicious user input?)

@fanatid
Copy link
Member

fanatid commented Apr 19, 2017

Currently engines.node is >=4.0.0, but all nodes < 4.5.0 not support from/alloc/allocUnsafe ...
https://github.com/bitcoinjs/bitcoinjs-lib/blob/v3.0.0/package.json#L7

fanatid
fanatid reviewed Apr 19, 2017
View reviewed changes
src/address.js
@@ -18,7 +18,7 @@ function fromBase58Check (address) {
function toBase58Check (hash, version) {
typeforce(types.tuple(types.Hash160bit, types.UInt8), arguments)

var payload = new Buffer(21)
var payload = Buffer.allocUnsafe(21)
Copy link
Member

@fanatid fanatid Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I see two variants in this PR: Buffer.alloc(length) and Buffer.allocUnsafe(length). Can you choose one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fanatid I've used Buffer.alloc all through the tests... and Buffer.allocUnsafe in parts of the code where were previously using new Buffer; so no change.

We should probably only need need Buffer.alloc in cases of user input no?

Copy link
Member

@fanatid fanatid Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

allocUnsafe (from docs) uses internal memory pool, maybe we should use alloc everywhere? (at least in bitcoinjs-lib)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this code is tested well enough that we can use both?

@dcousens
Copy link
Contributor Author

dcousens commented Apr 19, 2017 •
edited
Loading

Wait... bs58check originally broke because it relied on bs58 [which uses base-x] which had the Buffer.from introduced by mistake... but that has since been reverted and we only lost 0.12... so actually this is a breaking change.

Blimey.
Waiting on travis to confirm.

@dcousens dcousens added this to the 4.0.0 milestone Apr 19, 2017
@fanatid
Copy link
Member

fanatid commented Apr 19, 2017

travis will be fine, because 4 there is a link to latest 4.x

@fanatid
Copy link
Member

fanatid commented Apr 19, 2017

base-x was bumped correctly https://github.com/cryptocoinjs/base-x/blob/v3.0.0/package.json#L41

@dcousens
Copy link
Contributor Author

dcousens commented Apr 19, 2017 •
edited
Loading

@fanatid only after a screw around.
Hence my confusion; and why yes, this is a breaking change.

Well, I guess this isn't merging any time soon then. Ha.

@dcousens
Copy link
Contributor Author

Re-targeting 3.0.0 by using safe-buffer

@dcousens
Copy link
Contributor Author

Rebased, using safe-buffer and good to merge for a patch release

@dcousens dcousens mentioned this pull request May 23, 2017
Closed
@dcousens dcousens merged commit fe0ad81 into master May 23, 2017
@dcousens dcousens deleted the safebuf branch May 23, 2017 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanatid fanatid fanatid approved these changes

@jprichardson jprichardson Awaiting requested review from jprichardson

@afk11 afk11 Awaiting requested review from afk11

Assignees

@dcousens dcousens

Labels
refactor
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dcousens @fanatid