This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Fix a crash in SizeClassAllocator32 with an out-of-range pointer
ClosedPublic

Authored by kubamracek on Nov 26 2015, 1:52 AM.

Details

Summary

This only happens on a 64-bit platform that uses SizeClassAllocator32 (e.g. ASan on AArch64). When querying a large invalid pointer about its size, e.g. with:

__sanitizer_get_allocated_size(0xdeadbeefdeadbeef);

...an assertion will fail:

AddressSanitizer CHECK failed: .../sanitizer_allocator.h "((res)) < ((kNumPossibleRegions))"

This patch changes PointerIsMine to return false if the pointer is outside of [kSpaceBeg, kSpaceBeg + kSpaceSize).

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 41217.Nov 26 2015, 1:52 AM
kubamracek retitled this revision from to [sanitizer] Fix a crash in SizeClassAllocator32 with an out-of-range pointer.
kubamracek updated this object.
kubamracek added reviewers: samsonov, glider, kcc, dvyukov.
kubamracek added subscribers: llvm-commits, zaks.anna.
dvyukov added inline comments.Nov 30 2015, 6:14 AM
lib/sanitizer_common/sanitizer_allocator.h
753

The condition in GetSizeClass->ComputeRegionId is different. It effectively checks that mem < kSpaceSize.
At least ComputeRegionId assumes that kSpaceBeg==0. We seem to be missing some tests.
Kostya?

kubamracek added inline comments.Dec 7 2015, 3:28 AM
lib/sanitizer_common/sanitizer_allocator.h
753

How should this be fixed? All uses of SizeClassAllocator32 currently use "0" as kSpaceBeg, so we could just assume that as a requirement.

dvyukov edited edge metadata.Dec 7 2015, 5:39 AM

I will ask Kostya.

kcc edited edge metadata.Dec 8 2015, 12:40 AM

Where is "__sanitizer_get_allocated_size(0xdeadbeefdeadbeef);" called?
I'd say this is a misuse of the function, you should not pass a pointer outside of the AS there.

Kostya, I think we should remove kSpaceBeg and replace it with zero. What do you think?

kcc added a comment.Dec 8 2015, 3:11 AM

Kostya, I think we should remove kSpaceBeg and replace it with zero. What do you think?

You mean, only SizeClassAllocator32::kSpaceBeg, right?
Why?

Theoretically, we may gain something by making kSpaceBeg non-zero.
Before we actually do it, SizeClassAllocator32::kSpaceBeg is kind of useless, but it also doesn't harm much.

It is broken. We never used it. We don't have tests for it.
See previous comments.

kcc added a comment.Dec 8 2015, 3:29 AM

No strong opinion here. If you think we should remove it -- ok, let's do it.
We can reinstate it back if needed.

kubamracek updated this revision to Diff 55819.May 2 2016, 8:06 AM
kubamracek edited edge metadata.

Ping. Adding a test case.

dvyukov accepted this revision.May 2 2016, 8:10 AM
dvyukov edited edge metadata.
This revision is now accepted and ready to land.May 2 2016, 8:10 AM
This revision was automatically updated to reflect the committed changes.