Field destruction order - MiraclePtr in a partially destructed object

[Łukasz Anforowicz]

Hello,

I wanted to mention https://bugs.chromium.org/p/chromium/issues/detail?id=1157988#c8 and discuss with the group.  This bug is Restrict-View-Google like all crashes, but a simplified example would be:

struct Foo {
  int id;
};

struct Inner {

  Inner(Outer* outer) : outer_(outer) {}

  ~Inner() {

    int id = outer_->foo_->id;

  }

  Outer* outer_;

};

struct Outer {

  // Caller guarantees that |foo| will be alive longer than Outer.

  Outer(Foo* foo) {

    foo_ = foo;

  }

  Inner inner_;

  CheckedPtr<Foo> foo_;

};

In the example above, |Outer::foo_| field is destructed first - at that time CheckedPtr's destructor decrements BackupRefPtr's ref-count and (because the decrement might have led to a |free|) has to make sure that this CheckedPtr cannot be dereferenced (by setting it to |nullptr|).  Later |Outer::inner_| field is destructed and this may lead to dereferencing the |Outer:foo_| pointer field - this would have worked with raw pointers but crashes with CheckedPtr.

One fix would be to move all the rewritten pointer fields to the top (ensure that CheckedPtr fields are declared before any other non-POD fields).  This can be seen as an additional ergonomics burden of the BackupRefPtr approach.  In theory, the rewriter could reorder field declarations, but in practice this is tricky, because comments need to be also moved, but 1) comments are already stripped when working with the AST and 2) sometimes a single comment applies to multiple fields and 3) private:/protected:/etc aspects need to be preserved.

Another fix would be to avoid using partially-destructed objects (e.g. Outer where some fields have been destructed and some haven't).  This also seems difficult in practice (even if all the typical guidelines are followed - e.g. avoiding calling virtual methods in destructors).  I wonder what the C++ spec has to say about accessing raw pointers (or other POD fields) that have been "destructed" - maybe this is somehow Undefined Behavior even without CheckedPtr in the picture?

Are there any other ways to address this?  Maybe we can hope that fixing the field order in a handful of cases will be sufficient?

At any rate, we probably should document this gotcha in //base/memory/checked_ptr.md - I'll try to put together a CL with some doc tweaks

[Bruce Dawson]

What a fascinating bug. How many instances of these are you seeing? It feels like using raw pointers after they are "destroyed" might be undefined behavior, but I am definitely not a language lawyer.

If we can manually fix a handful of cases and then update the documentation that would be ideal. Here's hoping...

--
You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAA_NCUHDSP%2Bd0-Q5FH%2Bi_sZ4Pp8_Q2N0%3DeU5S4BQTX9Xa%2B4x2g%40mail.gmail.com.
For more options, visit https://groups.google.com/a/chromium.org/d/optout.

--

Bruce Dawson

[Daniel Cheng]

I read the standard and did some digging around. With raw pointers, it's somewhat unclear in the C++ standard if non-class types have destructors, and when they're considered destroyed. Certainly, there was some hesitation about declaring the non-rewritten version as ok though…

Automatically reordering fields feels kind of dangerous, so I'm also hoping we can get away with just updating a few places…

Daniel

[Bartek Nowierski]

> What a fascinating bug. How many instances of these are you seeing?

I believe Keishi found (and fixed) a handful of these bugs.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAF3XrKrV5rfxEQPjKbM4DzkKUOLMAcNUTA4wpmiQqQ4N7DeBGA%40mail.gmail.com.

[Daniel Cheng]

After some discussion on Slack, the conclusion is:

- prior to C++20, this is not UB

- C++20 makes this explicitly UB

jdoerrie@ constructed a constexpr example to demonstrate this is UB in C++20: https://godbolt.org/z/qTbxn1

kmoon@ also dug up a SO discussion (https://stackoverflow.com/questions/61067402/what-is-the-effect-of-call-to-a-trivial-destructor) and C++ WG note which discusses this exactly issue: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2256

So... it sounds like something we'll have to fix anyway, so we probably don't want to do it automatically in the rewriting step (or at least not as part of the MiraclePtr rewriter).

Daniel

[Kentaro Hara]

+1 to fixing.

CheckedPtr is useful to detect the places and improve our code base :D

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAF3XrKo%3DO1me0%2BmUOvmit30o00gVGT6vOsooMDWeGgXgJ4-isA%40mail.gmail.com.

For more options, visit https://groups.google.com/a/chromium.org/d/optout.

--

Kentaro Hara, Tokyo

[Daniel Cheng]

I did a little experiment earlier today.

Test program:

#include <stdio.h>
#include <memory>
 
int global = 12345;
 
struct B;
 
struct A {
  std::unique_ptr<B> b = std::make_unique<B>(this);
  int* ptr = &global;
};
 
struct B {
  explicit B(A* a) : a(a) {}
  ~B() { printf("%d\n", *a->ptr); }
  A* a;
};
 
int main() {
  A a;
}

Unfortunately it doesn't seem like our sanitizers catch this today:

dcheng@nausicaa:~/src/chrome/src$ third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++20 -fsanitize=address test.cc
dcheng@nausicaa:~/src/chrome/src$ ./a.out
12345
dcheng@nausicaa:~/src/chrome/src$ third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++20 -fsanitize=memory test.cc
dcheng@nausicaa:~/src/chrome/src$ ./a.out
12345
dcheng@nausicaa:~/src/chrome/src$ third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++20 -fsanitize=undefined test.cc
dcheng@nausicaa:~/src/chrome/src$ ./a.out
12345

In theory, it shouldn't be too hard to add it, and it might be nice to catch these issues without requiring a MiraclePtr rewrite.

Daniel

[Łukasz Anforowicz]

rickyz@ has kindly found an existing feature request for sanitizers: https://github.com/google/sanitizers/issues/596