diff options
author | Carl Lerche <me@carllerche.com> | 2020-10-21 14:43:57 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-10-21 14:43:57 -0700 |
commit | b48fec96551ac95768b76102703c4039a64c1168 (patch) | |
tree | 11b24f3863a28010c7f16380ef35c03857f15d6c | |
parent | 8dbc3c79379f2243fc04d444239d009c1c610016 (diff) |
net: fix use-after-free in slab compaction (#3019)
An off-by-one bug results in freeing the incorrect page. This
also adds an `asan` CI job.
Fixes: 3014
-rw-r--r-- | .github/workflows/ci.yml | 15 | ||||
-rw-r--r-- | tokio/src/util/slab.rs | 53 |
2 files changed, 65 insertions, 3 deletions
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2bcc47d..975ed5c4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -112,6 +112,21 @@ jobs: - name: miri run: cargo miri test --features rt,rt-multi-thread,sync task working-directory: tokio + san: + name: san + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions-rs/toolchain@v1 + with: + toolchain: ${{ env.nightly }} + override: true + - name: asan + run: cargo test --all-features --target x86_64-unknown-linux-gnu --lib -- --test-threads 1 + working-directory: tokio + env: + RUSTFLAGS: -Z sanitizer=address + ASAN_OPTIONS: detect_leaks=0 cross: name: cross diff --git a/tokio/src/util/slab.rs b/tokio/src/util/slab.rs index 0ab46adc..efc72e13 100644 --- a/tokio/src/util/slab.rs +++ b/tokio/src/util/slab.rs @@ -272,7 +272,7 @@ impl<T> Slab<T> { pub(crate) fn compact(&mut self) { // Iterate each page except the very first one. The very first page is // never freed. - for (idx, page) in (&self.pages[1..]).iter().enumerate() { + for (idx, page) in self.pages.iter().enumerate().skip(1) { if page.used.load(Relaxed) != 0 || !page.allocated.load(Relaxed) { // If the page has slots in use or the memory has not been // allocated then it cannot be compacted. @@ -302,6 +302,13 @@ impl<T> Slab<T> { // Drop the lock so we can drop the vector outside the lock below. drop(slots); + debug_assert!( + self.cached[idx].slots.is_null() || self.cached[idx].slots == vec.as_ptr(), + "cached = {:?}; actual = {:?}", + self.cached[idx].slots, + vec.as_ptr(), + ); + // Clear cache self.cached[idx].slots = ptr::null(); self.cached[idx].init = 0; @@ -488,8 +495,11 @@ impl<T> CachedPage<T> { /// Refresh the cache fn refresh(&mut self, page: &Page<T>) { let slots = page.slots.lock(); - self.slots = slots.slots.as_ptr(); - self.init = slots.slots.len(); + + if !slots.slots.is_empty() { + self.slots = slots.slots.as_ptr(); + self.init = slots.slots.len(); + } } // Get a value by index @@ -791,4 +801,41 @@ mod test { } } } + + #[test] + fn issue_3014() { + let mut slab = Slab::<Foo>::new(); + let alloc = slab.allocator(); + let mut entries = vec![]; + + for _ in 0..5 { + entries.clear(); + + // Allocate a few pages + 1 + for i in 0..(32 + 64 + 128 + 1) { + let (addr, val) = alloc.allocate().unwrap(); + val.id.store(i, SeqCst); + + entries.push((addr, val, i)); + } + + for (addr, val, i) in &entries { + assert_eq!(*i, val.id.load(SeqCst)); + assert_eq!(*i, slab.get(*addr).unwrap().id.load(SeqCst)); + } + + // Release the last entry + entries.pop(); + + // Compact + slab.compact(); + + // Check all the addresses + + for (addr, val, i) in &entries { + assert_eq!(*i, val.id.load(SeqCst)); + assert_eq!(*i, slab.get(*addr).unwrap().id.load(SeqCst)); + } + } + } } |