cranelift-jit: should functions that turn raw pointers into symbols be unsafe?

[trinity-1686a]

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 (dropped Vec, pointer to data from an old stack-frame...), and writing to any of those is definitely Undefined Behavior

[sunfishcode]

Good spot! I agree, these should be marked unsafe.

[bjorn3]

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.

[bnjbvr]

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.

[sunfishcode]

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.

[nox]

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.

[nox]

(Disregard the second part I confused myself between CLIF and WASM I puts.)

[bjorn3]

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.

[AdaBehan]

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

[bjorn3]

cranelift-simple-jit was renamed to cranelift-jit. JITBuilder::symbol still exists, but I stand by it not being a safety issue: #1093 (comment)