Page MenuHomePhabricator

Fix bug where we request a shadow memory one page larger than necessary.
AcceptedPublic

Authored by delcypher on Aug 5 2020, 6:43 PM.

Details

Summary

The passed in space_size had left_padding added to it.
This is wrong because in the implementation FindAvailableMemoryRange
the computed size of the found region has the left_padding subtracted
(possibly more due to alignment) already.

Here's the relevant snippet from FindAvailableMemoryRange.

// We found a free region [free_begin..address-1].
uptr gap_start = RoundUpTo((uptr)free_begin + left_padding, alignment);
uptr gap_end = RoundDownTo((uptr)address, alignment);
uptr gap_size = gap_end > gap_start ? gap_end - gap_start : 0;
if (size < gap_size) {
  return gap_start;
}

In the size comparison (size < gap_size) gap_size doesn't
include padding so size shouldn't either.

rdar://problem/66603866

Diff Detail

Event Timeline

delcypher created this revision.Aug 5 2020, 6:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2020, 6:43 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
delcypher requested review of this revision.Aug 5 2020, 6:43 PM
yln added a subscriber: tejohnson.

Ahh, this is directly related to D85387 and already addresses one of my questions there!
Happy with this change, let's get @tejohnson's feedback.

Thanks for investigating this!

It looks reasonable, but adding @vitalybuka since this predates my recent refactoring, and he is probably a better person to review. It looks like the win code has the exact same issue, so should get a similar fix. The linux code doesn't seem to (it doesn't call FindAvailableMemoryRange, and doesn't have a similar comparison).

@yln Note there are other bugs here but I plan to fix this in future commits (e.g. if (size < gap_size) should be if (size <= gap_size)).

vitalybuka accepted this revision.Aug 18 2020, 8:19 PM
This revision is now accepted and ready to land.Aug 18 2020, 8:19 PM

This looks reasonable, but just for verification, could you post the "before" and "after" of the little shadow map printout from ASAN_OPTIONS=verbosity=1?

kubamracek accepted this revision.Aug 19 2020, 9:23 AM