rune icon indicating copy to clipboard operation
rune copied to clipboard

Arena Singleton

Open CeleritasCelery opened this issue 2 years ago • 2 comments

The soundness of the code heavily relies on the fact that only one Arena exists in a thread at a time. Otherwise you might have code that borrows from an Arena that it does not own. And could then get collected and have use after free.

let arena1 = Arena::new();
let arena2 = Arena::new();
let foo = arena1.add("foo"); # arena1 owns foo
rebind!(foo, arena2); # foo now borrows from arena2
arena1.garbage_collect(); # foo will be freed but still accessible

To guarantee this we have singleton check that makes sure two can't be created at the same time.

https://github.com/CeleritasCelery/rune/blob/7136b74386a4586537ffa59c3be4849cb76354fe/src/arena/mod.rs#L211-L239

Why this is sound

We can only have one global Block (part of Arena) and one thread local one. The global version is already created in our global symbol intern struct so any attempt to create another would panic. This global Block is not exposed to user unless they are editing the symbol intern struct.

If we attempt to create two thread local arena's it will panic, which is safe. However if these checks could ever be circumvented it would be unsound. But I think that is impossible.

CeleritasCelery avatar Apr 11 '22 15:04 CeleritasCelery

I don't think the Relaxed ordering is correct here. The check could be reordered to after the construction or even later after the arena is used, causing two arenas to exist simultaneously. I would go for an AcqRel, Relaxed combination here, to prevent reordering and establish a clear happens-before relationship between the tjreads. But I'm not very good with atomics, so take this with a grain of salt.

Noratrieb avatar Apr 22 '22 15:04 Noratrieb

I can see your point, but even if the checks get reordered such that two exist at the same time, the check will eventually happen and cause the program to panic. But the only reason to use a looser ordering over a stricter one is performance. And since this check should only happen once in a correct program, we should use the stricter one as you suggested. I will fix that. Thanks!

CeleritasCelery avatar Apr 24 '22 19:04 CeleritasCelery