Skip to content
This repository has been archived by the owner on Jan 11, 2022. It is now read-only.

Fix long string decode #91

Merged
merged 1 commit into from
Jul 15, 2020
Merged

Fix long string decode #91

merged 1 commit into from
Jul 15, 2020

Conversation

wemeetagain
Copy link
Contributor

Fix a few edge-cases in decoding long strings that previously could cause OOM crash.
Found investigating ChainSafe/discv5#64

@ryanio
Copy link
Contributor

ryanio commented Jul 12, 2020

thanks @wemeetagain, this looks great.

i noticed the coverage step was failing to push to coveralls due to:

[error] "2020-07-10T20:06:25.737Z"  'error from getOptions'
##[error]Error: Command failed: git cat-file -p 3ebc75f2f9e2e2368d93fd9c4d671536713be8de
fatal: Not a valid object name 3ebc75f2f9e2e2368d93fd9c4d671536713be8de

which I actually just ran into at web3.js a few days ago. the solution was to update actions/checkout to v2 (see here for more). i opened #92 to fix.

@holgerd77
Copy link
Member

Thanks @wemeetagain, I have merged in #92, this would need a branch update/rebase here for CI to re-run, don't have the permissions to trigger myself.

@holgerd77
Copy link
Member

(please give an explicit comment statement once you have done, these branch updates always tends to get lost/overlooked otherwise)

@wemeetagain
Copy link
Contributor Author

Rebased off latest!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 90.598% when pulling 4d30598 on ChainSafe:fix-long-string-decode into a3e2f2e on ethereumjs:master.

ryanio
ryanio reviewed Jul 14, 2020
View reviewed changes
test/dataTypes.spec.ts
@@ -5,15 +5,36 @@ const BN = require('bn.js')
const Buffer = require('buffer').Buffer // needed for karma

describe('invalid rlps', function() {
it('should not crash on an invalid rlp', function() {
const errCases = [
// prettier-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 oh that's neat

ryanio
ryanio approved these changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

Lgtm!

@holgerd77 holgerd77 merged commit 8015e24 into ethereumjs:master Jul 15, 2020
@holgerd77
Copy link
Member

@wemeetagain thanks for fixing, will directly prepare a release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanio ryanio ryanio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wemeetagain @ryanio @holgerd77 @coveralls