This is an archive of the discontinued LLVM Phabricator instance.

[asan] Avoid redundant poisoning checks in __sanitizer_contiguous_container_find_bad_address
ClosedPublic

Authored by i.baravy on Nov 23 2016, 11:02 AM.

Details

Summary

__sanitizer_contiguous_container_find_bad_address computes three regions of a container to check for poisoning: begin, middle, end. The issue is that in current design the first region can be significantly larger than kMaxRangeToCheck.

Proposed patch fixes a typo to calculate the first region properly.

Diff Detail

Repository
rL LLVM

Event Timeline

i.baravy updated this revision to Diff 79111.Nov 23 2016, 11:02 AM
i.baravy retitled this revision from to [asan] Avoid duplicate and redundant poisoning checks in __sanitizer_contiguous_container_find_bad_address.
i.baravy updated this object.
i.baravy added reviewers: kcc, eugenis.
i.baravy set the repository for this revision to rL LLVM.
i.baravy added a project: Restricted Project.
i.baravy added subscribers: m.ostapenko, llvm-commits.
i.baravy updated this object.Nov 23 2016, 11:13 AM
i.baravy updated this object.

Could you please also update some test to cover new behavior?

lib/asan/asan_poisoning.cc
423

Could you please combine these
for (uptr i = r2_beg; i < mid; i++)
for (uptr i = mid; i < r2_end; i++)

into
for (uptr i = r2_beg; i < r2_end; i++)

i.baravy updated this revision to Diff 79131.Nov 23 2016, 11:30 AM

Merge cycles for adjacent regions.

eugenis added inline comments.Nov 23 2016, 2:05 PM
lib/asan/asan_poisoning.cc
423

But these loops check for opposite conditions. Now it is simply wrong: bytes after mid must be poisoned.

vitalybuka added inline comments.Nov 23 2016, 2:25 PM
lib/asan/asan_poisoning.cc
423

Oh, I didn't notice this, please undo this part.

m.ostapenko added inline comments.Nov 24 2016, 12:31 AM
lib/asan/asan_poisoning.cc
416

Why do you need all these changes (416, 417, 418)? If I read the code correctly, originally ASan checks bytes around beg, mid and end. What's incorrect here? Overlapping of memory ranges isn't a big deal here IMHO.

i.baravy updated this revision to Diff 79196.Nov 24 2016, 1:57 AM
i.baravy updated this object.
i.baravy marked an inline comment as done.

Revert merging cycles.

i.baravy marked 3 inline comments as done.Nov 24 2016, 2:00 AM

Thank you for the review, guys!

Vitaly,
could you suggest a test to update, please? I have no idea how to test this typo.

i.baravy retitled this revision from [asan] Avoid duplicate and redundant poisoning checks in __sanitizer_contiguous_container_find_bad_address to [asan] Avoid redundant poisoning checks in __sanitizer_contiguous_container_find_bad_address.Nov 24 2016, 2:02 AM
filcab added a subscriber: filcab.Nov 24 2016, 2:45 AM

Thank you for the review, guys!

Vitaly,
could you suggest a test to update, please? I have no idea how to test this typo.

I'm unsure you can easily test this since, due to the CHECK_LE(mid, end); (and no overflow), we know that end + kMaxRangeToCheck is bigger than mid, so we'll always get mid.
Maybe you can test it by having more than 64 bytes between beg and mid, and poisoning the 33rd byte. That way you know the check shouldn't be able to figure it out and you can check for lack of error (which is what should happen).
Of course, you'd need to add comments explaining you're testing that exact case, and any change to kMaxRangeToCheck would need a change in the test.

eugenis edited edge metadata.Nov 28 2016, 1:07 PM

Thank you for the review, guys!

Vitaly,
could you suggest a test to update, please? I have no idea how to test this typo.

I'm unsure you can easily test this since, due to the CHECK_LE(mid, end); (and no overflow), we know that end + kMaxRangeToCheck is bigger than mid, so we'll always get mid.
Maybe you can test it by having more than 64 bytes between beg and mid, and poisoning the 33rd byte. That way you know the check shouldn't be able to figure it out and you can check for lack of error (which is what should happen).
Of course, you'd need to add comments explaining you're testing that exact case, and any change to kMaxRangeToCheck would need a change in the test.

I don't think such test adds any value. I'm fine w/o any test changes.

lib/asan/asan_poisoning.cc
416

Well, with this change we get the same result with less work, so why not?

vitalybuka accepted this revision.Nov 28 2016, 4:13 PM
vitalybuka edited edge metadata.
This revision is now accepted and ready to land.Nov 28 2016, 4:13 PM
This revision was automatically updated to reflect the committed changes.