[PATCH] D41664: Remove test which assumed array cookies can't be poisoned when using an operator new defined in a class

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

In https://reviews.llvm.org/D41664#973746, @kcc wrote:

> Let me rephrase the question. 
>  Is the code in new_array_cookie_with_new_from_class.cc a valid C++? 
>  I.e. is the code allowed to access *reinterpret_cast<uintptr_t*>(Foo::allocated) at line 38?


Technically it is. Just like overriding malloc, saving a pointer to every block, and occasionally reading from those pointers. Both these cases will trigger ASan errors on array cookies (the malloc one will trigger it even without my patch, not on this example, but in other, simpler, ones).

> If this code is valid, your change https://reviews.llvm.org/D41301 will make asan bark on a valid code, which is exactly what this test is protecting from.

The `poison_array_cookie` functionality lives between the C++ standard and the C++ Itanium ABI, as you know. Since the C++ ABI doesn't forbid reading from the array cookie in general, this specific Asan feature can error on valid C++ code. It's still very useful to have it on by default. If someone sees a false positive on one of these (because they implemented a special allocator (e. g: A compacting allocator, which can move objects)), they'll need to disable array cookie poisoning. I think that's why the flag is there. I think treating all array cookies equally is a valuable feature (we've already had a bug report about this, where they were expecting to have the array cookies poisoned, but they weren't getting any errors due to using an overridden operator new (which still had an array cookie, just not poisoned)).

I think poisoning "some" array cookies is less useful than poisoning "all" array cookies. Especially since we already have the escape hatch anyway, for special cases.

Thank you,
Filipe

P. S: Another thing we can do is have a different variable called `__asan_really_poison_array_cookie` (or something), and use that one for overridden (with extra arguments) operator new[]. I don't think we need that complexity, though. And explaining this (or why some array cookies don't get poisoned) in documentation also doesn't seem like the best thing to do.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D41664