can you please upload the patch with "arc" tool, otherwise context is not visible

Not sure I understand this "if".
Can we get here when windows_hook_rtl_allocators==false?
If true, I don't see much difference after asan_inited set to true.

can you please clang-format the patch?

I'd expect this check is needed unconditionally, we can get to asan_malloc_usable_size even "if-then" about was executed.

Isn't better to use REAL in cases above instead of CHECK(Flags...) ?

will do

These if's are handling some edge cases when you hook the RTL allocators. There are some allocations which will occur before interception happens, so you have to check where the allocation came from.

I agree these look pretty gross and aren't super clear. I'll see if I can clean it up or write some comments to make them obvious.

I have my editor set to format on save/paste but something must have gone wrong. Will make sure the new sections are formatted properly

Oh good point, I'll check assertions again file-wide.

I need more context on what you mean here, where would it be better?

I've cleaned these up, they look a little weird at first but the new code makes sure to split up unsupported flags at allocation time when the runtime flag is set. In that case it is enough to check who owns the allocation, since the user can allocate a memory section without the unsupported flags but then use them on a later call to HeapSize or HeapReAlloc. When the RTL interception is turned on it's enough to just check who owns it, since if the user wants to use HEAP_NO_SERIALIZE on Free or Size it will just get ignored in the event ASAN owns the allocation. If the allocation is passed into the RTL library it will pass the flag through.

If the flag is turned off we keep the original behavior of asserting on those illegal flags, since there is no falling back to the original allocator in that version.

That's a lot, does that make sense or am I talking myself into garbage lol

this is marked as done, but I don't see a difference.

I expected something like:

if (flags()->windows_hook_rtl_allocators) {
  ...
  REAL...
}
CHECK(dwFlags == 0 && "unsupported heap flags");

without else

Right, I don't think this is actually the behavior that's needed though. It's the same reason as in HeapFree, when the RTL interception is turned on the check for incomaptible flags occurs only on (re)allocation. If the user is trying to pass unsupported flags to Size or Free for an allocation that's owned by the ASAN allocator we still want that size or free to come back to the user code, reguardless of where it lives. It's a weird situation that comes from juggling the original and asan allocator.

Separate patch?

Why these and new constants are macros and not regular consts or constexpr?

Leave this to compiler?

template<class ReAllocFunction...> 
static void* SharedReAlloc(ReAllocFunction reallocFunc, ...

this should be static on in namespace {}

void* SharedReAlloc?

Ignore my comment, now I understand your approach.

Still shows "Context not available."

could you please use consts?

Sure, I can wrap this up with some other msvc host compat stuff (coming soon)

No reason, I'll switch them

I did try this solution first, but we're stuck here because the template parameters would end up being pointers that are not compile-time constants. The arguments that are being pointed to are not the regular HeapAlloc address (which get patched), it's the pointers to the REAL(HeapAlloc) which are not known until hooking is finished.

The templated solution is cleaner but doesn't work in this situation. I will address these other concerns though it should be in the namespace. Thank you for the feedback!

been reading too much old win32 code sorry 😂
will fix!

Please capitalize the type name of the enum, so AllocationOwnership.

This is used in one place, I would sink it down to the use.

This is only used if RTL allocator hooking is enabled, I'd sink it into the if.

I think it would be simpler to drop this check and always do the IsTlsInitialized() check. I understand that adding the new heap interceptors is what causes us to run code prior to TLS initialization, but in the future ASan could change again in some other way that causes us to clal ASanTSDGet early and removing an if and defending against that possibility seems good.

Also, this isn't indented correctly, please fix that before committing.

There should be a space between the ){. I'd recommend using git-clang-format or some other tool to run clang-format to fix these minor issues.

This needs an || defined(__i386__) for MinGW, and same with defined(__x86_64__) below. And changing the #define _X86_ into #define _X86_ 1 avoids a warning about redefinition to a different value.

This file implicitly includes a lot of windows specific headers. See the comment below about "Intentionally not including windows.h here"; this brings back the same issue that was fixed earlier by removing the include of windows.h here, in particular errors like these:

compiler-rt/lib/asan/asan_malloc_win.cc:84:6: error: redeclaration of 'free' cannot add 'dllexport' attribute
void free(void *ptr) {
     ^

To avoid this, we need to make sure we don't see any inline function that uses the function free up to this point, when we redefine it with new attributes.

In MinGW, min() isn't defined by the headers included so far. I guess that could be considered a MinGW header bug (it's not defined if __cplusplus is defined - fixing that if necessary should hopefully be doable), but the issue mentioned above about explicitly avoiding windows platform headers might also lead to it being unavailable in MSVC style builds.

_Frees_ptr_opt_ is not available/defined in MinGW - can it be left out here, or does that cause errors about mismatching attributes in an MSVC build?