Open trinity-1686a opened 5 years ago
Good spot! I agree, these should be marked unsafe.
I dont think it should be unsafe. These pointers are never dereferenced, only written to the jitted code. Calling the jitted code is already unsafe, as the input clif could do memory unsafe things already.
Agreed with @bjorn3 analysis: if you follow the use tree (they flow into SimpleJitBackend, then are used in lookup_symbol then get_definition) these pointers are embedded within the JIT code, but never written to.
That being said, since this is the responsibility of the user to provide a safe pointer here, we could just add an "unsafe" keyword to these two functions: this would let the embedder know they have to be careful here, and if they get a crash in generated code, they can grep for "unsafe" (in their code as well as Cranelift's) and find a reference to this function.
I agree; it's a good point that unsafe
isn't strictly necessary on these functions. However, passing raw function pointers into the JIT here can result in them being called, so these functions conceptually add new unsafety.
The unsafe on JIT code conceptually covers the fact that Cranelift IR contains instructions like load
and store
which can access arbitrary addresses, which is clearly unsafe. As an aside, I think it could be possible to create a subset of Cranelift IR which could be entirely safe (eg. prohibit load
and store
and require code use heap_load
and heap_store
instead), however that's a bigger topic.
Marking cranelift_simplejit::SimpleJITBuilder::symbol
as unsafe would conceptually cover the unsafety of calling the raw pointer being passed in -- that it's a valid pointer, pointing to a function with a matching signature, which is conceptually new unsafety not specifically covered by the generic unsafety of calling JIT code. So I think it makes sense to add unsafe to these functions.
IMO it should definitely be unsafe.
As an aside, I find it surprising that JIT'ing is safe but executing the JIT result is not, I would have expected the opposite.
(Disregard the second part I confused myself between CLIF and WASM I puts.)
symbol
is no more unsafe than any other define_function
and define_data
with Linkage::Import
. symbol
only adds an option to try when importing using define_*
before reaching out to dlsym
.
Is this issue still ongoing ? I don't see SimpleJITBuilder anywhere. I had assume it would have been in here https://github.com/bytecodealliance/wasmtime/tree/main/cranelift/jit
cranelift-simple-jit was renamed to cranelift-jit. JITBuilder::symbol
still exists, but I stand by it not being a safety issue: https://github.com/bytecodealliance/wasmtime/issues/1093#issuecomment-549900733
The function cranelift_simplejit::SimpleJITBuilder::symbol (as it's close related friend
symbols
, and maybe other funcions) take a*const u8
as parameter. From what I can see, there is no check whatsoever on the value provided before it gets used here. This function should probably either be marked as unsafe, or take something less permissive than a*const u8
(maybe a NewType whose builder is marked unsafe?). As of now it is possible to pass it a null pointer or a dangling pointer (droppedVec
, pointer to data from an old stack-frame...), and writing to any of those is definitely Undefined Behavior