Page MenuHomePhabricator

Add free_on_realloc_zero=true flag for compatibility with allocators which allow a realloc(p, 0) and don't free the pointer.
ClosedPublic

Authored by filcab on Mar 23 2017, 10:51 AM.

Details

Summary

I know of two implementations that do this (ASan is not protecting against accessing the returned memory for now, just like malloc(0)):
SIE libc on the PS4
dlmalloc has a flag for this

This allows us to properly support this behaviour.

Diff Detail

Repository
rL LLVM

Event Timeline

filcab created this revision.Mar 23 2017, 10:51 AM
vsk edited edge metadata.Mar 23 2017, 11:41 AM

Ah, so is it also allowed to access P[0] under ASan after doing P = malloc(0)? If so, I can't spot any issues here.

kcc edited edge metadata.

I am horrified by the amount of flags we already have, and would like to reduce it, not increase it.
Can you please explain in more detaild why anyone would need this behavior and how this behavior matches the C/C++ standard behavior?

kcc added a comment.Mar 23 2017, 1:45 PM

The standard says the behaviour is implementation defined, IIRC (I can quote later).

Yes, please!

I can keep this patch private if it's too much of a problem.

It's not too much of a problem, and I *am* interested to have reasonable patches upstream.
Let's see what the standard says.

kcc added a comment.Mar 23 2017, 4:30 PM

Ok, let's do it. But I'd like to choose a better flag name.

lib/asan/asan_allocator.cc
803 ↗(On Diff #92834)

maybe "free_and return_null_on_realloc_zero"?

Os some other name with the opposite meaning (and default=false):

allocator_returns_non_null_on_realloc_zero?
alekseyshl added inline comments.Mar 23 2017, 4:58 PM
lib/asan/asan_allocator.cc
806 ↗(On Diff #92834)

No need in else block after return, less nesting is better.

test/asan/TestCases/realloc.cc
5 ↗(On Diff #92834)

--allow-empty --check-prefix=NO-FREE

to match other tests

5 ↗(On Diff #92834)

Why do you need --allow-empty? Isn't it always supposed to print "Allocated something on realloc(p, 0)"?

11 ↗(On Diff #92834)

Why argc and not the particular number of bytes?

13 ↗(On Diff #92834)

if (p) {
} else {
}

filcab updated this revision to Diff 92921.Mar 24 2017, 4:46 AM

Address revision comments.

filcab updated this revision to Diff 92922.Mar 24 2017, 4:50 AM
filcab marked 5 inline comments as done.

Address review comments

lib/asan/asan_allocator.cc
803 ↗(On Diff #92834)

I phrased it in terms of the allocator, and explicitly mentioned is returns null new proposal for name (it's a bit long, but I can't shorten it and still mention all those points): allocator_frees_and_returns_null_on_realloc_zero

806 ↗(On Diff #92834)

Thanks. Done!

test/asan/TestCases/realloc.cc
5 ↗(On Diff #92834)

Was from a previous iteration of the test. Removed.

11 ↗(On Diff #92834)

It was originally to try to avoid clang optimizing away the memory allocations. Which doesn't make sense since I'm passing -O0. Doesn't hurt, though. Let me know if you'd rather have a constant anyway.

13 ↗(On Diff #92834)

Added braces.

alekseyshl accepted this revision.Mar 24 2017, 1:56 PM
alekseyshl added inline comments.
test/asan/TestCases/realloc.cc
11 ↗(On Diff #92834)

Yes, please.

asm volatile("" : : "r"(p) : "memory"); after malloc should prevent optimizations.

This revision is now accepted and ready to land.Mar 24 2017, 1:56 PM
This revision was automatically updated to reflect the committed changes.
filcab marked an inline comment as done.