poison the rawPtr in ~RefPtr() after release

RESOLVED WONTFIX

Status

()

Core
MFBT
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: jerry, Assigned: jerry)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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<>.
(Assignee)

Comment 1

2 years ago
Created attachment 8750716 [details] [diff] [review]
poison the mRawPtr in ~RefPtr(). v1
Attachment #8750716 - Flags: review?(nfroyd)
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?
(Assignee)

Comment 3

2 years ago
(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<>.
Status: NEW → ASSIGNED
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.
Attachment #8750716 - Flags: review?(nfroyd) → feedback?(continuation)
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?
Attachment #8750716 - Flags: feedback?(continuation)
(Assignee)

Comment 6

2 years ago
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()
                               }
Flags: needinfo?(continuation)
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.
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.
Flags: needinfo?(continuation)
Bug 929478 is our bug for ThreadSanitizer, if you are curious.
(Assignee)

Comment 10

2 years ago
Thanks, turn to check the threadSanitizer.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.