devtools: add back pointers detection to blink-gc-plugin [chromium/src : main]

1 view
Skip to first unread message

Paul Semel (Gerrit)

unread,
Mar 21, 2023, 12:19:25 PM3/21/23
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Daniel Cheng, danakj, Alexis Menard, David Bokan, chromium...@chromium.org, Dirk Schulze, Olga Gerchikov, Nate Chapin, Kenneth Rohde Christiansen, Hiroki Nakagawa, Stephen Chenney, Alex Keng

Attention is currently required from: Daniel Cheng, danakj.

View Change

1 comment:

  • File third_party/blink/renderer/core/fileapi/file_reader.h:

    • Patch Set #3, Line 139: UngarbageCollected<std::unique_ptr<FileReaderLoader>> loader_;

      @dch...@chromium.org here is how it could look like to make the pointer safer.. […]

      On the other hand, the issue is that for this particular example, the protection is useless because the class is resetting the pointer in its destructor... Maybe we'd need to force a `Dispose` method to be called in `UngarbageCollected`?

To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Comment-Date: Tue, 21 Mar 2023 16:19:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Semel <paul...@chromium.org>
Gerrit-MessageType: comment

Paul Semel (Gerrit)

unread,
Mar 22, 2023, 11:25:56 AM3/22/23
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Tricium, Daniel Cheng, danakj, Alexis Menard, David Bokan, chromium...@chromium.org, Dirk Schulze, Olga Gerchikov, Nate Chapin, Kenneth Rohde Christiansen, Hiroki Nakagawa, Stephen Chenney, Alex Keng

Attention is currently required from: Daniel Cheng, danakj.

View Change

7 comments:

  • Patchset:

  • File third_party/blink/renderer/platform/weborigin/kurl.h:

    • Patch Set #2, Line 270: GC_PLUGIN_IGNORE("")

      Maybe the CL description could include the high-level approach we're using to detect this? […]

      Right, that one is simply because down the chain, the object stores a pointer to itself, and thus, since we only have composition from there, the stored address is a "garbage collected" one. I guess all occurrences of this object will be marked `GC_SAFE_FIELD`.

  • File tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.h:

  • File tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.cpp:

    • Patch Set #2, Line 41: Name

      Nit: here and throughout Chrome naming conventions (e.g. […]

      Done

    • Patch Set #2, Line 107: CheckBackPointers(edge, parent->IsUniquePtr() || parent->IsRefPtr());

      Can we add a brief comment explaining this logic and the if conditional? […]

      Done

  • File tools/clang/blink_gc_plugin/RecordInfo.h:

  • File tools/clang/blink_gc_plugin/tests/forbidden_fields.h:

To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Comment-Date: Wed, 22 Mar 2023 15:25:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
Gerrit-MessageType: comment

Paul Semel (Gerrit)

unread,
Mar 24, 2023, 6:16:16 AM3/24/23
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Chromium LUCI CQ, Tricium, Daniel Cheng, danakj, Alexis Menard, David Bokan, chromium...@chromium.org, Dirk Schulze, Olga Gerchikov, Nate Chapin, Kenneth Rohde Christiansen, Hiroki Nakagawa, Stephen Chenney, Alex Keng

Attention is currently required from: Daniel Cheng, danakj.

View Change

1 comment:

To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Comment-Date: Fri, 24 Mar 2023 10:16:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Daniel Cheng (Gerrit)

unread,
Mar 28, 2023, 4:41:30 AM3/28/23
to Paul Semel, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Chromium LUCI CQ, Tricium, Daniel Cheng, danakj, Alexis Menard, David Bokan, chromium...@chromium.org, Dirk Schulze, Olga Gerchikov, Nate Chapin, Kenneth Rohde Christiansen, Hiroki Nakagawa, Stephen Chenney, Alex Keng

Attention is currently required from: Paul Semel.

View Change

10 comments:

  • File tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.h:

    • Patch Set #6, Line 48: ValueEdge's

      Hmm... maybe this should be `edge`? (Since there is no exact match for ValueEdge)

    • Patch Set #6, Line 50: should_check_subfields

      Let's also explain this arg and how to use it in the comment (e.g. what is a subfield?)

  • File tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.cpp:

    • Patch Set #6, Line 42: static inline bool isMatchingDecl(llvm::StringRef Name,

      Please add a comment about this helper explaining its arguments and purpose.

    • Patch Set #6, Line 47: baseDecl

      Nit: use google C++ style naming (i.e. Name should be name above, baseDecl should be base_decl, isMatchingDecl() should be IsMatchingDecl(), et cetera)

    • Patch Set #6, Line 49: if (Name == baseDecl->getQualifiedNameAsString()) {

      (Having a function-level comment might eliminate the need; otherwise, what's the reason for this comparison? What are we looking for? et cetera)

    • Patch Set #6, Line 52: // FIXME: should we recurse here?

      File a bug for this? And maybe add a bit more detail? Why do we want to recurse? Presumably we want to check all the base classes right?

    • Patch Set #6, Line 112: uniquely

      Is this supposed to be "unlikely"?

  • File tools/clang/blink_gc_plugin/tests/forbidden_fields.h:

    • Patch Set #6, Line 48: T

      I recommend more descriptive names here. The test case is a bit hard to read right now, because the various declarations are in reverse order.

      It might also be helpful to separate things out a bit.

    • Patch Set #6, Line 66: };

      So just to clarify, the situation we are trying to prevent is...

      ```
      class TracedEmbeddedObject {
      };

      class ManagedByOilpan : public GarbageCollected<ManagedByOilpan> {
      TracedEmbeddedObject embedded_object;
      };
      class NonGarbageCollected {
      // dangerous, because this is usually part of ManagedByOilpan
      TraceEmbeddedObject* embedded_object;
      };
      ```

      Or am I misunderstanding the scope of the problem?

      I'm also trying to understand why it's a problem for B (which owns C) to have raw pointers to back to B (as a T). This does seem like a safe case, or am I missing something here? Or is it because we have no way to distinguish so we err on the side of over warning?

  • File tools/clang/blink_gc_plugin/tests/forbidden_fields.txt:

To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Paul Semel <paul...@chromium.org>
Gerrit-Comment-Date: Tue, 28 Mar 2023 08:41:18 +0000

Paul Semel (Gerrit)

unread,
Mar 28, 2023, 9:45:03 AM3/28/23
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Chromium LUCI CQ, Tricium, Daniel Cheng, danakj, Alexis Menard, David Bokan, chromium...@chromium.org, Dirk Schulze, Olga Gerchikov, Nate Chapin, Kenneth Rohde Christiansen, Hiroki Nakagawa, Stephen Chenney, Alex Keng

Attention is currently required from: Daniel Cheng.

View Change

8 comments:

  • File tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.h:

    • Hmm... […]

      Done

    • Let's also explain this arg and how to use it in the comment (e.g. […]

      Right, what do you think of it now?

  • File tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.cpp:

    • Patch Set #6, Line 42: static inline bool isMatchingDecl(llvm::StringRef Name,

      Please add a comment about this helper explaining its arguments and purpose.

    • Done

    • Nit: use google C++ style naming (i.e. […]

      Done

    • (Having a function-level comment might eliminate the need; otherwise, what's the reason for this com […]

      Done

    • File a bug for this? And maybe add a bit more detail? Why do we want to recurse? Presumably we want […]

      Yes, exactly!
      I opened a bug to potentially do that in a follow-up: https://crbug.com/1428550.

    • Done

  • File tools/clang/blink_gc_plugin/tests/forbidden_fields.h:

    • Patch Set #6, Line 66: };

      So just to clarify, the situation we are trying to prevent is... […]

      So, for the code example you gave, we would not issue an error because we'd only lookup `ManagedByOilpan`, which doesn't contains (nor its embedded objects) pointers.

      This is related to https://docs.google.com/document/d/1EdNzrox5XsTkjAG0MLWeMhpCm_SrjOVCTqvlu9HEv1o/edit?resourcekey=0-oAx_fS7rOnKaMbbR-B2HNg#heading=h.waqiqxufvgqd.

      But basically, while not **directly** unsafe, the test case might still be considered unsafe since we are keeping a raw pointer to GC memory, but GC is not aware of this pointer, and so there are some cases where this can become really bad, like for example:

      • B giving ownership of C to some other classes that is not GC'd as well.
      • C calling some functions during its destructor that actually uses the pointer.

      I guess I'd agree with you that the danger of the `unique_ptr` case is more obvious, but I feel like even for the other case, the raw pointer is still not GC-aware, and thus the pointer will inevitably become "dangling" between the disposal phase and the moment the destructor of the class gets called. And again, nothing prevents `B` from giving the ownership of C somewhere else, which then make the issue equivalent to the `unique_ptr` issue.

      Does it make sense to you?

To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Comment-Date: Tue, 28 Mar 2023 13:44:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Paul Semel (Gerrit)

unread,
Apr 4, 2023, 11:22:32 AM4/4/23
to Anton Bikineev, danakj, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Daniel Cheng

Attention is currently required from: Anton Bikineev, Daniel Cheng.

Paul Semel would like Anton Bikineev to review this change.

Paul Semel removed danakj from this change.

View Change

devtools: add back pointers detection to blink-gc-plugin

To detect those back pointers, we are recursively traversing GC roots
and checking for composed objects, likewise what's already done for
forbidden fields. Whenever we encounter a raw pointer, we check that it
doesn't refer to a type we already encounter during the traversal. In
such case, we consider this pointer a back pointer, since it is likely
to store the address of the object back in the chain.
For smart pointers, we also traverse their subfields, since it is more
likely that we find back pointers there.

Classes retaining pointers to GC'd memory can be problematic, since
object lifetime and GC'd lifetime differ. Hopefully, we already prevent
classes to have pointers to root GC'd object. Unfortunately, it's hard
to prevent pointers to objects that are composed in a GC'd object. This
CL attempts at preventing this.

Bug: 1381979
Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
---
M tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp
M tools/clang/blink_gc_plugin/BlinkGCPluginOptions.h
M tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.cpp
M tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.h
M tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp
M tools/clang/blink_gc_plugin/DiagnosticsReporter.h
M tools/clang/blink_gc_plugin/RecordInfo.cpp
M tools/clang/blink_gc_plugin/RecordInfo.h
A tools/clang/blink_gc_plugin/tests/back_pointers.cpp
A tools/clang/blink_gc_plugin/tests/back_pointers.flags
A tools/clang/blink_gc_plugin/tests/back_pointers.h
A tools/clang/blink_gc_plugin/tests/back_pointers.txt
A tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.flags
M tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.txt
A tools/clang/blink_gc_plugin/tests/forbidden_fields.flags
A tools/clang/blink_gc_plugin/tests/raw_ptr_to_gc_managed_class.flags
M tools/clang/blink_gc_plugin/tests/raw_ptr_to_gc_managed_class.txt
M tools/clang/blink_gc_plugin/tests/raw_ptr_to_gc_managed_class_error.flags
M tools/clang/blink_gc_plugin/tests/raw_ptr_to_gc_managed_class_error.txt
A tools/clang/blink_gc_plugin/tests/ref_ptr_to_gc_managed_class.flags
M tools/clang/blink_gc_plugin/tests/ref_ptr_to_gc_managed_class.txt
A tools/clang/blink_gc_plugin/tests/stack_allocated.flags
M tools/clang/blink_gc_plugin/tests/stack_allocated.txt
A tools/clang/blink_gc_plugin/tests/unique_ptr_to_gc_managed_class.flags
M tools/clang/blink_gc_plugin/tests/unique_ptr_to_gc_managed_class.txt
25 files changed, 271 insertions(+), 10 deletions(-)


To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Anton Bikineev <biki...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: Anton Bikineev <biki...@chromium.org>
Gerrit-MessageType: newchange

Daniel Cheng (Gerrit)

unread,
Apr 19, 2023, 4:24:18 AM4/19/23
to Paul Semel, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Anton Bikineev, Chromium LUCI CQ, Tricium, Daniel Cheng, Alexis Menard, David Bokan, chromium...@chromium.org, Dirk Schulze, Olga Gerchikov, Nate Chapin, Kenneth Rohde Christiansen, Hiroki Nakagawa, Stephen Chenney, Alex Keng

Attention is currently required from: Anton Bikineev, Paul Semel.

View Change

5 comments:

  • File tools/clang/blink_gc_plugin/CheckForbiddenFieldsVisitor.cpp:

  • File tools/clang/blink_gc_plugin/tests/forbidden_fields.h:

    • So, for the code example you gave, we would not issue an error because we'd only lookup `ManagedByOi […]

      OK, so just to make sure I understand, what this plugin is trying to warn against is:

      GCedObject holds a std::unique_ptr<NonGC>.
      NonGC has pointers to itself (or it owns objects that point to NonGC).

      The pointers back to GC are where the "back pointers" part of this CL is coming from. The idea being that if there are back pointers detected, it is more likely to lead to some sort of escape across the GC boundary problem, and thus, potential memory safety issues?

      My personal feeling is the heuristic, as implemented, is a bit too broad and also leads to warning spam (see the other comments). Looking at the followup CL, I see that at least some of the ignore annotations are to mark WebURLError, which looks completely innocuous. There are also other unintuitive places: KURL is not garbage-collected at all, so it's surprising to see the annotation there (even if it "makes sense"). Overall, I think this dilutes the value of having the GC_PLUGIN_IGNORE annotation.

      I see your point about GC to non-GC being a risk for dangling, but this feels like something that may be better suited for some sort of static analysis monitoring dashboard? Or is there a way to tune the heuristic a bit more?

  • File tools/clang/blink_gc_plugin/tests/stack_allocated.txt:

    • Patch Set #8, Line 17:

      ./stack_allocated.h:30:1: warning: [blink-gc] Class 'HeapObject' contains invalid fields.
      class HeapObject : public GarbageCollected<HeapObject> {
      ^
      ./stack_allocated.h:35:5: note: [blink-gc] From part object field 'm_part' here:
      StackObject m_part; // Cannot embed a stack allocated object.
      ^

      Is it expected for this warning to be doubled up?

    • Patch Set #8, Line 24: HeapObject* m_obj; // Does not need tracing.

      I don't feel like this is a good warning. This is specifically supposed to be allowed.

  • File tools/clang/blink_gc_plugin/tests/unique_ptr_to_gc_managed_class.txt:

    • Patch Set #8, Line 21: Vector<std::unique_ptr<HeapObject>> m_objs;

      Similarly I'm not quite sure about this: I'm a bit worried, because I don't feel like it's great to issue double warnings (we already have a warning on line 25, and then we have another warning here, which is kind of a false positive: the problem isn't that HeapObject isn't GCed, it's that it was incorrectly wrapped in unique_ptr here)

      The same is true of the raw ptr and scoped_refptr warnings above.

To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Anton Bikineev <biki...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Paul Semel <paul...@chromium.org>
Gerrit-Attention: Anton Bikineev <biki...@chromium.org>
Gerrit-Comment-Date: Wed, 19 Apr 2023 08:24:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Semel <paul...@chromium.org>
Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>

Paul Semel (Gerrit)

unread,
Apr 25, 2023, 11:37:52 AM4/25/23
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Anton Bikineev, Chromium LUCI CQ, Tricium, Daniel Cheng, Alexis Menard, David Bokan, chromium...@chromium.org, Dirk Schulze, Olga Gerchikov, Nate Chapin, Kenneth Rohde Christiansen, Hiroki Nakagawa, Stephen Chenney, Alex Keng

Attention is currently required from: Anton Bikineev, Daniel Cheng.

View Change

1 comment:

  • File tools/clang/blink_gc_plugin/tests/unique_ptr_to_gc_managed_class.txt:

    • Similarly I'm not quite sure about this: I'm a bit worried, because I don't feel like it's great to […]

      Ok, about that one, I will just make sure we do not overlap with another warning so that we don't issue those ones here.

To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Anton Bikineev <biki...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: Anton Bikineev <biki...@chromium.org>
Gerrit-Comment-Date: Tue, 25 Apr 2023 15:37:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>

Daniel Cheng (Gerrit)

unread,
Apr 26, 2023, 6:39:39 AM4/26/23
to Paul Semel, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, devtools-re...@chromium.org, devtools...@chromium.org, drott+bl...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jkarli...@chromium.org, jsbell+ser...@chromium.org, kainin...@chromium.org, kinuko+...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, mfoltz...@chromium.org, pdr+graphi...@chromium.org, ricea...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, steimel+watch...@chromium.org, yhiran...@chromium.org, Anton Bikineev, Chromium LUCI CQ, Tricium, Daniel Cheng, Alexis Menard, David Bokan, chromium...@chromium.org, Dirk Schulze, Olga Gerchikov, Nate Chapin, Kenneth Rohde Christiansen, Hiroki Nakagawa, Stephen Chenney, Alex Keng

Attention is currently required from: Anton Bikineev, Paul Semel.

View Change

1 comment:

  • File tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp:

    • Patch Set #8, Line 90: "Consider making those classes garbage collected.";

      tl;dr I think this error message is a bit unclear. Trying to think about how to adjust the wording led to a very long reply below... (sorry, if you want to meet again, I'm happy to try to schedule some time):

      Per our discussion earlier, we consider these back references problematic because:

      1. normally, we forbid native C++ pointers to GarbageCollected objects
      2. however, in the case we're detecting here, we likely have native C++ pointers to an object that is owned by a GarbageCollected
      3. we believe this is a violation of (1) above, even though this is indirect.

      I've been thinking for awhile now, and it's difficult for me to express why this is problematic in a compiler diagnostic, because there are several different issues (one from the email thread "detecting back pointers to inner GC’d object", and two from the doc discussing the original issue).

      Given the example in the thread:

      ```
      class B;
      class C {
      B* b_;
      };
      class B {
      C c_;
      };
      class A : GarbageCollected<A> {
      B b_;
      };
      ```

      ### Issue #1: the back reference `b_` is dangerous when used from `C`'s destructor.

      I'm still not sure I agree with this assertion. It actually seems like it should be safe (though like any other complex dtor, potentially a code smell)? This is just part of normal C++ destruction which runs as part of destroying `A`. However, it *could* be problematic if it ends up reaching out to another Oilpan object during finalization (e.g. `C` has a pointer to `D` and `D` has a WeakPersistent).

      ### Issue #2: `B` could be UaFed if unexpectedly destroyed.
      Because `B` is not `GarbageCollected`, Oilpan will not keep it live if `A` has no references. So if a method on `B` runs but `A` is no longer considered live, then `A` may be finalized, destroying `B`, and causing the method on `B` to UaF `this`.

      ### Issue #3: Back pointers to Oilpan objects are bad. The original back pointer highlighted in the doc escaped detection because:

      • `A` is `GarbageCollected` but implements `B::Client` and `B::Client` is not `GarbageCollected`.
      • `B` holds a `B::Client`, which is wildly unsafe, because `B::Client` refers to a `GarbageCollected` class.
      • `A` must implement a prefinalizer to cleanup the `B::Client` pointer.

      ### How these issues related to the diagnostic message

      This check is trying to find issue #1 (even though I'm not 100% sure it's unsafe) as a signal for danger. Of course, maybe issue #1 *is* dangerous and I'm just missing why (happy to hear more thoughts on this)... regardless it is finding places where we could convert non-GCed classes to GCed classes, so the check as implemented does have value. I just find it very indirect, and as such, I feel like the diagnostic message focusing on back references doesn't quite capture the real problem. The diagnostic message also points at the back reference field, even though the thing that should be changed/fixed is most likely something at a higher-level.

      I think someone who deeply understands the problem space can understand the fix; I feel like it will be confusing to someone who's newer to the codebase/Oilpan though.

      Issue #2 is (sort of) indirectly addressed by this check, in that it finds (some) non-GCed classes owned by GCed classes, where these sorts of problematic interactions can happen (generally because of a posted task using a `WeakPtr` that is not yet invalidated due to the delay between or because of a mojo receiver/remote that was not yet reset). So I think this actually suggests another potential approach: we can look for non-GarbageCollected types owned by GarbageCollected types that embed fields with the types mentioned above, because they provide ways of interacting with the task system that could cause the task system to reach back into zombie Oilpan objects.

      Functionally, this is very similar to the back reference approach already implemented here, but more focused on known-problematic types.

      Two additional notes:

      • This check would not trigger on `KURL` (which is IMO, more correct); however, it may also fail to highlight all potential candidates for converting non-GarbageCollected types to GarbageCollected types. But skimming through `FileReaderLoader` which was converted to GC in https://chromium-review.googlesource.com/c/chromium/src/+/4382944, it seems like this heuristic would have been good enough there.
      • it's worth noting that this is still an indirect heuristic; in crbug.com/1405256, it was not a `mojo::Receiver` or `base::WeakPtr` that was problematic. Actually, somewhat mysteriously, it was a `CrossThreadPersistent` while executing a task so I'm not sure why `this` got deleted... maybe I'm getting confused about which object is getting destroyed.

      #3 is not addressed by this directly either (but may be indirectly caught, similar to #2). I do feel like this suggests another potential heuristic (though it may result in too many false positives): when a pointer to a `GarbageCollected` class is cast to a base class type that does *not* inherit from `GarbageCollected` or `GarbageCollectedMixin`, we should warn because a pointer to a GCed class is escaping the viewability of the GC checker.

To view, visit change 4353238. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iba6cde3f54883bb8d8c9af5f703c3aa97c07e0d4
Gerrit-Change-Number: 4353238
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Semel <paul...@chromium.org>
Gerrit-Reviewer: Anton Bikineev <biki...@chromium.org>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Paul Semel <paul...@chromium.org>
Gerrit-CC: Alex Keng <shi...@microsoft.com>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Paul Semel <paul...@chromium.org>
Gerrit-Attention: Anton Bikineev <biki...@chromium.org>
Gerrit-Comment-Date: Wed, 26 Apr 2023 10:39:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Reply all
Reply to author
Forward
0 new messages