Skip to content
This repository was archived by the owner on Dec 1, 2023. It is now read-only.

Remove calls to <Collection>::with_capacity(). #122

Closed
wants to merge 1 commit into from

Conversation

TyOverby
Copy link
Contributor

For instances where the length of a collection
must be obeyed, but is untrusted, calls to
with_capacity() could result in OOM errors.

This change makes it so that collections don't
pre-allocate memory at all.

For instances where the length of a collection
must be obeyed, but is untrusted, calls to
with_capacity() could result in OOM errors.

This change makes it so that collections don't
pre-allocate memory at all.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Contributor

This has the unfortunate consequence of the somewhat niche case you're protecting against leaking out and affecting all users of this library, so I'm somewhat not-inclined to merge this as it seems like the concerns are bleeding too much elsewhere into the library.

@huonw
Copy link
Contributor

huonw commented Jul 7, 2015

I'm also not inclined to merge this.

Just to clarify the background, this is protecting against a malicious attacker crafting input that includes a huge number for some length field, and hence causes the application to crash while allocating?

@TyOverby
Copy link
Contributor Author

@huonw: Yes. The main fix (but it's a breaking change) for this problem is here: #121.

@alexcrichton: Judging by your comment in #121, would you merge this if I replaced with_capacity(len) to with_capacity(min(1024, len))?

@alexcrichton
Copy link
Contributor

@TyOverby perhaps, but I haven't really thought through the ramifications of this. For example taking a moment to think about it more closely it may not matter too much because the main reason to preallocate is when you have a large array (e.g. so you don't have to reallocate so much). For small examples preallocating 1024 spots may not matter too much as they're small anyway.

@alexcrichton
Copy link
Contributor

superseded by #150

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

Successfully merging this pull request may close these issues.

4 participants