53427 - PORT_FreeArena NEVER zeros memory before freeing it

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

PORT_FreeArena's second argument, "zero" (as in zero it before 
freeing it), does nothing.  So, despite appearances, none of 
the calls shown below actually zeros the memory.  
I consider this a gaping security flaw in NSS.

./crmf/cmmfrec.c:        PORT_FreeArena(inKeyRecRep->poolp, PR_TRUE);
./crmf/crmfreq.c:           PORT_FreeArena(inCertReq->poolp, PR_TRUE);
./crmf/crmfreq.c:    PORT_FreeArena(inCertReqMsg->poolp, PR_TRUE);
./crmf/crmfreq.c:        PORT_FreeArena(inCertReqMsgs->poolp, PR_TRUE);
./crmf/respcmn.c:        PORT_FreeArena(inCertRepContent->poolp, PR_TRUE);
./crypto/dhkg.c:      PORT_FreeArena(arena, PR_TRUE);
./crypto/dhkg.c:        PORT_FreeArena(context->privateKey->arena, PR_TRUE);
./crypto/dsakg.c:       PORT_FreeArena(arena, PR_TRUE);
./crypto/rsakeygn.c:    PORT_FreeArena(arena, PR_TRUE);
./cryptohi/seckey.c:        PORT_FreeArena(privk->arena, PR_TRUE);
./cryptohi/seckey.c:            PORT_FreeArena(poolp, PR_TRUE);
./cryptohi/seckey.c:            PORT_FreeArena(poolp, PR_TRUE);
./fortcrypt/swfort/nslib.c:    PORT_FreeArena(privKey->params.arena, PR_TRUE);
./fortcrypt/swfort/swflib.c:    PORT_FreeArena(privKey->params.arena, PR_TRUE);
./fortcrypt/swfort/swfparse.c:    if (arena) PORT_FreeArena(arena,PR_TRUE);
./fortcrypt/swfort/swfutl.c:    PORT_FreeArena(poolp, PR_TRUE);
./pk11wrap/pk11skey.c:  PORT_FreeArena(arena, PR_TRUE);
./pk11wrap/pk11skey.c:     PORT_FreeArena(arena, PR_TRUE);
./pk11wrap/pk11skey.c:      PORT_FreeArena(arena, PR_TRUE);
./pkcs12/p12creat.c:    PORT_FreeArena(poolp, PR_TRUE);
./pkcs12/p12creat.c:    PORT_FreeArena(pfx->poolp, PR_TRUE);
./pkcs12/p12d.c:    PORT_FreeArena(arena, PR_TRUE);
./pkcs12/p12d.c:        PORT_FreeArena(p12dcx->arena, PR_TRUE);
./pkcs12/p12d.c:        PORT_FreeArena(arena, PR_TRUE);
./pkcs12/p12dec.c:          PORT_FreeArena(pfx->poolp, PR_TRUE);
./pkcs12/p12e.c:        PORT_FreeArena(arena, PR_TRUE);
./pkcs12/p12e.c:    PORT_FreeArena(p12ecx->arena, PR_TRUE);
./pkcs12/p12exp.c:      PORT_FreeArena(permArena, PR_TRUE);
./pkcs12/p12exp.c:    PORT_FreeArena(arena, PR_TRUE);
./pkcs12/p12exp.c:    PORT_FreeArena(poolp, PR_TRUE);
./pkcs12/p12exp.c:          PORT_FreeArena(safe->poolp, PR_TRUE);
./pkcs12/p12exp.c:          PORT_FreeArena(safe->poolp, PR_TRUE);
./pkcs12/p12local.c:    PORT_FreeArena(poolp, PR_TRUE);
./pkcs12/p12local.c:    PORT_FreeArena(poolp, PR_TRUE);
./pkcs12/p12local.c:    PORT_FreeArena(temparena, PR_TRUE);
./pkcs12/p12local.c:    PORT_FreeArena(temparena, PR_TRUE);
./pkcs12/p12local.c:    PORT_FreeArena(temparena, PR_TRUE);
./softoken/fipstest.c:        PORT_FreeArena( rsa_public_arena, PR_TRUE );
./softoken/fipstest.c:    PORT_FreeArena(dsa_private_key->params.arena, PR_TRUE)
;
./softoken/keydb.c:     PORT_FreeArena(poolp, PR_TRUE);\
./softoken/keydb.c:     PORT_FreeArena(permarena, PR_TRUE);
./softoken/keydb.c:     PORT_FreeArena(temparena, PR_TRUE);
./softoken/keydb.c:         PORT_FreeArena(permarena, PR_TRUE);
./softoken/keydb.c:     PORT_FreeArena(temparena, PR_TRUE);
./softoken/keydb.c:     PORT_FreeArena(temparena, PR_TRUE);
./softoken/keydb.c:         PORT_FreeArena(permarena, PR_TRUE);
./softoken/keydb.c:    PORT_FreeArena(temparena, PR_TRUE);
./softoken/keydb.c:     PORT_FreeArena(arena, PR_TRUE);
./softoken/lowkey.c:    PORT_FreeArena(privk->arena, PR_TRUE);
./softoken/lowkey.c:    PORT_FreeArena(poolp, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(arena, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(key->params.arena, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(arena, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(key->params.arena, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(rsaPriv->arena, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(dsaPriv->params.arena, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(dhPriv->arena, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(arena, PR_TRUE);
./softoken/pkcs11c.c:   PORT_FreeArena(arena, PR_TRUE);
./softoken/secpkcs5.c:  PORT_FreeArena(poolp, PR_TRUE);
./softoken/secpkcs5.c:  PORT_FreeArena(poolp, PR_TRUE);
./softoken/secpkcs5.c:  PORT_FreeArena(poolp, PR_TRUE);
./softoken/secpkcs5.c:  PORT_FreeArena(pbe_param->poolp, PR_TRUE);
./softoken/secpkcs5.c:  PORT_FreeArena(poolp, PR_TRUE);
./softoken/secpkcs5.c:  PORT_FreeArena(arena, PR_TRUE);
./softoken/secpkcs5.c:  PORT_FreeArena(pbeCtxt->arena, PR_TRUE);
./util/secdig.c:        PORT_FreeArena(arena, PR_TRUE);

Comment 1

[Wan-Teh Chang]

Reassigned to larryh.  Target NSS 3.3.

Comment 2

[larryh (gone)]

The arena library in NSPR never provided for a "clear on release" option.
Thanks for your suggestion. ... When did you say this was important?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Wan-teh has set the target to 3.3.  I think the intent is to 
prevent work on this feature from reducing the effort on 3.2,
the performance release.

Comment 4

[larryh (gone)]

OK NSS 3.3. When, on the calendar, is that?

Comment 5

[larryh (gone)]

This will require an API change or the addition of a new function to implement 
the capability.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Larry,  I want you to reconsider doing this in NSS 3.3.

This is a security bug in NSS (not sure why it was marked
as an enhancement).  The function is not doing what it was designed
to do.  

Ideally, the clearing would be done in the PL_Arena code, but
the work could also be done in PORT_FreeArena itself.  
That's not as clean, but if the choices are 
do it uncleanly or don't do it at all, I'd vote for 
do it uncleanly.

Comment 7

[larryh (gone)]

Reassign to wtc.

Comment 8

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I want to fix this bug.  The bug really should be fixed in NSPR.
But Fixing bugs in NSPR is off limits to me (and just about everyone, AFAICT).
So, my choices are:
a) wait perpetually for someone to fix NSPR, or
b) fix this in NSS by making NSS use private data structures of the PLArenaPool.

I've waited long enough for the former.  
So it's time for me to attempt the latter.

Comment 10

[Wan-Teh Chang]

Nelson, I don't have time to fix NSPR bugs now.
I don't even have time to fix NSS bugs that are
unrelated to FIPS.  You are welcome to fix this
bug by modifying NSPR. Darin Fisher can review
your NSPR patches.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - depends on NSPR patch

I will ask for review when the NSPR patch is already ready for review.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 231875 [details] [diff] [review]
patch v1 - depends on NSPR patch

Please review this patch together with the patch for bug 347106.

Comment 13

[Wan-Teh Chang]

Attachment: alternative patch (for NSS_3_11_BRANCH)

As I noted in bug 347106 comment 11, it is fine to
fix this bug entirely in NSS.  This patch does that.
Note that it only zeroizes the memory that has been
used (from a->base to a->avail) rather than the whole
arena (from a->base to a->limit).  Also it doesn't
reset the a->avail pointer because it's not necessary.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 233318 [details] [diff] [review]
alternative patch (for NSS_3_11_BRANCH)

You beat me to it, Wan-Teh.

Comment 15

[Wan-Teh Chang]

I checked in the "alternative patch (for NSS_3_11_BRANCH)"
on the NSS trunk (NSS 3.12) first.

Checking in secport.c;
/cvsroot/mozilla/security/nss/lib/util/secport.c,v  <--  secport.c
new revision: 1.19; previous revision: 1.18
done

Julien, please review that patch so that I can check it in
on the NSS_3_11_BRANCH.  Thanks.

Comment 16

[Julien Pierre]

Comment on attachment 233318 [details] [diff] [review]
alternative patch (for NSS_3_11_BRANCH)

I'm afraid I don't think this patch does the job. It zeroes only memory from base to avail . It should zero memory from base to limit . That is because the arena could have been marked and released, and thus there could still be sensitive data between avail and limit that needs to be zero'ed .

Comment 17

[Wan-Teh Chang]

Attachment: alternative patch v2 (for NSS_3_11_BRANCH)

Good catch, Julien.  This patch zeroizes memory from base to limit.
Note that it doesn't reset the avail pointer.

I studied the PLArena code more closely and found that the problem
with arena mark and release and memory zeroization is more complicated.
PL_ARENA_RELEASE may return several PLArena's to the arena freelist
(this happens when PL_ARENA_RELEASE calls the PL_ArenaRelease function).
Since NSS has no control over those PLArena's, it is possible that
they contain sensitive information.

For the softoken, it seems that the only functions that may release 
arena pool memory containing any information are the SEC_ASN1Decode*
functions fail (see sec_asn1d_push_state and sec_asn1d_free_child).
All the other functions used by the softoken only release arena pool
memory that is still blank (i.e., just allocated).

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

When we "release' a mark, any arenas in the pool that become completely
unused are freed (returned to the free list) at that point.  
Nothing we can do at arenapool destruction time will help with arenas 
that were freed by being released.  So, we better not be using mark and 
release on arenas that contain private keys.

Comment 19

[Julien Pierre]

Comment on attachment 233644 [details] [diff] [review]
alternative patch v2 (for NSS_3_11_BRANCH)

This is better.
Ideally we could detect the use of mark/release and only zero the unused portion if needed. This may require an extra flag in the PLArena structure. But this patch is fine.

Re: comments 17 and 18 :

I think we could deal with the arena recycling by zero'ing the content of the arena when doing a PL_ARENA_RELEASE . That might be expensive to do unconditionally, however.

But hopefully our use of Mark and Release is uncommon enough and it won't become a performance problem.

If it does, perhaps we can mark the arenas as sensitive somehow so as to designate which ones NSPR would need to zero (or not zero) upon releasing to the freelist.

Comment 20

[Wan-Teh Chang]

Attachment: One way to ensure that the softoken never releases arenas containing sensitive information

For FIPS validation of the softoken, I need to make sure that
the softoken doesn't release arenas containing sensitive information.

I first used LXR to examine every call to PORT_ArenaRelease inside
the source files that make up the softoken (lib/{util,freebl/softoken}).
I concluded that the SEC_ASN1De* functions are the only ones that may
release arenas that contain information (which may or may not be
sensitive).

Then I searched for "SEC_ASN1De*".  For each instance, we have two
options:
1. Replace the SEC_ASN1De* call with a SEC_QuickDER* call because
QuickDER doesn't mark and release arenas.
2. If the information in the arenas is not sensitive, call PORT_FreeArena
with zero=PR_FALSE.

It turns out that all the SEC_ASN1De* calls in the softoken operate
on non-sensitive or encrypted sensitive information.  So I decided
to use Option 2 for all of them.  Here are the comments on the patch.

1. softoken/keydb.c: remove dead code.
2. softoken/lowpbe.c: the NSSPKCS5PBEParameter struct doesn't contain
any sensitive information.
3. softoken/pkcs11c.c: the decoded output contains encrypted private
key info.
4. util/secdig.c: digest info is not sensitive info.

Comment 21

[Julien Pierre]

Wan-Teh,

When I wrote QuickDER, I tried to replace the use of the legacy decoder with QuickDER in every part of NSS where it was possible.  See bug 178895 . I believe the remaining places that have SEC_ASN1DecodeItem use it because they need to be able to accept BER . So replacing the decoder with QuickDER probably isn't going to be an option where you want to do it.

Comment 22

[Wan-Teh Chang]

Attachment: Another way: use QuickDER in the softoken

This patch replaces all calls to SEC_ASN1DecodeItem in the softoken
with SEC_QuickDERDecodeItem.  Julien, can you tell if any of these
must use SEC_ASN1DecodeItem?

Comment 23

[Julien Pierre]

Comment on attachment 233665 [details] [diff] [review]
Another way: use QuickDER in the softoken

Wan-Teh,

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=178895#c14 , I would say at least the parts of your patch that have to do with PKCS#5 and PKCS#12 in lowpbe.c require SEC_ASN1DecodeItem .

I think the private key info in pkcs11c.c, if it is PKCS#8, is also BER-encoded, so the same problem exists.

I think the change for util/secdig.c is possible . See https://bugzilla.mozilla.org/show_bug.cgi?id=178897#c5 . This is probably the possible change that was mentioned.

But your patch may not be doing it right . Remember that even though it has the same function signature, SEC_QuickDERDecodeItem has a different memory model than SEC_ASN1DecodeItem. QuickDER fills a structure with pointers into the input data. So, you need to copy the didata input data to the arena and then decode the copy.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

zeroing each arena in the pool from base to limit does not ensure that all 
arenas that contained sensitive (private key) info get zeroed.  This is because
release will return to the arena free list any arenas that are completely 
unused as a result of being released.  

Sounds like the fix will necessitate changing the ASN1 decoder to not release
any more, so that all the memory it allocates will still be available to be 
zeroed before being freed.  

Comment 25

[Wan-Teh Chang]

Bob,

Sorry to cc you in the middle of a long discussion.  You can
start from comment 17.  Right now the remaining work (for the
softoken to meet the FIPS key zeroization requirment) is to
ensure that the softoken doesn't call SEC_ASN1DecodeItem on
*plaintext* private keys, secret keys, passwords, or any other
security related information whose disclosure or modification
can compromise the security of the softoken.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

In reply to comments 22 and 23:
PKS#11 doesn't require keys to be DER encoded.  Only BER is required.
So we cannot use QuickDERDecoder on them.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 233644 [details] [diff] [review]
alternative patch v2 (for NSS_3_11_BRANCH)

My comment 24 was meant to be a review comment for this patch.

Comment 28

[Wan-Teh Chang]

Attachment: Pass the correct 'zero' argument to PORT_FreeArena after calls to SEC_ASN1DecodeItem in the softoken

This patch is a step towards solving the problem for the softoken.
It makes sure that after all the SEC_ASN1DecodeItem calls in the
softoken, we pass the correct 'zero' argument to PORT_FreeArena.

1. softoken/keydb.c: remove dead code.
2. softoken/lowpbe.c: is NSSPKCS5PBEParameter all public info?
3. softoken/pkcs11c.c: we should pass zero=PR_FALSE to the first
PORT_FreeArena call and zero=PR_TRUE to the second PORT_FreeArena
call because the private key is unencrypted (it's decrypted in
NSC_UnwrapKey).
4. util/secdig.c: DigestInfo is public info.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 233659 [details] [diff] [review]
One way to ensure that the softoken never releases arenas containing sensitive information

The change to softoken/keydb.c, removing dead code, seems fine. 
r+ for that part.

I don't know if the pbe_param can contain sensitive info or not,
so I withold review on the change to nsspkcs5_DestroyPBEParameter.
If the param is not sensitive, then r+

In sftk_unwrapPrivateKey, the proposed change to not zero the arena
if PORT_ArenaZAlloc fails is fine, because nothing has been put 
into the arenapool that might be sensitive at that point.  
But just 6 more lines down, if/when the decode has failed, the 
arenapool should be zeroed, including any parts that were released. :-/

I think DigestInfos don't contain sensitive info.  r+ for that part.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 233690 [details] [diff] [review]
Pass the correct 'zero' argument to PORT_FreeArena after calls to SEC_ASN1DecodeItem in the softoken

I think this patch is necessary, but perhaps not sufficient.  If the memory allocated in SEC_ASN1DecodeItem has been released before it returns, it may already have been freed.

Comment 31

[Robert Relyea]

Comment on attachment 233690 [details] [diff] [review]
Pass the correct 'zero' argument to PORT_FreeArena after calls to SEC_ASN1DecodeItem in the softoken

r+

I've verified pkcs5pbe contains no sensitive information.

bob

Comment 32

[Wan-Teh Chang]

Comment on attachment 233690 [details] [diff] [review]
Pass the correct 'zero' argument to PORT_FreeArena after calls to SEC_ASN1DecodeItem in the softoken

I checked in this patch on the NSS trunk (3.12) and the
NSS_3_11_BRANCH (3.11.3).

Comment 33

[Wan-Teh Chang]

Attachment: Make the first arena large enough to decode NSSLOWKEYPrivateKeyInfo

If we really can't use the QuickDER decoder to decode NSSLOWKEYPrivateKeyInfo,
then I can think of only three solutions.

1. Change secasn1d.c to not mark and release arenas.
2. Pass NULL as the 'poolp' argument to this SEC_ASN1DecodeItem call.
3. Make the first arena in the arena pool large enough so that this
SEC_ASN1DecodeItem call doesn't need to create the second arena when
decoding NSSLOWKEYPrivateKeyInfo.  This way, if the decoding fails,
it won't release any arena to the arena freelist.

This patch implements solution 3.  The size of the first arena is
just an estimate.  I also reset the arena size to its normal value
after we have decoded NSSLOWKEYPrivateKeyInfo.  If you find that
a violation of API abstraction, please let me know.

Comment 34

[Wan-Teh Chang]

Comment on attachment 233708 [details] [diff] [review]
Make the first arena large enough to decode NSSLOWKEYPrivateKeyInfo

I found that solution 2 and solution 3 (this patch) in
comment 33 don't work because SEC_ASN1DecodeItem marks
and releases its own arena pool (called 'our_pool' in
the source code).

It's not clear by my inspection of secasn1d.c whether
the decoder ever copies user data into 'our_pool'.  I hope
you can help me determine that.  If we aren't sure, the only
safe solution is solution 1.

Comment 35

[Julien Pierre]

Wan-Teh,

It would be preferrable to implement solution 1 anyway. Marking and releasing is not safe in multithreaded programs. In the general case, the decoder can never be 100% sure that the arena that was passed in isn't known to other threads. I had originally implemented QuickDER using mark and release as well, but had to back off. Of course, QuickDER allocates very little memory in general, so the memory growth for failure cases wasn't that bad. But SEC_ASN1DecodeItem copies each individual decoded item into the arena, one at a time, and it would be very bad if it couldn't release that memory. I think that answers the question about whether there is sensitive data copied into the arena - most definitely yes.

I think there is an approach we can use to get away from using Mark and Release, but it involves enhancements to the arena code, either in NSPR or NSS . We can have the decoder work on a local, temporary arena, and once it is successful, merge that arena to the incoming arena that was passed in at the top-level.
Bug 348291 tracks the RFE to add the capability to merge arenas.

Comment 36

[Wan-Teh Chang]

Attachment: Add PORT_ArenaZRelease and change secasn1d.c to zero our_pool before freeing/releasing it

Before I describe the patch, I have a PKCS #11 question for all of you.
PKCS #11 v2.20 Section 12.6 says that not only PrivateKeyInfo but also
the private keys within it are BER-encoded.  But the sftk_unwrapPrivateKey
function uses the QuickDER decoder to decode the private keys within
PrivateKeyInfo.  Isn't that wrong?

Now the patch.  I considered several solutions, including a variant of
Julien's proposal of replacing arena mark and release with a temporary
arena.  In the end, the solution I like the most is to allow
PORT_ArenaRelease to zeroize the arena memory before releasing it.
This parallels the 'zero' argument to the PORT_FreeArena function.
There are two ways to design the new API.  The first approach is to
add a new function, say PORT_ArenaRelease2, that takes a PRBool zero
argument.  The second approach, implemented by this patch, is to add
a new function PORT_ArenaZRelease that zeroizes arena memory.  This
new function explains the changes to lib/util/secport.{h,c}.

Note that I zeroize from 'mark' to a->avail, rather than a->limit.
Also note that with this patch applied, PORT_FreeArena only need
to zeroize from a->base to a->avail (which is what my very first
patch does).

The slow and fast paths in port_ArenaZeroAfterMark are modeled after
the PL_ARENA_RELEASE macro.  (The function could just have the slow path.)

Finally, I changed lib/util/secasn1d.c to zeroize 'our_pool' before
releasing/freeing it.  When you review this change, you need to
also verify that none of the other PORT_ArenaRelease and PORT_FreeArena
calls in that file need to be changed.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

> But the sftk_unwrapPrivateKey function uses the QuickDER decoder to 
> decode the private keys within PrivateKeyInfo.  Isn't that wrong?

Yes, IMO, it is wrong.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 233816 [details] [diff] [review]
Add PORT_ArenaZRelease and change secasn1d.c to zero our_pool before freeing/releasing it

I wish this patch also made public a function with the same signature as 
port_ArenaRelease.  Otherwise, I fear that down the road we will see 
new code that looks like:
    if (zero_it)
        PORT_ArenaZRelease (our_pool, our_mark);
else
	PORT_ArenaRelease (our_pool, our_mark);
and I'd prefer to see
    PORT_ArenaReleaseMaybeZ(our_pool, our_mark, zero_it);

But this is a mere wish list, not a must have.

Comment 39

[Julien Pierre]

I note that patch 233816 (Add PORT_ArenaZRelease) does not change PORT_FreeArena to do any zero'ing, unlike patch 233644 (alternative patch v2) .

Both changes need to be made to resolve this bug. nelson gave an r- to the first patch because it didn't contain the second code change; but the second patch doesn't contain the first change.   A combined patch might be better to clear things up.

patch 233690 (Pass the correct zero argument) is of course also needed for the FIPS work, but that is outside the original scope of this bug.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Julien, it appears to me that attachment 233318 [details] [diff] [review] 
"alternative patch (for NSS_3_11_BRANCH)" was checked in.
I don't know why it's now marked obsolete.

Comment 41

[Julien Pierre]

Nelson,

Patch 233318 ("Alternative patch") got checked in to the tip after your r+. Wan-Teh asked me for a 2nd review for the branch, but I gave it an sr- yesterday. That's probably why it has been marked obsolete.

Wan-Teh then produced patch 233644 ("Alternative patch 2") - to which you gave r- for incompleteness. This code change is still needed, but doesn't appear in any other patch that got 2 reviews.

Comment 42

[Wan-Teh Chang]

Attachment: Combined patch - doesn't depend on NSPR patch (checked in)

OK, here is the combined patch to fix PORT_FreeArena and
add PORT_ArenaZRelease.

Note that PORT_FreeArena only zeroizes from a->base to a->avail.

Comment 43

[Julien Pierre]

Comment on attachment 233881 [details] [diff] [review]
Combined patch - doesn't depend on NSPR patch (checked in)

Looks fine for the branch checkin. We don't need to zeroize to from base to limit as in alternative patch v2, since releasing now takes care of zeroing the freed memory. And when marking but not releasing, zeroing from base to avail zeroes all buffers used by the application.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 233881 [details] [diff] [review]
Combined patch - doesn't depend on NSPR patch (checked in)

r=nelson

Comment 45

[Wan-Teh Chang]

I checked in the combined patch on the NSS trunk (NSS 3.12)
and NSS_3_11_BRANCH (NSS 3.11.3).  As an NSS bug, this bug
is fixed.  We should move the function port_ArenaZeroAfterMark
to NSPR.  Let's do that in the NSPR bug 347106.

Comment 46

[Wan-Teh Chang]

Re: Nelson's suggested API change in comment 38

I searched for all calls to PORT_FreeArena and found that
we always pass constants as the 'zero' argument:
http://lxr.mozilla.org/security/search?string=PORT_FreeArena

This result would argue for the current API (PORT_ArenaZRelease).
On the other hand, I actually liked Nelson's suggestion better.
The reason I didn't implement it was that I couldn't come up
with a good name for the new arena release function with a 'zero'
argument.  (It's easy to pick a good name for the current API:
PORT_ArenaZRelease was modeled after PORT_ZFree, PORT_ZAlloc,
PORT_ArenaZAlloc, etc.)