arc::Weak::upgrade does not account for overflow

[apasel422]

Similar to the issue fixed in #27174, these two methods do not account for overflow, causing use-after-frees in extremely contrived situations on machines with 32-bit usizes.

Both of these examples exhibit the erroneous behavior when compiled and run with -C opt-level=3 --target=i686-unknown-linux-gnu:

downgrade.rs:

use std::sync::Arc;

fn main() {
    let a = Arc::new("42".to_owned());

    for _ in 0..std::usize::MAX - 2 {
        let w = Arc::downgrade(&a); // add 1 to the weak count
        std::mem::forget(w);        // avoid decrementing the weak count
    }

    // the weak count is now `std::usize::MAX - 1`

    {
        let w = Arc::downgrade(&a); // add 1 to the weak count
        let x = w.clone();          // add 1 to the weak count so it wraps to 0
        drop(w);                    // drop `w`, deallocating the underlying `String`
        drop(x);                    // drop `x`, trying to deallocate the same `String`
    }
}

upgrade.rs:

use std::sync::Arc;

fn main() {
    let a = Arc::new("42".to_owned());
    let w = Arc::downgrade(&a);

    for _ in 0..std::usize::MAX - 1 {
        let b = w.upgrade(); // add 1 to the strong count
        std::mem::forget(b); // avoid decrementing the strong count
    }

    // the strong count is now `std::usize::MAX`

    {
        let b = w.upgrade().unwrap(); // add 1 to the strong count so it wraps to 0
        let c = b.clone();            // add 1 to the strong count so it is 1
        drop(c);                      // drop `c`, dropping the underlying `String`
    }

    println!("{:?}", a); // accesses the dropped `String`
}

Note that I've been working on a general consolidation of both reference-counted types at https://github.com/apasel422/ref_count.

[Gankra]

The fix to this is exactly the same as the rest. Add a check, abort the program.

[apasel422]

@gankro Yup, working on it now.

[apasel422]

Actually, because Arc::downgrade explicitly checks for usize::MAX, I'm not sure it needs to compare with MAX_REFCOUNT. The crash I was getting in the downgrade.rs example above was actually from calling abort in clone.

Does that seem right to you, @gankro?

[Gankra]

Probably? I'm pretty high on cough-syrup and stress right now.

I wouldn't expect any sound advice for the next two weeks.

[m-ou-se]

[..] not sure it needs to compare with MAX_REFCOUNT [..]

Turns out Arc::downgrade does need to compare with MAX_REFCOUNT: #108706 ^^