Page MenuHomePhabricator

[WebAssembly] Add validation to reloc section
ClosedPublic

Authored by ncw on Feb 23 2018, 8:40 AM.

Details

Summary

We now check relocations offsets are within range, and the relocation index is valid.

Also updated tests which contained invalid Wasm files that were previously not checked.


Could this be controversial?!? I know some people advocate for "minimal" correctness checks in LLVM, since the linker reads in hundreds of object files and we don't want to do expensive validation for each one.

These checks are pretty lightweight though, and should handily catch any bugs we might have.

Diff Detail

Event Timeline

ncw created this revision.Feb 23 2018, 8:40 AM
sbc100 added inline comments.Feb 27 2018, 12:32 PM
include/llvm/Object/Wasm.h
208

I find these method names a little confusing. Perhaps the code will be clearer just to inline them? (or perhaps drop the final Index from the name)?

lib/Object/WasmObjectFile.cpp
637

I think one variable per line makes this more readable (although I don't see anything in the llvm style it certainly seems to be the defacto style in the code I've been looking at).

Maybe call the first one LastRelocOffset?

696

Hmm, you can't do this at parse time because the symbol table doesn't exist yet?

In that case i think we need to specify that the reloc sections need to come after the linking metadata section, since they depend on the symbol table.

test/ObjectYAML/wasm/code_section.yaml
53

What is this adding the to the test? Presumably you might want to add new tests to check the failure cases?

test/ObjectYAML/wasm/data_section.yaml
35

Ditto

ncw planned changes to this revision.Feb 28 2018, 1:37 PM
ncw added inline comments.
include/llvm/Object/Wasm.h
208

Will do.

lib/Object/WasmObjectFile.cpp
696

Hmm, that only works if the reloc sections come together at the end of the file.

  • CODE must come before DATA
  • Symbol table must come after DATA (since otherwise the Data symbol indexes can't be validated as it's read in)
  • Relocs have to come at the end of the file therefore

I can make that change - it'll be a big tedious reordering of all the test output, but should be fairly straightforward.

This PR can go on hold then until it's that's done.

test/ObjectYAML/wasm/code_section.yaml
53

It's adding to the test since the test contains a file with relocations but no symbol table (so it's an invalid file).

You're right, I could add tests for exercising our handling of bad Wasm files... but we currently have very very few tests for bad Wasm files. What's the policy - is it our intention to have tests for all the various cases of illegal Wasm files?

I guess we would ideally, I'll add a test for the new error messages I've put in.

sbc100 added inline comments.Feb 28 2018, 2:05 PM
test/ObjectYAML/wasm/code_section.yaml
53

Oh I see. You mean that added these validations caused these tests to start failing? In that case yes, I think these test changes makes sense to be part of this CL. Perhaps mention that in the description.

ncw updated this revision to Diff 136539.Mar 1 2018, 8:56 AM
ncw edited the summary of this revision. (Show Details)

Rebased on top of D43940; it's now simpler if we force the reloc section to come after the symtab.

ncw marked 4 inline comments as done.Mar 1 2018, 8:56 AM
ncw updated this revision to Diff 136774.Mar 2 2018, 9:29 AM
ncw edited the summary of this revision. (Show Details)

Rebased

sbc100 accepted this revision.Mar 2 2018, 9:56 AM

(might want to clean up the description a little when committing).

This revision is now accepted and ready to land.Mar 2 2018, 9:56 AM
This revision was automatically updated to reflect the committed changes.