Neil wrote, On 2009-03-29 17:01: > Firefox may want to switch to using the jemalloc allocator on the Mac.
Well, I hope it won't mean for the Mac what it has meant on Windows. On Windows, the JEMalloc code can ONLY be built with the non-free "professional" version of Microsoft Visual Studio, and CANNOT be built with the free version. Consequently, developers who use the free version must build EVERYTHING themselves, and cannot mix and match libraries they build with libraries they download from Mozilla's site. This has greatly increased the perception, among many developers who develop for Windows, that Firefox has effectively reverted to being MoCo's corporate product, and not quite the truly open product that it once was, and still claims to be. It's open, but its components cannot be mixed and matched with builds from other builders, even for the same platform. > As I understand it, this is tricky because you can't globally replace > malloc/free. While in theory it would be possible to replace NS_Free > and/or PR_Free, this runs into problems when the code doesn't match its > allocators. Right. > In particular, I am counting NS_Free, PR_Free, PL_strfree and free as > separate deallocators, Oh, it's worse than that. It you link a shared library or executable with the "static" version of the c run time on Windows, then that shared library/executable has its own heap. It's "free" is a different deallocator than the "free" in other shared libs. So, unless you're careful to completely avoid linking with static C run time libraries, every library's "free" is a separate deallocator. This wouldn't be so bad if everyone avoided using static c RTL, but Mozilla actually recommends that people DO use the static RTL, to work around the afore- mentioned problem with JEMalloc on Windows. :( > which might strike you as a bit harsh, as you probably only need to > care about NS_Free. That's an interesting statement. It suggests some mismatch between the people you intended to read this message, and the people who actually read the dev-tech-crypto mailing list. Developers of NSS and NSPR _NEVER_ use NS_Free and never care about it. They all hang out in dev-tech-crypto. Developers of PSM (which is part of Necko or Gecko, I think) care about all of those allocators you mentioned. They occasionally (but somewhat rarely, I think) hang out in dev-tech-crypto. I don't know where else they hang out. Developers of mail/news are yet a third breed. :) They don't hang out in dev-tech-crypto at all, AFAIK. (sniffle) > Note that nsXPIDLCString and .Adopt() counts as an NS_Free, while > nsCRT::strdup counts as a PR_Malloc, That sounds confusing. > and I'm currently lumping PR_smprintf_free in with free, and > PORT_Free with whatever it calls. Hmm. All the PR_ functions use free, which is the free for NSPR's heap. I think of all the PR_ functions as having their own heap, for which the free function (as seen by code outside of NSPR) is PR_Free. So, effectively, PR_smprintf_free is the same as PR_Free, both using NSPR's heap. PORT_Free is a wrapper for PR_Free. NSS tries pretty hard to only use NSPR's allocator for allocation of any objects whose addresses NSS may give out to its callers. NSS may occasionally use malloc/calloc/free for data items that are always purely private to NSS, never being passed to other libraries to (potentially) be freed by those other libraries. If you find a place where NSS allocates memory using a non-NSPR allocator and passes that memory to code outside of NSS to be freed, that's bug. > I instrumented the code and I ran all the mochitests (which include > some SSL) and I could only find a few (<20) allocator mismatches in > Gecko/Toolkit. By comparison, there are a horrifying number of allocator > mismatches in mailnews (nsDirPrefs.cpp on its own manages to use one > deallocator for a variable allocated by one of three allocators) and > several in libjar, narrowly relegating you to third in a hypothetical > "hall of shame". (I haven't filed bugs on mailnews or libjar yet either.) I wonder who is the "you" relegated to third place. NSS? PSM? mailnews? Which libjar? There are two. NSS has one used in NSS utility programs, and Firefox has one. Firefox doesn't use NSS's and NSS utilities don't use Firefox's. See http://mxr.mozilla.org/mozilla/find?string=jar%2F&tree=mozilla > Using the above criteria, I found the following allocator mismatches > under security/* OK, so, under security/* you have PSM and NSS. Different groups of developers with different constraints. > nsNSSCallbacks.cpp: HandshakeCallback calls Adopt(cipherName) instead of > Assign and PORT_Free. In fact, it can leak cipherName. > > nsNSSCertificate.cpp: several times it Adopt()s the result of > CERT_Hexify, when it should use PORT_Free. > > nsNSSIOLayer.cpp: both adopts the result of and allocates an outparam > using nsCRT::strdup, when both times it should use NS_strdup, while > SSL_RevealURL allocates with PL_strdup but charCleaner uses PR_FREEIF. > > nsSSLStatus.cpp: allocates an outparam using PL_strdup, when it should > use ToNewCString. OK, the above issues are all PSM issues. Bugs for those would be filed against product: Core, component: Security/PSM. > pk12util.c: uses PR_Free for a string allocated with malloc, That's a bug in an NSS utility program. The NSS team will definitely be interested in fixing that. Please file an NSS bug. But know that that won't have any effect on Firefox. > and PORT_ZFree for a string allocated with PL_strdup. PORT_ZFree is a wrapper for PR_Free. The question is: is it correct to allocate with PL_strdup (part of NSPR) and free with PL_Free (also part of NSPR, but part of a separate NSPR shared library) ? Today, the answer is almost certainly "No, it's not correct". I wonder if we could change that, so that NSPR effectively has just one allocator (that is, on heap) used by both PL_ and PR_ functions, or if that would break the world as we know it. Whether right or wrong, good or bad, NSS intentionally confuses PR_ and PL_ having #defines that give PR_ names to the PL_ functions. That's probably hurting this. > Would you be interested in patches for some or all of these? Who would > want to review them? Definitely yes for any bugs in NSS or NSPR. You can ask me to review all the NSS and NSPR bug patches. I may reassign that task to others for some bugs. For PSM, the developers are Kai Engert and Honza Bombas. Again, I'd say file bugs, and ask one of them to review. You might write directly to them for their suggestions on reviewer selection for PSM bugs. I can't advise you about mailnews. By the way, what tool do you use to find these mismatches of allocators and deallocators? I think those issues haven't gotten as much attention as they deserve simply because we're not aware of tools that make it easy. Thanks for researching this, Neil. -- dev-tech-crypto mailing list dev-tech-crypto@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-tech-crypto
