Page MenuHomePhabricator

[llvm-dwp] Fix a possible out of bound access.
ClosedPublic

Authored by ikudrin on Mar 21 2020, 4:42 AM.

Details

Summary

llvm-dwp did not check section identifiers read from input files. In the case of an unexpected identifier, the calculated index for Contributions[] pointed outside the array. This fix avoids the issue by skipping unsupported identifiers.

Diff Detail

Event Timeline

ikudrin created this revision.Mar 21 2020, 4:42 AM

Thanks!

Does the test need the debug_info section? (Probably needs them for some reason, might be worth mentioning)
Seems like the test should also have a tu_index to test the change there?

ikudrin updated this revision to Diff 253512.Mar 30 2020, 12:47 AM
  • The test was extended to add a TU index case.
  • Added a comment to the test that the additional sections are required to reach the test points in the code.
jhenderson added inline comments.Mar 30 2020, 1:13 AM
llvm/tools/llvm-dwp/llvm-dwp.cpp
220

You've got test cases for Kind < DW_SEC_INFO, but you probably also need cases for Kind > DW_SECT_MACRO.

250–252

I've not really looked around this code, so this might not make sense, but is it also worth checking that we don't emit data for these sections?

ikudrin updated this revision to Diff 253525.Mar 30 2020, 2:03 AM
ikudrin marked 3 inline comments as done.
  • Extended the test by adding an unknown ID which is greater than DW_SECT_MACRO.
llvm/tools/llvm-dwp/llvm-dwp.cpp
250–252

llvm-dwp does not emit anything for unknown sections and the patch is not changing that.

Thanks. Looks good from my point of view, but @dblaikie should confirm he's happy.

dblaikie added inline comments.Apr 2 2020, 4:36 PM
llvm/test/tools/llvm-dwp/X86/unknown-section-id.s
2–9

What does llvm-dwp do insteand of out of bounds access? Testing for "does anything other than crash" is usually a bad sign - presumably there's some specific behavior that we'd like llvm-dwp to have in these cases and should be testing for it here.

ikudrin marked 2 inline comments as done.Apr 2 2020, 10:18 PM
ikudrin added inline comments.
llvm/test/tools/llvm-dwp/X86/unknown-section-id.s
2–9

It just ignores the unknown sections because there is little it can do with them anyway.

jhenderson added inline comments.Apr 3 2020, 12:29 AM
llvm/test/tools/llvm-dwp/X86/unknown-section-id.s
2–9

Could we demonstrate that it does something else after the failure point and also that it doesn't try to dump the unknown sections?

ikudrin marked 2 inline comments as done.Apr 3 2020, 1:15 AM
ikudrin added inline comments.
llvm/test/tools/llvm-dwp/X86/unknown-section-id.s
2–9

Nothing comes to my mind. llvm-dwp creates the output, as usual. It ignores unknown sections, as usual. The only change is that it does not write to random memory locations, but that is not something that is easy to demonstrate; only UBSan catches that. Do you have any particular suggestions?

jhenderson added inline comments.Apr 3 2020, 2:59 AM
llvm/test/tools/llvm-dwp/X86/unknown-section-id.s
2–9

Perhaps at least show that the output contains only the expected sections maybe (not their contents)?

ikudrin updated this revision to Diff 254747.Apr 3 2020, 3:50 AM
ikudrin marked 2 inline comments as done.
  • Extended the test.
jhenderson accepted this revision.Apr 3 2020, 4:27 AM

Looks good, though probably should get someone else to confirm.

This revision is now accepted and ready to land.Apr 3 2020, 4:27 AM
dblaikie accepted this revision.Apr 5 2020, 5:43 PM

Looks good - thanks!

This revision was automatically updated to reflect the committed changes.