Page MenuHomePhabricator

[WebAssembly] Don't generate invalid modules when function signatures mismatch
ClosedPublic

Authored by sbc100 on Feb 7 2019, 10:15 AM.

Details

Summary

Previously we could emit a warning and generate a potentially invalid
wasm module (due to call sites and functions having conflicting
signatures). Now, rather than create invalid binaries we handle such
cases by creating stub functions containing unreachable, effectively
turning these into runtime errors rather than validation failures.

Fixes PR40470

Event Timeline

sbc100 created this revision.Feb 7 2019, 10:15 AM
sbc100 updated this revision to Diff 185823.Feb 7 2019, 11:00 AM
  • add docs
sbc100 updated this revision to Diff 185828.Feb 7 2019, 11:06 AM
  • rebase
sbc100 retitled this revision from [WebAssembly] Handle mismatched signatures more gracefully to [WebAssembly] Don't generate invalid modules when function signatures mismatch.Feb 7 2019, 11:12 AM
sbc100 updated this revision to Diff 185837.Feb 7 2019, 11:37 AM
  • comments
sbc100 updated this revision to Diff 185844.Feb 7 2019, 12:03 PM
  • rebase
sbc100 added a reviewer: ruiu.Feb 7 2019, 2:51 PM
sbc100 updated this revision to Diff 185873.Feb 7 2019, 2:53 PM
  • fix after upstream llvm change
ruiu added inline comments.Feb 7 2019, 2:56 PM
docs/WebAssembly.rst
98 ↗(On Diff #185872)

Is this a new default behavior? I wonder what is the motivation of the change. I mean, finding an error at static-link-time is almost always better than finding it at run-time, no?

sbc100 marked an inline comment as done.Feb 7 2019, 3:05 PM
sbc100 added inline comments.
docs/WebAssembly.rst
98 ↗(On Diff #185872)

It complicated.

Before this change, we generate a warning and an invalid module.

I've tried in the past to make this a hard error because generating an invalid module seems like always the wrong thing to do.

When I do this lots of things break. The most common failure is CMake checks for whether a function exists. Cmake create a tiny C program with the named symbol and checks that it links. But it doesn't know the signature so I always uses "char <func>()". This is almost always wrong but the expectation is that linking should work.

In you actually tried to execute that at runtime you'd get undefined behaviour, but its not trying to run it.

So, the conclusion was to make this the default behaviour and allow the link to succeed, and have an option for a more strict mode.

We could invert the option... but that wouldn't save us any complexity and I quite the like the default to match the native linker behaviour.

ruiu added a comment.Feb 7 2019, 3:18 PM

Ah, thanks for the explanation. I think that makes sense to me.

If you create a module with a invalid function type, what would happen at runtime? Does it fail to load?

sbc100 added a comment.Feb 7 2019, 3:37 PM

Ah, thanks for the explanation. I think that makes sense to me.

If you create a module with a invalid function type, what would happen at runtime? Does it fail to load?

Yes, the browser would fail to validate the module.

Now it will validate, and even run, as long as you don't hit that invalid call sites.

ruiu added a comment.Feb 7 2019, 3:49 PM

How can you fix the CMake issue you mentioned by this change? If CMake checks only if a source file compiles, then this patch doesn't change the behavior (previously we created a broken module but it at least links). If CMake actually tries to run a function, then previously it fails at load-time, and with this patch it would fail when a program attempt to execute a function. So, looks like the behavior observed by CMake doesn't change that much. What am I missing?

sbc100 added a comment.Feb 7 2019, 3:57 PM

How can you fix the CMake issue you mentioned by this change? If CMake checks only if a source file compiles, then this patch doesn't change the behavior (previously we created a broken module but it at least links). If CMake actually tries to run a function, then previously it fails at load-time, and with this patch it would fail when a program attempt to execute a function. So, looks like the behavior observed by CMake doesn't change that much. What am I missing?

You are right, I'm sorry, there is a little more to the story: The emscripten toolchain driver "emcc" runs a post-link tool after lld which will fail (quite rightly) when given an invalid module.

I think its pretty safe to say we should never generate invalid modules anyway, so even without this issue I would say the current state is broken.

sbc100 added a comment.Feb 7 2019, 4:00 PM

Also, I should have mentioned that we can improve on this change by allowing for more flexible stub functions.

For example, if one translation unit declares a function as returning void but the function as defined returns, say, int32, we can have a stub function that simply drops the return value. We have pass in LLVM that already does this for uses within the same bitcode file https://github.com/llvm-mirror/llvm/blob/master/lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp.

ruiu added inline comments.Feb 7 2019, 4:21 PM
docs/WebAssembly.rst
101–106 ↗(On Diff #185873)

Could you update this comment a bit to reflect the discussion we've had so far? I think that it should mention the following things:

  • The runtime verifier rejects modules if it finds a function type mismatch
  • So there's no point of creating a module by ignoring type mismatch errors
  • But sometimes link failure as a result of a type mismatch is too strict; you want to create an executable even if it contains a few errors.
  • So lld/wasm supports two modes: one is to report an error for type mismatch and exit. The other is to create a stub function for an erroneous function, so that the generated file at least passes the verifier, which effectively turns a type error into a runtime error. The latter is the default mode.
wasm/Options.td
94 ↗(On Diff #185873)

Remove a space before :. Indent with two spaces.

wasm/SymbolTable.cpp
311 ↗(On Diff #185873)

created -> Created

410–411 ↗(On Diff #185873)

else if should be on the same line.

571–572 ↗(On Diff #185873)

It looks like you are trying to reduce the number of stub functions, by generating only one stub for each function type. But is this necessary? Given that no production code will contain a stub, no one cares if a binary with type mismatch contains many duplicated stubs?

sbc100 updated this revision to Diff 185894.Feb 7 2019, 4:53 PM
sbc100 marked 5 inline comments as done.
  • feedback + clang-format
wasm/Options.td
94 ↗(On Diff #185873)

Ha.. I hit my clang format button to get this :)

wasm/SymbolTable.cpp
571–572 ↗(On Diff #185873)

Yes, I create one stub for each variant of each symbol by encoding the signature into a string and appending it to the symbol name.

Are you suggesting that createFunctionVariant not even use the symbol table at all and just unconditionally create a new Symbol outside the symbol table? I guess it might work, but I'm not sure how much simpler it would make the code.

Finally I'm not sure this won't end up happening somewhat in production code, especially if we implement the more useful stubs that coerce rather than trap.

ruiu added inline comments.Feb 7 2019, 5:05 PM
wasm/SymbolTable.cpp
571–572 ↗(On Diff #185873)

I don't have a clear idea what I was suggesting, but feels like there's room to simplify the way how we creates stub functions. Currently the code for creating stub functions somewhat scatters over many places in this file.

I wonder if you can create a vector to which we store all (undefined?) symbols that we find type mismatch, and scan that vector at some point to create stubs?

sbc100 updated this revision to Diff 186332.Feb 11 2019, 2:12 PM
  • move to monorepo
sbc100 updated this revision to Diff 186339.Feb 11 2019, 2:29 PM
  • group functions
sbc100 updated this revision to Diff 186349.Feb 11 2019, 3:14 PM
  • more testing
sbc100 marked an inline comment as done.Feb 11 2019, 3:15 PM
sbc100 added inline comments.
wasm/SymbolTable.cpp
571–572 ↗(On Diff #185873)

My intent here is pretty much as you describe. I have a side data structure SymVariants which contains all the symbols have have mismatching symbols. This structure is empty for normal programs and only becomes populated once we have our first mismatch.

Admittedly this has to be handled in two places (for both defined and undefined functions) and we don't know which one will come first.

I'm refactored to try to keep the new code together and added more test cases.

ruiu added inline comments.Feb 11 2019, 4:00 PM
lld/wasm/SymbolTable.cpp
130

Could you add a short comment here saying that if one symbol is a function and the other is not a function, it is always a function type error?

130–134

It looks like this function does two different things:

  1. verifying that both symbols are function symbols
  2. verifying that the two functions have the same type signature

a boolean return value doesn't make much sense for (1).

I'd remove this part of code from this function and move that error check to the caller.

lld/wasm/Symbols.h
76

At this point we probably should simply make Name a public member, as it has both set and get accessors.

sbc100 updated this revision to Diff 186568.Feb 12 2019, 5:13 PM
sbc100 marked 2 inline comments as done.
  • feedback
ruiu added inline comments.Feb 12 2019, 7:49 PM
lld/wasm/Options.td
94

A little bikeshedish, but --signature-check=strict or --strict-signature-check feel natural to me, but --signature-check-strict seems a bit odd as a command line option name.

sbc100 edited the summary of this revision. (Show Details)Feb 15 2019, 1:02 PM
sbc100 updated this revision to Diff 187283.Feb 18 2019, 4:14 PM
  • Remove variant symbols from the symbol table.
ruiu added inline comments.Feb 19 2019, 11:31 AM
lld/wasm/Options.td
94

Sorry for being nitpicky, but since command line names cannot change once set, so bear with me...

There's no linker command line option that ends with -checking. Perhaps a most similar option is -warn-mismatch and -no-warn-mismatch, which is in the form of -<verb>-... and -no-<verb>-. Maybe we should follow that convention? I.e. -check-signature?

Does anyone have a suggestion?

sbc100 updated this revision to Diff 187428.Feb 19 2019, 12:37 PM
  • remove setName
sbc100 updated this revision to Diff 187680.Feb 20 2019, 2:34 PM
sbc100 marked an inline comment as done.
  • remove new flag. Just go with --fatal-warnings for now
ruiu accepted this revision.Feb 20 2019, 2:38 PM

LGTM

This revision is now accepted and ready to land.Feb 20 2019, 2:38 PM
This revision was automatically updated to reflect the committed changes.