This is an archive of the discontinued LLVM Phabricator instance.

-fsanitize=vptr warnings on bad static types in dynamic_cast and typeid
ClosedPublic

Authored by sberg on Nov 21 2017, 4:38 AM.

Details

Summary

...when such an operation is done on an object during con-/destruction.

This adds a test case to compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp that, unlike the existing test cases there, wants to detect multiple UBSan warnings in one go. Therefore, that file had to be changed from globally using -fno-sanitize-recover to individually using halt_on_error only where appropriate.

Diff Detail

Repository
rL LLVM

Event Timeline

sberg created this revision.Nov 21 2017, 4:38 AM
sberg added a comment.Nov 21 2017, 4:42 AM

Some items I'm unclear about:

  • Should those additional calls to EmitTypeCheck be restricted via those SkippedChecks or not?
  • Maybe there's a better way to add tests for this than to try shoehorn it into the existing vptr.cpp?
sberg updated this revision to Diff 124154.Nov 24 2017, 3:22 AM

Oh, I'd meant to upload the version of the patch where the newly added calls to EmitTypeCheck use the default Alignment and SkippedChecks arguments, instead of explicitly skipping Alignment and ObjectSize checks. (That's the version I had successfully used in a LibreOffice UBSan 'make check' test build. But I still don't know which version is better.)

sberg added a comment.Dec 13 2017, 3:41 AM

friendly ping; any input on my two questions maybe?

vsk added a subscriber: vsk.Dec 19 2017, 12:03 PM

I don't think any checks can be skipped in the newly-introduced calls to EmitTypeCheck. Clang uses EmitDynamicCast on arbitrary addresses, not just addresses which are known to be checked for alignment/etc. Regarding the test update, I think it makes sense to extend the runtime test in vptr.cpp, but that we'd also benefit from a small/narrow IR test (e.g in test/CodeGenCXX/ubsan-vtable-checks.cpp). With the added test I think this patch would be in great shape.

sberg updated this revision to Diff 127676.Dec 20 2017, 3:01 AM

added a small IR test

vsk accepted this revision.Dec 20 2017, 11:39 AM
vsk added reviewers: vsk, thakis.

Thanks, lgtm. Could you wait a day or two before committing? IIRC Richard or Nico have a -fsanitize=vptr Chromium bot, and they may have further comments.

This revision is now accepted and ready to land.Dec 20 2017, 11:39 AM
This revision was automatically updated to reflect the committed changes.