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

Cap calls to with_capacity with sane default #150

Merged
merged 1 commit into from
Mar 30, 2016

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 more than
1MB of memory.

@TyOverby
Copy link
Contributor Author

This should be close to the final implementation, however I still need to write a test that would crash current HEAD, but passes with this change.

@alexcrichton
Copy link
Contributor

Looks good to me, thanks! I'm fine merging with or without a test

@TyOverby
Copy link
Contributor Author

I have a few more questions:

What happens inside of Vec and HashMap when they contain zero-sized types? My implementation lets them allocate all they want under the assumption that there isn't really any allocation being done. Is this a reasonable assumption?

Do you want me to include the version number bump?

@alexcrichton
Copy link
Contributor

For Vec at least I believe no allocations are made, for HashMap though I'm not sure (cc @pczarn, @cgaebel, @gereeter, sorry, I forget who's most familiar with HashMap internals!). I think, however, it's a reasonable assumption.

I'll include a separate version bump after this, so no need to include it here

@gereeter
Copy link

Vec definitely does not allocate when fed zero-sized types, but HashMap and BTreeMap currently do. However, there isn't any particular reason why they have to allocate - the code optimizing for ZSTs just hasn't been written.

@TyOverby
Copy link
Contributor Author

@alexcrichton I've updated the PR to cap with_capacity for zero-sized-types.

@alexcrichton
Copy link
Contributor

Thanks! Did you want to add some tests as well?

@TyOverby
Copy link
Contributor Author

@alexcrichton I haven't yet, but I will tonight.

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 more than
1MB of memory.
@TyOverby
Copy link
Contributor Author

@alexcrichton The code has been updated with a test.

@alexcrichton alexcrichton merged commit 0a962d2 into rust-lang-deprecated:master Mar 30, 2016
@alexcrichton
Copy link
Contributor

Thanks @TyOverby!

@TyOverby
Copy link
Contributor Author

TyOverby commented Apr 4, 2016

@alexcrichton Could you bump the version number and publish to crates.io? I'd like to include a regression test in bincode for this scenario.

@alexcrichton
Copy link
Contributor

Huh I thought I did that... sure though!

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.

3 participants