MapGuard dereferences to a dangling pointer

[Qwaz]

Hello fellow Rustacean,
we (Rust group @sslab-gatech) are scanning Rust code on crates.io for potential memory safety and soundness bugs and found an issue in this crate which allows safe Rust code to exhibit an undefined behavior.

Issue Description

arc-swap/src/access.rs

Lines 279 to 293 in b5ec44c

	impl<A, T, F, R> Access<R> for Map<A, T, F>
	where
	A: Access<T>,
	F: Fn(&T) -> &R,
	{
	type Guard = MapGuard<A::Guard, R>;
	fn load(&self) -> Self::Guard {
	let guard = self.access.load();
	let value: *const _ = (self.projection)(&guard);
	MapGuard {
	_guard: guard,
	value,
	}
	}
	}

arc-swap/src/access.rs

Lines 230 to 242 in b5ec44c

	impl<G, T> Deref for MapGuard<G, T> {
	type Target = T;
	fn deref(&self) -> &T {
	// Why this is safe:
	// * The pointer is originally converted from a reference. It's not null, it's aligned,
	// it's the right type, etc.
	// * The pointee couldn't have gone away ‒ the guard keeps the original reference alive, so
	// must the new one still be alive too. Moving the guard is fine, we assume the RefCnt is
	// Pin (because it's Arc or Rc or something like that ‒ when that one moves, the data it
	// points to stay at the same place).
	unsafe { &*self.value }
	}
	}

As noted in the comment, unsafe code in MapGuard expects the underlying guard type to dereferences to the same value even when the guard object is moved. However, Map uses Access as a trait bound which does not guarantee this property. As a result, Map accesses a dangling pointer when it is used with an Access implementation that does not dereferences to the same value when moved.

arc-swap/src/access.rs

Lines 295 to 322 in b5ec44c

	#[doc(hidden)]
	#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
	pub struct ConstantDeref<T>(T);
	impl<T> Deref for ConstantDeref<T> {
	type Target = T;
	fn deref(&self) -> &T {
	&self.0
	}
	}
	/// Access to an constant.
	///
	/// This wraps a constant value to provide [`Access`] to it. It is constant in the sense that,
	/// unlike [`ArcSwapAny`] and [`Map`], the loaded value will always stay the same (there's no
	/// remote `store`).
	///
	/// The purpose is mostly testing and plugging a parameter that works generically from code that
	/// doesn't need the updating functionality.
	#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
	pub struct Constant<T>(pub T);
	impl<T: Clone> Access<T> for Constant<T> {
	type Guard = ConstantDeref<T>;
	fn load(&self) -> Self::Guard {
	ConstantDeref(self.0.clone())
	}
	}

Constant seems to be the only type in this crate that implements Access in this way, but there can be other user types that implements Access on their own.

Reproduction

Below is an example program that segfaults, written only with safe APIs of arc-swap.

Show Detail

#![forbid(unsafe_code)]

use arc_swap::access::Map;
use arc_swap::access::{Access, Constant};

static CORRECT_ADDR: &str = "I'm pointing to the correct location!";

#[derive(Clone)]
struct MemoryChecker {
    // should be always CORRECT_ADDR
    message: &'static str,
}

impl MemoryChecker {
    pub fn new() -> Self {
        MemoryChecker {
            message: CORRECT_ADDR,
        }
    }

    pub fn validate(&self) {
        println!(
            "Pointing to {:?}, len {}",
            self.message as *const _,
            self.message.len()
        );
        println!("Message: {}", self.message);
    }
}

fn overwrite() {
    let a = 123;
    let b = 456;
    println!("Overwriting stack content {} {}", a, b);
}

fn main() {
    let constant = Constant(MemoryChecker::new());
    constant.0.validate();

    // Create a map with identity mapping
    let map = Map::new(constant, |checker: &MemoryChecker| checker);

    // After calling this, `value` pointer of `MapGuard` points to a dangling stack address
    let loaded = map.load();

    overwrite();

    loaded.validate();
}

Output:

Pointing to 0x55dcc9269000, len 37
Message: I'm pointing to the correct location!
Overwriting stack content 123 456
Pointing to 0x55dcc9267530, len 140726361573556

Terminated with signal 11 (SIGSEGV)

Tested Environment

• Crate: arc-swap
• Version: 1.0.0
• OS: Ubuntu 20.04.1 LTS
• Rustc version: rustc 1.48.0 (7eac88abb 2020-11-16)

[vorner]

Hello

It's a great discovery, though disturbing this has slipped through. I'll have to think how to plug this problem in a way that doesn't disrupt users too much, but I'll have a look at it today or tomorrow.

Thank you for finding it.

[vorner]

Fix released as 1.1.0 (by eliminating all unsafe code in that file).

It is technically a breaking change, but the chance of actually breaking code is low and the chance people will migrate from the broken version are higher this way, and even rustc makes an exception for soundness issues in the stability guarantees. It seems less bad to release as part of the 1. version.

[vorner]

Added a backport, released as 0.4.8, for all the reverse dependencies that didn't migrate to the 1. version yet.

[Qwaz]

Thank you for the quick fix!

[abergmann]

CVE-2020-35711 was assigned to this issue.