This is an archive of the discontinued LLVM Phabricator instance.

[Asan] Don't crash if metadata is not initialized
ClosedPublic

Authored by vitalybuka on Sep 1 2020, 5:11 AM.

Details

Reviewers
morehouse
Group Reviewers
Restricted Project
Commits
rGc05095cd6865: [Asan] Don't crash if metadata is not initialized
Summary

Fixes https://github.com/google/sanitizers/issues/1193.

AsanChunk can be uninitialized yet just after return from the secondary
allocator. If lsan starts scan just before metadata assignment it can
fail to find corresponding AsanChunk.

It should be safe to ignore this and let lsan to assume that
AsanChunk is in the beginning of the block. This block is from the
secondary allocator and created with mmap, so it should not contain
any pointers and will make lsan to miss some leaks.

Similar already happens for primary allocator. If it can't find real
AsanChunk it falls back and assume that block starts with AsanChunk.
Then if the block is already returned to allocator we have garbage in
AsanChunk and may scan dead memory hiding some leaks.
I'll fix this in D87135.

Diff Detail

Event Timeline

vitalybuka created this revision.Sep 1 2020, 5:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 5:11 AM
Herald added subscribers: Restricted Project, jfb. · View Herald Transcript
vitalybuka requested review of this revision.Sep 1 2020, 5:11 AM
vitalybuka edited the summary of this revision. (Show Details)Sep 1 2020, 5:20 AM
vitalybuka added a reviewer: Restricted Project.

simplify test

morehouse added inline comments.Sep 2 2020, 5:16 PM
compiler-rt/lib/asan/asan_allocator.cpp
757

The reason for falling back to alloc_beg is not obvious from the code.

Could you add a comment explaining why we do this, and why it's safe? (both here and below)

765

Since we're here, could you also document why falling back to alloc_beg is safe for the primary?

vitalybuka updated this revision to Diff 289850.Sep 3 2020, 8:56 PM

move nullptr handling closer to lsan

vitalybuka marked an inline comment as done.Sep 3 2020, 9:03 PM
vitalybuka added inline comments.
compiler-rt/lib/asan/asan_allocator.cpp
765

Looks like this is not very safe as well, lsan is lucky as LsanMetadata::allocated() will return false and the chunk will be ignored.
Other callers like AsanChunkView can handle nullptr, but will display wrong information in case of the current primary fallback.

morehouse accepted this revision.Sep 8 2020, 1:11 PM

Please update the commit message.

compiler-rt/lib/asan/asan_allocator.cpp
1121

Do we also need to make these changes in lsan_allocator.cpp?

1123

s/0/nullptr

This revision is now accepted and ready to land.Sep 8 2020, 1:11 PM
vitalybuka edited the summary of this revision. (Show Details)Sep 8 2020, 1:51 PM
vitalybuka updated this revision to Diff 290578.Sep 8 2020, 1:55 PM
vitalybuka marked an inline comment as done.
vitalybuka edited the summary of this revision. (Show Details)

0 -> nullptr

compiler-rt/lib/asan/asan_allocator.cpp
1121

Yes. LsanMetadata argument is results of GetUserBegin
pure lsan version of GetUserBegin never returns null
asan has to return null as we can get into situations when AsanChunk is not yet initialized.

This revision was landed with ongoing or failed builds.Sep 8 2020, 1:58 PM
This revision was automatically updated to reflect the committed changes.