Is it memory safe to use on-stack raw pointers for cross-thread objects?

[Xiaocheng Hu]

I'm trying to implement a GC-ed concurrent data structure with exactly one writer thread and one reader thread, and am wondering about memory safety issues.

For example, think about a linked list:

```
struct MyNode : public GarbageCollected<MyNode> {
  Member<MyNode> next;
  int value;
};

class MyLinkedList : public GarbageCollected<MyLinkedList> {
 public:
  // Run on writer thread.
  void InsertAfter(MyNode* new_node, MyNode* prev);
  void RemoveNext(MyNode* prev);

  // Runs on reader thread. Returns the first node satisfying some predicate.
  MyNode* Find(Pred some_predicate) const;

  // A variant of Find() but returns CrossThreadPersistent<>.
  CrossThreadPersistent<MyNode> Find2(Pred some_predicate) const;

 private:
  // Mutex guarding linked list operations.
  base::Lock_;

  Member<MyNode> head_;
};
```

All `MyLinkedList` and `MyNode` objects live on the writer thread. The reader thread keeps a `CrossThreadPersistent<MyLinkedList>` to access the structure.

In the above example, is `Find()` memory safe? There seems to be a risk that the returned pointer, which is a raw pointer on the stack of the reader thread, will not be seen by the writer thread's garbage collector. As a result, the return object may be GC-ed at any time, leaving a dangling pointer on the reader thread.

I suppose `Find2()` is memory safe, but that also means that the reader thread has to manually keep the objects that are being used alive. If the reader thread is running some complicated task and needs to keep a lot of nodes alive, then we need to maintain a lot of CrossThreadPersistent<>, which looks pretty bad to me. At least I assume this isn't performant.

Thanks!

[Omer Katz]

Oilpan doesn't support multi-threading and reading objects concurrently is generally unsafe, regardless of how you hold them, especially if you intend to access fields, etc.

Yes, CrossThreadPersistent exists and is being used, but there's ongoing work to replace it with the thread-safe CrossThreadHandle (which only lets you retain an object, not access it) and not all existing usecases are easy to replace.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABDsFEZjZOwiU4%2B%3DX%3DEAYGU9q7L3VcuZreFRAj%3D6X1cX4SP03A%40mail.gmail.com.

--

Omer Katz

V8, Software Engineer

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. 

     

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Jeremy Roman]

I'm quite skeptical of this being safe, at least without external synchronization of some kind that assures that the heap in which it belongs is alive at all. Even CrossThreadPersistent is mainly used to keep objects alive through a pointer until that pointer returns to the thread that owns the heap; raw pointers in another thread's stack certainly won't keep it alive.

--

[Xiaocheng Hu]

Ah right, the heap might be gone...

Assuming the heap won't be gone, it seems that `Find2()` in my example is still unsafe, either. In particular, the assignment operator `CrossThreadPersistent<T>::operator=(Member<T>& member)` isn't implemented in a thread safe manner -- the object might be GC-ed between loading the value of Member<> into an on-stack raw pointer and storing the raw pointer into the CrossThreadPersistent<>.

So the conclusion is that CrossThreadPersistent must be created on the object's owner thread, which is similar to how CrossThreadHandles are created. And other threads have no way to access the same-thread handles (Member<> etc) safely, even assuming the heap isn't gone.

[Michael Lippautz]

Just to confirm your observations: 

- Yes, CrossThreadPersistent doesn't have a thread-safe way to assign from Member or raw pointers. It needs to be constructed on the object's owning thread. The difference to CrossThreadHandle is that it has an unsafe deref which is used in certain places for historic reasons.

- As for accessing Member<> in a thread-safe way: Your observation that this is unsupported is also correct. Technically, it would be fine (even with TSAN) to always write through the object's owning thread and use external synchronization for reads. This is brittle though.

Note that the decision to not invest in DX here for Blink is deliberate (see https://groups.google.com/a/chromium.org/g/platform-architecture-dev/c/sL4iFI47zSg). We have certainly come up with primitives for e.g. V8-internal usage that work reasonably well.

-Michael

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABDsFEaT%2BPwQrcpkgNDLB79kuWUp3KaOowFxu356BUHa7fZM3Q%40mail.gmail.com.