Field destruction order - MiraclePtr in a partially destructed object

292 views
Skip to first unread message

Łukasz Anforowicz

unread,
Dec 11, 2020, 8:13:22 PM12/11/20
to memory-s...@chromium.org, Daniel Cheng, Keishi Hattori, Tom Sepez
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

unread,
Dec 11, 2020, 8:39:22 PM12/11/20
to Łukasz Anforowicz, memory-s...@chromium.org, Daniel Cheng, Keishi Hattori, Tom Sepez
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

unread,
Dec 11, 2020, 9:32:27 PM12/11/20
to Bruce Dawson, Łukasz Anforowicz, memory-s...@chromium.org, Keishi Hattori, Tom Sepez
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

unread,
Dec 12, 2020, 11:50:29 AM12/12/20
to Daniel Cheng, Bruce Dawson, Łukasz Anforowicz, memory-s...@chromium.org, Keishi Hattori, Tom Sepez
> What a fascinating bug. How many instances of these are you seeing?
I believe Keishi found (and fixed) a handful of these bugs.


Daniel Cheng

unread,
Dec 12, 2020, 4:37:20 PM12/12/20
to Bartek Nowierski, Bruce Dawson, Łukasz Anforowicz, memory-s...@chromium.org, Keishi Hattori, Tom Sepez, K Moon, Jan Wilken Dörrie
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

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

unread,
Dec 13, 2020, 8:46:20 PM12/13/20
to Daniel Cheng, Bartek Nowierski, Bruce Dawson, Łukasz Anforowicz, memory-s...@chromium.org, Keishi Hattori, Tom Sepez, K Moon, Jan Wilken Dörrie
+1 to fixing.

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





--
Kentaro Hara, Tokyo

Daniel Cheng

unread,
Dec 15, 2020, 4:41:19 PM12/15/20
to Kentaro Hara, Bartek Nowierski, Bruce Dawson, Łukasz Anforowicz, memory-s...@chromium.org, Keishi Hattori, Tom Sepez, K Moon, Jan Wilken Dörrie, Kostya Serebryany
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

unread,
Dec 15, 2020, 6:47:48 PM12/15/20
to memory-safety-dev, Daniel Cheng, Bartek Nowierski, Bruce Dawson, Łukasz Anforowicz, memory-s...@chromium.org, Keishi Hattori, Tom Sepez, K. Moon, Jan Wilken Dörrie, Kostya Serebryany, Kentaro Hara, Ricky Zhou
rickyz@ has kindly found an existing feature request for sanitizers: https://github.com/google/sanitizers/issues/596
Reply all
Reply to author
Forward
0 new messages