This is an archive of the discontinued LLVM Phabricator instance.

scudo: Limit the number of bytes tested in a realloc test.
ClosedPublic

Authored by pcc on Nov 26 2019, 7:23 PM.

Details

Summary

This test was previously effectively doing:
P = malloc(X); write X bytes to P; P = realloc(P, X - Y); P = realloc(P, X)
and expecting that all X bytes stored to P would still be identical after
the final realloc.

This happens to be true for the current scudo implementation of realloc,
but is not guaranteed to be true by the C standard ("Any bytes in the new
object beyond the size of the old object have indeterminate values.").
This implementation detail will change with the new memory tagging support,
which unconditionally zeros newly allocated granules when memory tagging
is enabled. Fix this by limiting the number of bytes that we test to the
minimum size that we realloc the allocation to.

Diff Detail

Event Timeline

pcc created this revision.Nov 26 2019, 7:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 26 2019, 7:23 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Build result: pass - 60330 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

cryptoad added a comment.EditedNov 27 2019, 7:46 AM

Hey,
I guess my test is just wrong.
Initially I wanted to cover the situation described at https://reviews.llvm.org/D67293, eg: reallocating slightly larger chunk keeps the same block to save on some dealloc/alloc combos.
But you are right in the sense of that there is no guarantee that the extra bytes are identical.
I still want the sizes to be increasing to cover that scenario, but I think what I should have done is keep a MinSize that tracks the smallest size so that we keep the minimal size all the way throughout the test.
Does that make sense to you?

pcc marked an inline comment as done.Nov 27 2019, 7:55 AM
pcc added inline comments.
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
124

We already have scudo::Min(DataSize, NewSize) here. The problem is that it increases from DataSize - 32 to DataSize during the test. Maybe we should replace this with DataSize - 32?

pcc marked an inline comment as done.Nov 27 2019, 8:00 AM
pcc added inline comments.
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
124

I see that you edited your message while I was writing my reply. Your new suggestion would also work for me, let me know which one you prefer.

cryptoad added inline comments.Nov 27 2019, 8:01 AM
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
124

Yeah, I apologize my initial response was also wrong.
Whichever you feel is better I am perfectly fine with either.

pcc updated this revision to Diff 231294.Nov 27 2019, 10:15 AM
pcc retitled this revision from scudo: Reverse order of testing sizes for realloc. to scudo: Limit the number of bytes tested in a realloc test..
pcc edited the summary of this revision. (Show Details)
pcc removed subscribers: tellenbach, merge_guards_bot.

Address review comment

cryptoad accepted this revision.Nov 27 2019, 10:20 AM
This revision is now accepted and ready to land.Nov 27 2019, 10:20 AM

Build result: pass - 60353 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

This revision was automatically updated to reflect the committed changes.