[compiler-rt] [win/asan] Improve SharedReAlloc with HEAP_REALLOC_IN_PLACE_ONLY. (PR #132558)

[via llvm-commits]

bernhardu wrote:

@thurstond, sorry for not responding to this earlier.

> My earlier comment stands:
> _"I still wonder whether implementing this will reduce bug detection.

In the previous version shrinking was possible, but either accesses wrote an "use-after-poison" message, or a later version took changes to the core asan allocator. I think these two approaches were not a reduction in bug detection.
The current version may reduce bug detection, as it ignores shrink requests, so accesses to the part of an allocation, which the appilcation tried to free, get not detected, if the application also ignores the returned NULL.

> For example, some code might incorrectly not expect NULL to be returned (even though the allocator is allowed to do so),

>From my point of view an application using HEAP_REALLOC_IN_PLACE_ONLY and receiving NULL is allowed to access the memory via the old pointer, as the allocation should stay unmodified in that case (see [documentation, near HEAP_REALLOC_IN_PLACE_ONLY](https://learn.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heaprealloc).
If it tries to access the memory via the returned NULL, and therefore crashing, I assume this points out this is an application bug, which would be a good thing to make visible?

> or perhaps returning NULL will force the user code to call realloc without HEAP_REALLOC_IN_PLACE_ONLY (thereby allowing stronger use-after-free detection)."_

This is the intention of the last version to force the application into using using the failure path. I think the application has to be always prepared for receiving NULL when growing the allocation.
When the application does not know how many bytes it exactly needs and reserved more memory in the first place and wants to free the unneeded part, the application may just not care and just ignore if the shrinking failed or not.

> Would you consider placing the new behavior configurable via an ASAN_OPTION?

I did not yet consider making this configurable because in my opinion returning a differenct location with HEAP_REALLOC_IN_PLACE_ONLY is simply a bug.

But if this is the desired way to proceed I would happily try to add this in the next push?
Thanks for looking at this pull request.

https://github.com/llvm/llvm-project/pull/132558