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

Limit buffer capacity in multiaddr visit_seq #1833

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

pawanjay176
Copy link
Contributor

Buffer allocated in visit_seq does not have an upper limit. Limit buffer capacity to 4096.

Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

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

Just to clarify, I'm not so familiar with the serialisation code: Since this is just for Vec::with_capacity, this limits only the initial capacity of the buffer, so that you cannot make the deserialisation code allocate an arbitrarily large Vec without actually "sending" that much data (e.g. if the Multiaddr is received over the network), since the Vec will still grow beyond its capacity as necessary. Correct?

@pawanjay176
Copy link
Contributor Author

@romanb yes that is correct. Without a limit, you can send malformed input such that seq.size_hint() returns a very large value and crashes the program even if the following data is pretty small.

@mxinden mxinden merged commit f7ab4f7 into libp2p:master Nov 14, 2020
@mxinden
Copy link
Member

mxinden commented Nov 14, 2020

Thanks @pawanjay176!

@mxinden
Copy link
Member

mxinden commented Nov 14, 2020

I pushed 2e6b0ec to update patch version and changelog. Should we do a release already, or wait till the next larger release?

@romanb
Copy link
Contributor

romanb commented Nov 14, 2020

No reason to wait, a multiaddr patch release seems appropriate.

@mxinden
Copy link
Member

mxinden commented Nov 14, 2020

For the record parity-multiaddr v0.9.5 is now released and published.

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.

3 participants