Page MenuHomePhabricator

[WebAssembly] Add validation to reloc section

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



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
208 ↗(On Diff #135648)

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)?

571 ↗(On Diff #135648)

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?

615 ↗(On Diff #135648)

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.

53 ↗(On Diff #135648)

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

35 ↗(On Diff #135648)


ncw planned changes to this revision.Feb 28 2018, 1:37 PM
ncw added inline comments.
208 ↗(On Diff #135648)

Will do.

615 ↗(On Diff #135648)

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.

53 ↗(On Diff #135648)

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
53 ↗(On Diff #135648)

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)


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.