wasm2c: Use wrappers for function references #2465
Conversation
|
cc @shravanrn (can you take a look if this doesn't break anything with the segue stuff?) @keithw (can you take a look like, in general? tell us if this seems like the wrong approach...) @sbc100 |
|
Perhaps we can use some kind of annotation to tell UBSAN to ignore these call_indirect mismatches? |
|
This appears to be relevant reading material: python/cpython#111178 If we are understanding this right, this kind of thing is important for "control-flow integrity", a hardware feature(?) to prevent exploits. It would seem ill-advised to ignore it in wasm2c. |
|
Is it feasible to instead change the CALL_INDIRECT macro to generate the correct type signature (instead of making everything void*)? |
|
No, tables can change at runtime, even with dynamic linking (think dlopen()ing wasm2c'd modules). We don't know the type of the module instance, and we can't know all of the possible module instance types. It's either this, or wrapper functions. The issue isn't the argument types - CALL_INDIRECT already casts those correctly. It's the module instance. |
This should have no interaction with segue. Also the CI already includes a segue test, so it should (hopefully) be enough if we pass CI
So I assume you are referring to clang's software-enforced (and in the future possibly partially-hardware-backed) control-flow integrity (CFI). It's actually entirely fine to ignore clang CFI in wasm2c, as Wasm in effect has its own CFI built-in (the table's function type checks in Wasm are basically the same as Clang's CFI). Arguably, clang's CFI is slightly more fine-grained, but imho not sufficiently beneficial to make a significant change over. To be clear this is different from whether we should worry about UB or not. In general, we should really try to avoid UB for reasons that have nothing to do with CFI support. On the current PR : As a general rule, I am not a huge fan of regressing any strong typing guarantees we have in the code base. This is one of the few things preventing us from accidentally adding bugs as we add features. If anything, I think we should move to more strong types and less voids (or general types) where possible. Given all this, I think the right approach for this PR is probably one of the other options suggested earlier
|
this is proving to be more difficult than expected, and we didn't expect it to be easy... and we noticed tail calls don't suffer from this issue. do they use wrapper funcs? no, they just use |
f28b602 to
7665873
Compare
|
rebased and rewritten |
|
@sbc100 ping? |
5e7521b to
1dd9efe
Compare
| sanitize: | ||
| name: sanitize | ||
| runs-on: ubuntu-latest | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
pulls in newer clang. (17)
note that we're only doing this for the sanitizers.
There was a problem hiding this comment.
see also the relevant CI run: https://github.com/WebAssembly/wabt/actions/runs/10896075346/job/30235292696
There was a problem hiding this comment.
I see. SGTM.. I wonder wonder why ubuntu-latest is not already 24.04?
There was a problem hiding this comment.
🤷 github takes a while to roll out these things. we should set it back when ubuntu-latest becomes 24.04.
Clang 17(?) tightened UBSAN checks, so that you now get this:
This happens because emitted functions use a typed module instance, while function references use a
void*instance. It is UB in C to call the former with the latter, so clang is correct here.We had to pick one of two ways to fix this: either emit
void*wrapper functions that do the appropriate downcasting for any module functions that go into a table (potentially including imported functions), or the approach that takes significantly less effort of changing everything tovoid*and downcasting internally.We obviously chose the latter.We eventually started emitting wrapper functions.