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

Check for overflow in DroplessArena and align returned memory #73237

Merged
merged 2 commits into from
Jun 16, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jun 11, 2020

  • Check for overflow when calculating the slice start & end position.
  • Align the pointer obtained from the allocator, ensuring that it
    satisfies user requested alignment (the allocator is only asked for
    layout compatible with u8 slice).
  • Remove an incorrect assertion from DroplessArena::align.
  • Avoid forming references to an uninitialized memory in DroplessArena.

Helps with #73007, #72624.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (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. Due to 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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2020
@petrochenkov
Copy link
Contributor

r? @nnethercote

src/librustc_arena/lib.rs Outdated Show resolved Hide resolved
assert!(bytes != 0);
loop {
if let Some(a) = self.alloc_raw_fast(bytes, align) {
break a;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens here on overflow? It'll call self.grow(bytes). Then what will happen -- panic, success? I think the possibilities need to be clearer here. In particular, alloc_raw_fast() returns None in two cases: "overflow", and "need to grow". I feel like those two cases need to be distinguished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From perspective of code that tried to use the current chunk the situation is the same: there is no capacity left and arena needs to grow. Overflow does not necessarily preclude later success if say the chunk had merely a high address, requested capacity was large, and the calculation overflowed.

The code then delegates the responsibility to handle the situation to grow and subsequently to RawVec. Under assumption that grow doesn't hit checked_mul(2).unwrap() (this could be saturated to isize::MAX to be a little bit friendlier to 32-bit systems), the allocation is performed by RawVec which panics when requested capacity exceeds isize::MAX (on 32-bit systems) and aborts on OOM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. Could you put that information into a comment? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now with additional comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that. Sorry to nitpick this further, but you've added a comment explaining the high-level behaviour, which is fairly obvious, while not adding a comment distinguishing the overflow vs. need-to-grow cases, which is less obvious.

Can you add a comment inside the function explaining those two cases, much like your comment above? I specifically would like to see explanation of the "high address + large capacity" case, in particular. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a different comment explaining the abstract check we want to perform.
In my view the overflow is the same as the need-to-grow case, which is why I
didn't want to comment about the distinction. I just don't see any.

@tmiasko tmiasko force-pushed the arena branch 2 times, most recently from 194318a to 03360b4 Compare June 12, 2020 13:13
src/librustc_arena/lib.rs Outdated Show resolved Hide resolved
assert!(bytes != 0);
loop {
if let Some(a) = self.alloc_raw_fast(bytes, align) {
break a;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that. Sorry to nitpick this further, but you've added a comment explaining the high-level behaviour, which is fairly obvious, while not adding a comment distinguishing the overflow vs. need-to-grow cases, which is less obvious.

Can you add a comment inside the function explaining those two cases, much like your comment above? I specifically would like to see explanation of the "high address + large capacity" case, in particular. Thanks.

* Check for overflow when calculating the slice start & end position.
* Align the pointer obtained from the allocator, ensuring that it
  satisfies user requested alignment (the allocator is only asked for
  layout compatible with u8 slice).
* Remove an incorrect assertion from DroplessArena::align.
Return a pointer from `alloc_raw` instead of a slice. There is no
practical use for slice as a return type and changing it to a pointer
avoids forming references to an uninitialized memory.
@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 15, 2020

I also changed the return type of alloc_raw to a pointer instead of a slice.
Not only it is already used like that, but also avoids forming references to
uninitialized memory as described in #72624.

@nnethercote
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2020

📌 Commit 1f08951 has been approved by nnethercote

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 15, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
Check for overflow in DroplessArena and align returned memory

* Check for overflow when calculating the slice start & end position.
* Align the pointer obtained from the allocator, ensuring that it
  satisfies user requested alignment (the allocator is only asked for
  layout compatible with u8 slice).
* Remove an incorrect assertion from DroplessArena::align.
* Avoid forming references to an uninitialized memory in DroplessArena.

Helps with rust-lang#73007, rust-lang#72624.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#73237 (Check for overflow in DroplessArena and align returned memory)
 - rust-lang#73339 (Don't run generator transform when there's a TyErr)
 - rust-lang#73372 (Re-order correctly the sections in the sidebar)
 - rust-lang#73373 (Use track caller for bug! macro)
 - rust-lang#73380 (Add more info to `x.py build --help` on default value for `-j JOBS`.)
 - rust-lang#73381 (Fix typo in docs of std::mem)
 - rust-lang#73389 (Use `Ipv4Addr::from<[u8; 4]>` when possible)
 - rust-lang#73400 (Fix forge-platform-support URL)

Failed merges:

r? @ghost
@bors bors merged commit c65f39d into rust-lang:master Jun 16, 2020
@tmiasko tmiasko deleted the arena branch June 16, 2020 18:27
@yoshuawuyts
Copy link
Member

@tmiasko I didn't realize #73007 was merged; thanks so much for enabling that with this PR! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants