1271582 - poison the rawPtr in ~RefPtr() after release

Status: RESOLVED WONTFIX

(Core :: MFBT, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

In bug 1257996, we might have incorrect usage for RefPtr<>, this bug try to poison the rawPtr in RefPtr<>. That might catches the multi-thread access problem of RefPtr<>.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: poison the mRawPtr in ~RefPtr(). v1

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

https://hg.mozilla.org/mozilla-central/annotate/043082cb7bd8490c60815f67fbd1f33323ad7663/image/SurfaceCache.cpp#l483

|RefPtr<CachedSurface> surface| is a stack variable. How can it be shared among multiple threads?

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to JW Wang [:jwwang] from comment #2)
> https://hg.mozilla.org/mozilla-central/annotate/
> 043082cb7bd8490c60815f67fbd1f33323ad7663/image/SurfaceCache.cpp#l483
> 
> |RefPtr<CachedSurface> surface| is a stack variable. How can it be shared
> among multiple threads?

For SurfaceCache.cpp#l483, no. Maybe we expose the imgFrame somewhere and delete that rawPtr. So we have bug 1257996 crash. This patch can't catch that situation.
But I think we still can have this poison change. At least, that could find some race condition problems for RefPtr<>.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8750716 [details] [diff] [review]
poison the mRawPtr in ~RefPtr(). v1

Review of attachment 8750716 [details] [diff] [review]:
-----------------------------------------------------------------

Marking f+ for now (apparently I can't mark f+ and ask for feedback...).  I think the idea is potentially interesting, but I'm not sure how applicable it would be; I'm having trouble seeing how it would help detect race conditions as suggested in comment 3.

Andrew, do you think this is a good idea?  Are there more common scenarios than the one I outlined below where this could mitigate problems?

::: mfbt/RefPtr.h
@@ +78,5 @@
>      }
> +
> +#ifdef DEBUG
> +    // Poison the raw ptr.
> +    memset(&mRawPtr, 0xa5, sizeof(mRawPtr));

I'm finding it hard to envision a scenario where this helps, but the memory filling we get from jemalloc:

http://dxr.mozilla.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#167
http://dxr.mozilla.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#4607 (and other places, search for |opt_poison|)

doesn't.  And the memory poisoning from jemalloc is active all the time, not just in DEBUG.

I guess this would help in a case where you had:

struct S {
  // other members as appropriate
  RefPtr<T> mPtr;
};

nsTArray<S> array;

and you pushed some |S| objects onto the array, handed out pointers to those objects (the |S| objects, not the individual members in the |S| objects), and then removed things from the array.  You wouldn't necessarily free the storage associated with those objects, but you don't want people to access those objects that you've handed out, as they'd be accessing destroyed memory.  But then it's not clear to me that we shouldn't be doing this in nsTArray or similar...or would we want to do it in both places?

In any event, I think we'd want to use mozPoisonValue() from mfbt/Poison.h to obtain the value to store in the pointer.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8750716 [details] [diff] [review]
poison the mRawPtr in ~RefPtr(). v1

As Nathan said, jemalloc poisons on free. Also, our standard refcount classes do thread safety checks, so issues with that should already be detected in debug builds. What is the scenario that you feel that this helps with?

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

How about this?
call assignment op while ~dtor()?

RefPtr<A> ref1 = new A;
RefPtr<A> ref2 = new A;


Call ref1::~dtor at ThreadA    Call ref1=ref2 at ThreadB

~RefPtr() {
  if (rawPtr) {
    rawPtr->Release()  <--+
  }                       |  assign_assuming_AddRef(...) {
}                         |    if (oldPtr) {
                          +-->   old->Release()
                               }

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

RefPtr<> is never thread-safe on its own. You need synchronization if you want to share a RefPtr<> (mostly as a class member) across threads.

Comment 8

[Andrew McCreight [:mccr8]]

Ok, right, you are concerned about races in using the rawPtr field of RefPtr<> itself. Why would ThreadA be calling the dtor of RefPtr? If the RefPtr is part of a heap-allocated structure that is being freed, then it would get poisoned by jemalloc. If it is part of a global data structure, then it wouldn't be getting destroyed until late in shutdown anyways. I guess there is the nsTArray scenario that froydnj mentioned.

But dtor poisoning doesn't help in the case where both threads are calling assign_assuming_AddRef, which I'd imagine would be a much bigger issue. Have you tried using ThreadSanitizer? We have a lot of issues to fix with it, so there would be unrelated noise, but it should be able to detect races like this.

Comment 9

[Andrew McCreight [:mccr8]]

Bug 929478 is our bug for ThreadSanitizer, if you are curious.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Thanks, turn to check the threadSanitizer.