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: use @chainsafe/is-ip #280

Merged

Conversation

wemeetagain
Copy link
Contributor

@wemeetagain wemeetagain commented Oct 29, 2022

Problem:
is-ip 5.0.0 is significantly slower than 4.0.0. This is due to adding protection against regex DoS.
It does so by wrapping its regex calls in node:vm apis with a timeout.
Although the resulting code is protected against regex DoS, its also ~6000x slower, so slow that it shows up with non-negligible time in Lodestar cpu profiles.

Solution:
Replace is-ip dependency with @chainsafe/is-ip.
@chainsafe/is-ip is a drop-in replacement for is-ip that is a transliteration of some of rust's std net code - this.
It's ~300x faster for valid IPv4 values, ~30x faster for valid IPv6 values than is-ip 5.0.0. And it's not vulnerable to regex DoS because it doesn't use regex, rather it uses a simple parser built just for the job of parsing IP addr strings.

See discussion:

@achingbrain achingbrain changed the title chore: use @chainsafe/is-ip fix: use @chainsafe/is-ip Oct 29, 2022
@achingbrain
Copy link
Member

Will need a follow up PR to replace is-ip in https://github.com/frenchbread/private-ip

@achingbrain achingbrain merged commit 40124ff into multiformats:master Oct 29, 2022
@achingbrain
Copy link
Member

Thanks!

github-actions bot pushed a commit that referenced this pull request Oct 29, 2022
## [11.0.6](v11.0.5...v11.0.6) (2022-10-29)

### Bug Fixes

* use @chainsafe/is-ip ([#280](#280)) ([40124ff](40124ff))
@github-actions
Copy link

🎉 This PR is included in version 11.0.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants