683630 - FixedMalloc::FindBeginning is calling thread-unsafe version of QueryOwnsObject

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Bug 663508 added FixedMalloc::FindBeginningAndSize, which calls QueryOwnsObject on the elements of the m_allocs array of the FixedMalloc singleton.  Since QueryOwnsObject was previously only used by DEBUG code, the associated patch also took the DEBUG guards out from around FixedAlloc::QueryOwnsObject

  TR changeset:6540
  http://hg.mozilla.org/tamarin-redux/rev/0bdd7ace90ff

  TR-serrano changeset:6321
  http://asteam.corp.adobe.com/hg/tamarin-redux-serrano/rev/3b6a480af8d7

There is a potential problem here, because the m_allocs array is actually made up of FixedAllocSafe objects, and the declaraction+definition of FixedAllocSafe::QueryOwnsObject remains under the DEBUG guard.

It is currently the responsibility of the client of FixedAllocSafe to not abuse the fact that FixedAllocSafe publicly inherits from FixedAlloc.  In this case, I hypothesize that such abuse is leaking through in release builds, the FixedAllocSafe is inheriting the FixedAlloc implementation of QueryOwnsObject.

If my hypothesis is true, then the new calls to QueryOwnsObject from FixedMalloc::FindBeginningAndSize are not grabbing the lock that protects each of the FixedAllocSafe instances before performing the query.  This is presents a potential thread hazard. 

The quickest fix is probably to remove the debug guards around the declaration and definition of FixedAllocSafe::QueryOwnsObject.

(Also, check if private-inheritance between FixedAllocSafe and FixedAlloc would have caught this.  Its possible it wouldn't, since FriedAllocSafe already has a friend-relationship with all of its clients, namely FixedAlloc and FixedMalloc.)

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #0)
> (Also, check if private-inheritance between FixedAllocSafe and FixedAlloc
> would have caught this.  Its possible it wouldn't, since FriedAllocSafe
> already has a friend-relationship with all of its clients, namely FixedAlloc
> and FixedMalloc.)

As predicted, private inheritance would not have caught this case.

(But that doesn't mean private inheritance is a bad idea for expressing the relationship here.  Some using declarations in FixedAllocSafe will suffice for exposing the FixedAlloc methods that do get called from non-friended clients.)

Comment 2

[Tamarin Bot]

changeset: 6554:fba2c2fe3ff1
user:      Felix S Klock II <[e-mail]>
summary:   Bug 683630: Unconditionally define FixedAllocSafe::QueryOwnsObject (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/fba2c2fe3ff1

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #0)
> The new calls to QueryOwnsObject from
> FixedMalloc::FindBeginningAndSize are not grabbing the lock that protects
> each of the FixedAllocSafe instances before performing the query.  This is
> presents a potential thread hazard. 

(Its hard to write a test-case for this directly.  Maybe a combination of a dynamic instrumentation tool like valgrind and an appropriate test case could illustrate the problem.  I'm not sure.)