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

[filcab]

Summary

The original commit doesn't provide any rationale for this test, just
that "the cookie should not be poisoned" (why?).

This will allow us to re-land D41301, which make this test fail, as it poisons more cookies than before.

[filcab]

Ping!

[kcc]

The original commit doesn't provide any rationale for this test,

Well, apparently some real code broke and this test mimics that code (I don't remember the details, sadly).
How did you test https://reviews.llvm.org/D41301 ?

[filcab]

I have small tests on my side, but since it's really only about codegen, I haven't pushed a compiler-rt test (there was no bug to be fixed on compiler-rt's side).
I can change this test to remove that dependency instead of removing it, though.

[filcab]

BTW, the code around D41301 seem to have always been that way. Which made me unsure if this was a case of "only this got implemented and we never got around to adding the other cases", or if there was an actual problem somewhere.

[kcc]

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?

If this code is valid, your change D41301 will make asan bark on a valid code, which is exactly what this test is protecting from.

[filcab]

In 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 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.

[kcc]

Technically it is. Just like overriding malloc,

That's a weak analogy. You can't override malloc and still use asan with it.

Especially since we already have the escape hatch anyway, for special cases.

Which escape hatch do you mean?

[filcab]

In D41664#975091, @kcc wrote:

Technically it is. Just like overriding malloc,

That's a weak analogy. You can't override malloc and still use asan with it.

Derp, I forgot the open source version doesn't allow this. No problem.

Especially since we already have the escape hatch anyway, for special cases.

Which escape hatch do you mean?

I meant the poison_array_cookie flag.

How about this:
A -fsanitize-address-poison-all-array-new or similar (it would be all *except* placement new... Haven't got a better name, though).
That way, a user would be able to poison more array-new operators than the current solution. But we wouldn't break any legal C++ code.

Thank you,
Filipe

[kcc]

How about this:
A -fsanitize-address-poison-all-array-new or similar (it would be all *except* placement new... Haven't got a better name, though).
That way, a user would be able to poison more array-new operators than the current solution. But we wouldn't break any legal C++ code.

Yes, I think this has to be a separate compile-time flag, off by default, at least initially.
-fsanitize-address-poison-all-array-new is indeed a non-perfect name.
How about -fsanitize-address-poison-class member-array-new-cookie (or some such) to be more explicit?

[filcab]

I've posted D43013 to add the flag + behaviour to clang.
Thanks for pointing out the issues,

Filipe

test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cc

-// Test that we do not poison the array cookie if the operator new is defined
-// inside the class.
-// RUN: %clangxx_asan  %s -o %t && %run %t
-//
-// XFAIL: arm
-
-// UNSUPPORTED: ios
-
-#include <new>
-#include <stdlib.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <assert.h>
-struct Foo {
-  void *operator new(size_t s) { return Allocate(s); }
-  void *operator new[] (size_t s) { return Allocate(s); }
-  ~Foo();
-  static void *allocated;
-  static void *Allocate(size_t s) {
-    assert(!allocated);
-    return allocated = ::new char[s];
-  }
-};
-
-Foo::~Foo() {}
-void *Foo::allocated;
-
-Foo *getFoo(size_t n) {
-  return new Foo[n];
-}
-
-int main() {
-  Foo *foo = getFoo(10);
-  fprintf(stderr, "foo  : %p\n", foo);
-  fprintf(stderr, "alloc: %p\n", Foo::allocated);
-  assert(reinterpret_cast<uintptr_t>(foo) ==
-         reinterpret_cast<uintptr_t>(Foo::allocated) + sizeof(void*));
-  *reinterpret_cast<uintptr_t*>(Foo::allocated) = 42;
-  return 0;
-}