Closed mrhota closed 7 years ago
@dikaiosune I removed the unsafe annotations. They receive simple integers as arguments and don't do anything crazy (or interesting, really).
I added comments explaining some reasoning and passing off to musl commit messages for further discussion.
Cool. Looks good.
The functions don't do anything crazy, but we do mark their linkage as weak, and expect the linker to place its own definitions in for the dummies, right? I'm not sure what invariants we would expect a caller to enforce by requiring the calls be placed in an unsafe block, but it seems like it's maybe most appropriate to leave them as unsafe? I'll defer to your judgement.
@dikaiosune I got inspired to do that because was re-reading https://huonw.github.io/blog/2014/07/what-does-rusts-unsafe-mean/ for some insight into marking fn
s unsafe
vs using unsafe
blocks internally.
I wouldn't say we expect the linker to provide strong symbols for things like _fini
or __funcs_on_exit
, but we are prepared for it by marking the symbols weak. If we expected the symbols, we'd probably just have some extern
decls instead of definitions:
extern "C" {
fn _fini();
fn __funcs_on_exit();
}
If it's up to me, I prefer fewer unsafe
s!
Cool. Thinking about it more I think we kind of just have to trust that the linker will do what it says, and that there's nothing our userspace program can do to prevent it behaving poorly.
I prefer fewer
unsafe
s!
Heh, I actually have the opposite mindset. I would rather be overcautious about what should be declared unsafe lest we accidentally create a "safe" call chain that breaks things. I think in this case it's immaterial but an interesting philosophical question nonetheless.
One example I see is from this audit report I read this morning:
https://www.x41-dsec.de/reports/Kudelski-X41-Wire-Report-phase1-20170208.pdf
(pg 12, section 3.3) I think that this init function probably should have returned a Result<(), SomeError>
, but if they were trying to match a C API they alternatively could have marked it as an unsafe function, signaling to a caller that if they didn't handle its failure correctly that there could be memory unsafety as a result. Ideally they would have done both: used a Result to make use of the must_use
compiler lint but also mark the init function as unsafe so that the caller has to be aware of potential memory safety consequences of not handling its failure properly.
Anyways, it's late and I'm waiting for several builds so this is just a ramble.
some of the mess in exit.rs is meant for dynlink. Read commit messages for exit.c and friends for full details and justifications.