Page MenuHomePhabricator

[ubsan] Fix IsAccessibleMemoryRange call for VtablePrefix to prevent SIGSEGV.

Authored by Dor1s on Jun 14 2017, 11:18 AM.

Diff Detail


Event Timeline

Dor1s created this revision.Jun 14 2017, 11:18 AM
filcab added a subscriber: filcab.Jun 14 2017, 11:20 AM

Test case?

Test case?

I would say this is a follow-up for, but let me see if I can build another testcase...

vsk edited edge metadata.Jun 14 2017, 11:36 AM

Lgtm with some more testing. You could modify PR33221.cpp. Something like this?

char *c = new char[16 + sizeof(Derived)];
memset((void *)c, 0, 16 + sizeof(Derived));
Derived *list = (Derived *)(c + 16);
int foo = list->i;
Dor1s updated this revision to Diff 102610.Jun 14 2017, 3:09 PM

It revealed to be a bit more complicated, so there are two tests now.

  1. I fixed PR33221.cpp in a way that it fails with old revisions (i.e. before, but works for any other revision since then. Otherwise, the test didn't make sense, as UBSan has been reporting an error even with older revisions due to the check of Vptr against NULL value.
  1. I added another test for this particular CL. The idea of the test is to reproduce a case when VptrPrefix is pointing to a non accessible memory, while Vptr points to an accessible one. I haven't found a better way rather then allocate two sequential memory pages, so this extra test is Linux only.
Dor1s updated this revision to Diff 102611.Jun 14 2017, 3:10 PM

Remove a typo.

Dor1s updated this revision to Diff 102612.Jun 14 2017, 3:13 PM

Fix formatting

vsk added a comment.Jun 14 2017, 3:16 PM

Lgtm with minor changes.

24 ↗(On Diff #102611)

0 -> nullptr

25 ↗(On Diff #102611)

Could you bail out with 'return 0' when the mmap fails?

29 ↗(On Diff #102611)

Linux doesn't guarantee that 'accessible' will be at non_accessible + page_size, so bail out if it doesn't? Also bail out if the mmap fails.

Dor1s updated this revision to Diff 102613.Jun 14 2017, 3:25 PM
Dor1s marked 3 inline comments as done.

Address comments

29 ↗(On Diff #102611)

With MAP_FIXED it should fail if the specified address cannot be used, so bail out in case of a failure seems to be enough. Thanks for the review!

vsk added inline comments.Jun 14 2017, 3:56 PM
2 ↗(On Diff #102613)

One last thing. FileCheck will fail if either of the mmap's do. Could you pipe "%t &> %t.log", then in a separate RUN line, run FileCheck if the log is non-empty? Something like "cat %t.log | not count 0 && FileCheck ...".

Dor1s updated this revision to Diff 102685.Jun 15 2017, 10:22 AM
Dor1s marked an inline comment as done.

Now the test also works if any of the mmap calls failed.

2 ↗(On Diff #102613)

Oh, right! Huge thanks for the hint on how to handle that!

vsk accepted this revision.Jun 15 2017, 11:11 AM

LGTM, thanks for fixing this. Aside: FileCheck's has an "-input-file" option which can be useful in situations like this.

This revision is now accepted and ready to land.Jun 15 2017, 11:11 AM
Dor1s updated this revision to Diff 102693.Jun 15 2017, 11:15 AM

Good to know, it's useful indeed! May I ask you please to land this change as I'm not a committer yet? I'd greatly appreciate if you have a couple of minutes to do that :)

This revision was automatically updated to reflect the committed changes.
eugenis added a subscriber: eugenis.Oct 9 2017, 3:34 PM
eugenis added inline comments.
29 ↗(On Diff #102611)

If the memory region specified by addr and len overlaps pages of any existing mapping(s), then the overlapped part of the existing mapping(s) will be discarded.

I've been unlucky enough for this to overlap AsanThread for the main thread, and has been staring at null pointers for a while.

eugenis added inline comments.Oct 9 2017, 3:52 PM
29 ↗(On Diff #102611)

Fixed in r315247.

Dor1s added inline comments.Oct 10 2017, 8:02 AM
29 ↗(On Diff #102611)

Wow! Thanks for the info and for the fix!