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

Missing wraparound checks in DroplessArena allocation #72624

Closed
bluss opened this issue May 26, 2020 · 1 comment
Closed

Missing wraparound checks in DroplessArena allocation #72624

bluss opened this issue May 26, 2020 · 1 comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@bluss
Copy link
Member

bluss commented May 26, 2020

DroplessArena::alloc_raw does not check for wraparound when computing the end of the allocation, pointer arithmetic using self.ptr and bytes:

rust/src/libarena/lib.rs

Lines 382 to 391 in aeca4d6

pub fn alloc_raw(&self, bytes: usize, align: usize) -> &mut [u8] {
unsafe {
assert!(bytes != 0);
self.align(align);
let future_end = intrinsics::arith_offset(self.ptr.get(), bytes as isize);
if (future_end as *mut u8) >= self.end.get() {
self.grow(bytes);
}

This can be used to make the pointer wrap around, and "allocate", bumping the pointer, without growing the underlying allocation.

Callers alloc and alloc_slice can possibly be argued to be safe due to practical size limits on values and slices, but at least alloc_from_iter can be used to trigger this bug and write out of bounds of an allocation.

Fixes to make

  1. Check arithmetic and ensure the allocation can fit into the current (or any) chunk

(Suggested) cleanups to make

  1. The arith_offset intrinsic is the same thing as <*mut T>::wrapping_add, and the method should be preferred.
  2. alloc_raw should return something else than &mut [u8], because the contents of the slice are uninit. For example a raw slice or a slice of MaybeUninit.

This came up in discussion in PR #72417

@bluss bluss added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. labels May 26, 2020
@Elinvynia Elinvynia added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 9, 2020
@LeSeulArtichaut LeSeulArtichaut added T-libs Relevant to the library team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 9, 2020
@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Jun 14, 2020

Marking this as p-medium based on our prioritization discussion`.

@Dylan-DPC-zz Dylan-DPC-zz added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 14, 2020
Manishearth added a commit to Manishearth/rust that referenced this issue 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.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants