On Aug 9, 10:48 pm, "L. David Baron" <[EMAIL PROTECTED]> wrote: > On Sunday 2007-07-22 04:40 -0400, Mike Connor wrote: > > > On 22-Jul-07, at 2:59 AM, Robert Sayre wrote: > > > That doesn't sound reasonable. We are going to accept a very large > > > body of code with known quality control problems. > > > If you have evidence that libpkix has known quality control problems, > > For what it's worth, I do. > > I just got trace-malloc working on Windows, so I was looking at the > list of what we leak when starting and shutting down Firefox. (It's > hard to do that on Linux because GNOME and GTK don't bother freeing > things on shutdown.) > > Currently, all we do with libpkix when starting and shutting down > the browser is initialize it. That is, we call > PKIX_Initialize:http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib... > This creates 7 hash tables and a lock. NSS doesn't currently call > PKIX_Shutdown. But if we had it wouldn't have helped us free the > memory allocated when creating these 7 hash tables and a lock. > > Why not? > > PKIX has its own object system. Every object has, among other > things, a reference count, a hash code, and a lock used to protect > the reference count and the hash > code:http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib... > (I'd note that using atomic operations for reference counts avoids > the rather significant size overhead of the lock. And I'm skeptical > that the average mutex needs to be put in a hash table.) > > So, for example, this lock that we create is created in > PKIX_PL_MonitorLock_Create:http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib... > which in turn calls > PKIX_PL_Object_Alloc:http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib... > which makes sure to allocate the object header (above) in addition > to the (one word) PKIX_PL_MonitorLockStruct. So this lock object is > an 8 word struct that owns a PRLock (to protect its reference count > and hash code) and a PRMonitor (for the locking exposed to the > caller), which are not themselves small or simple objects (a PRLock > is 132 bytes on Windows; a PRMonitor is three allocations, a 132 > byte PRLock, a 132 byte PRCondVar, and a 12 byte PRMonitor). > (PKIX_PL_Mutex_Create gives you something similar, except with two > PRLocks instead of a PRMonitor and a PRLock.) > > The hash tables, in turn, have their own PRLock to protect their > reference count (like all objects) and a table lock to protect the > table (an object created by PKIX_PL_Mutex_Create, which in turn has > two PRLocks, one to protect its reference count and one to expose to > the caller). > > When we initialize libpkix, we tell it to use arenas for allocation, > by passing PR_TRUE as the second parameter to > PKIX_Initialize:http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib... > This means, through a bit of indirection, that the PKIX reference > counting functions are > no-ops:http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib... > This means that we'll never free all the locks that we allocate, > something that I imagine could be a significant memory leak if we > did more than create 7 hash tables and a lock. (For just that, it's > 3312 bytes of leaked lock structures, excluding the > 1 PKIX_PL_MonitorLockStruct, 7 PKIX_PL_MutexStructs, and their > header (320 bytes) that were allocated in the arena.) > > It also means that we'll never return any of these arena-allocated > objects to a freelist to be reallocated. Not that we would return > them to a freelist if we removed the reference counting, > anyway:http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib... > > ( And while I'm in that file, I'd note that if we're using arenas, > then its version of memcpy returns an *allocation > error*:http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib... > . Not that memcpy actually allocates any memory.) > > -David > > -- > L. David Baron http://dbaron.org/ > Mozilla Corporation http://www.mozilla.com/ > > application_pgp-signature_part > 1KDownload
This message posted by dbaron seems to have been lost in the thread. I'm raising it to its own new thread header position. I didn't see it in David's post, but he mentioned to me that the arena- based allocation of objects with non-LIFO allocation patterns, without a freelist to recycle arena-allocated objects who die in non-LIFO order, seems to be a problem too. That is, without LIFO allocation order, this code will bloat its arenas with uncollected garbage, potentially badly. >From what I can see and what David demonstrates, libpkix has wrong- headedly cloned Java's object model into C, in the most inefficient, not-seen-in-a-JVM-since-1995, unjustified for C code, and manifestly bloaty and leaky manner possible. This is not code I want in Firefox 3. Something fairly radical needs to be done to fix this problem. Nothing, certainly not EV cert promises, justifies this quality problem. Followups to m.d.planning. /be _______________________________________________ dev-tech-crypto mailing list [email protected] https://lists.mozilla.org/listinfo/dev-tech-crypto
