Multiple mutable references and memory bugs

[gavento]

Bronze GC allows creating simultaneous mutable references to a single object - probably by design. However, this allows users to create memory bugs in safe rust.

The example below writes into free'd memory in safe rust and does so in a very deliberate way, but I would think that allowing multiple mutable references may create undefined behavior (UB, which may cause memory bugs in surprising ways, or allow the compiler some nasty undesired mis-optimizations) in subtle and unexpected ways even with "reasonable" use of Bronze (e.g. there may be no consistent set of simple rules to make usage of Bronze actually safe, even if one wanted to take on this cost of moving away from reliably safe rust; e.g. because other libraries may make no-mutable-aliasing assumptions).

(Subjectively, it seems there is additional cost of this when Bronze is used to teach rust and proper memory handling. But that is independent of this.)

extern crate bronze_gc;
use bronze_gc::GcRef;

fn main() {
    let mut gr1 = GcRef::new(vec![1u16,2,3]);
    let mut gr2 = gr1.clone();

    let ref1: &mut Vec<u16> = gr1.as_mut();
    let ref2: &mut Vec<u16> = gr2.as_mut();
    // ref1 and ref2 now reference the same object:
    ref1.push(4);
    ref2.push(5);
    ref1.push(6);
    
    dbg!(&ref1, &ref2);

    // Evil stuff:
    let ref1elem0 = ref1.get_mut(0).unwrap();
    // Force reallocation of the underlying vec
    ref2.resize(1024, 0);
    // Now this writes to deallocated memory
    *ref1elem0 = 42;
    dbg!(&ref1elem0, ref2.get(0).unwrap()); // prints: 42, 1
}

Running valgrind target/debug/bronze_test:

[...]
==56780== Invalid write of size 2
==56780==    at 0x115A4A: bronze_test::main (main.rs:24)
[...]
==56780==  Address 0x4bfbf20 is 0 bytes inside a block of size 12 free'd
==56780==    at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
[...]
==56780==    by 0x115A41: bronze_test::main (main.rs:22)
==56780==  Block was alloc'd at
==56780==    at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
[...]
==56780==    by 0x1155B1: bronze_test::main (main.rs:13)
[...]

Edit: There is a discussion on rust forum.

[mcoblenz]

Thanks for this report. I posted on the forum, but I'll paste my reply here too:

The Bronze project is exploring the usability costs of the restrictions that Rust imposes. Indeed, if one uses as_mut() and as_ref() inappropriately, one can do unsafe things. As the project proceeds, I think it will be worth considering (1) dynamic safety enforcement, via a mechanism similar to the Ref/RefMut one used by RefCell; (2) a mechanism to avoid exposing references into the interior of objects owned by the garbage collector.

[gavento]

Thanks for the clarification of the goals of Bronze.

What I see as one potentially serious issue (e.g. within the paper) is that Bronze seems to ask the question "How much is Rust easier to use with a Rust-compatible/sound GC?" while it actually asks the question "How much is Rust easier to use with a unsound/over-permissive GC" (which I think is very hard or impossible to reliably use safely in wider Rust).

My steelman claim of the study now shows how would Rust be easier to use (in the sense of the study) if Bronze GcRef had an implicit RefCell wrapping the inner object -- that would IMO be safe and, while costing some runtime performance, would allow the students to do very similar things. (Some may run into double-borrow runtime errors, but the majority may not even see those in a naive implementation of the tasks.)

---

Regarding the ongoing discussion: Perhaps stating the purpose, experimental state and/or caveats in the github repo etc. may alleviate some of the misunderstanding and strong reactions. Some of the concerns may be due to how now, Bronze may be perceived as "a fully functional Rust GC, with design even backed by a paper", which would not be correct (e.g. to me it now seems very much experimental and exploratory with some unsound design bits; which is an OK state for a project to be in).

[gavento]

Note that one possible unsoundness fix would be to remove GcRef::as_mut() and provide an alias type GcRefCell<T> = GcRef<rc::GcCell<T>>, and let the students use that (i.e. always paying the runtime cost of the borrow check).

You could also add convenience methods e.g. GcRefCell<T>::as_ref(&self) -> &T and GcRefCell<T>::as_mut(&self) -> &mut T that would make the usage simpler.

While this would perhaps make Bronze safe and sound (as much as rust-gc is?), I am not sure how would it be different from RustGc.

[mcoblenz]

Updating the readme is a great idea, thanks; I did so. I guess I was sort of expecting that the lack of any kind of meaningful instructions in the README on the main branch would discourage people from thinking of this as something they should use right away, but perhaps that's not the case.

[mcoblenz]

I'll copy my forum post reply here for convenience:

Indeed, this is language design research. But I view this as an early-stage step toward compatibility with Rust, not with the goal of developing a fundamentally different end language. But as you say, there is still substantial work to be done before something like this could be considered for adoption in Rust.

Some people have said things like: "there's no need for GC in Rust, because you can already do everything you need to do." What we see in this work is that there is a significant usability cost to that position. Can we build a GC that meets all of Rust's design requirements? That's still a work in progress.

[nyanpasu64]

GcRefCell<T>::as_mut(&self) -> &mut T

It's a nice API, but RefCell has to return RefMut (a harder to use API) because otherwise it won't be informed when the borrow ends. (And GC finalization can't be used to decrement the RefCell because the GC may not realize a previous borrow has been dropped when a new one is created.) Correct me if I'm mistaken though.

My current favorite approach to shared mutability I want to explore is wrapping most fields in Cell, then altering the compiler (or using a proc macro) to allow implicitly converting between Cell<T> and T. However I haven't actually tested it, and without a compiler macro the Cell::get/set() is painful.