338688 - NSS allocation functions don't always set SEC_ERROR_NO_MEMORY

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

All the "PORT_" memory or object allocation functions, including those that 
allocate new arenas, are supposed to ENSURE that, if they return NULL, 
they have set the error code SEC_ERROR_NO_MEMORY, and they have incremented
the global SSL allocation failure counter, named port_allocFailures.

This rule also applies to NSS's PZ_ functions that allocate locks and related
objects.

But they don't all obey this rule.  
In particular, I found that PORT_NewArena and PZ_NewLock do not.

I also found that the NSPR function PR_NEWZAP and PR_NewLock do not always
set PR_OUT_OF_MEMORY_ERROR for their memory allocation failures, but that
is the subject of a separate NSPR bug.

The global variable port_allocFailures should be atomically incremented,
but that is the subject of another bug.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Correction:  
  I erroneously wrote: "the global SSL allocation failure counter".
I should have written: "the global NSS allocation failure counter".

Comment 2

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 3

[Biswatosh Chakraborty]

Attachment: Not for review. It sets errors to SEC_ERROR_NO_MEMORY during memory failures

It touches 2 files, namely (1)nss/lib/ckfw/nsprstub.c and 
(2) nss/lib/util/nssilock.c.

In the first file, it does 
(a)++port_allocFailures; and (b)PORT_SetError(SEC_ERROR_NO_MEMORY) in many places.
In the second file, it sets PORT_SetError(SEC_ERROR_NO_MEMORY) only.
In some functions, it purposefully does not set the error to SEC_ERROR_NO_MEMORY
in case of memory failure because that function is calling another function
which actually faces this shortage of memory and itself sets the error to this.
Thus, should there be a requirement for the wrapper function to also set this error when the internal(i.e called) function does it?

Comment 4

[Biswatosh Chakraborty]

Attachment: For review. Same as previous attachment but with some indendation changes.

Read comment #3 for details.

No change is proposed in this patch for PORT_NewArena() because 
this function calls PORT_ZAlloc() and in PORT_ZAlloc(), the needed
change is proposed instead. That is, on getting a memory error, the error
is being set to SEC_ERROR_NO_MEMORY in PORT_ZAlloc(). This philosophy
is followed in case of other functions too.

Comment 5

[Neil Williams]

Comment on attachment 272463 [details] [diff] [review]
For review. Same as previous attachment but with some indendation changes.

The patch to nsprstub.c looks good. Couple of problems with nssilock.c:

nssILockInit() setting the return code before calling exit() won't do much--might as well leave it out.

pz_NewLock() looks like a bug after your else clause, lock->ltype shouldn't be executed when lock == NULL.

Please ask Wan-Teh for a review of your next patch. I am not familiar with these routines.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Neil's review comments in comment #5 were exactly right. 
The new else statement in pz_NewLock doesn't detect all the paths in 
which lock might be NULL, and doesn't prevent dereferencing a NULL 
lock pointer.  Notice that dereferencing that NULL lock pointer is a
pre-existing bug, but you should fix it while you're working on that code.

Neil did a fine job of reviewing that code, and IMO could review the 
next patch, too.  Or I could (for this bug).  

Comment 7

[Biswatosh Chakraborty]

Attachment: For review. nss/lib/util/nssilock.c is different from the earlier patch.

nss/lib/ckfw/nsprstub.c remains same as in the earlier patch(272463).
Modifies nssilock.c mainly. This tries to incorporate the suggestions
made on the previous patch (in Comment #5 and #6).

Comment 8

[Biswatosh Chakraborty]

Attachment: For review.

Sorry for giving a wrong patch. I request this patch for review. Remarks on this patch given in the previous comment.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272654 [details] [diff] [review]
For review. 

Getting closer, but still not quite right.

pz_NewLock and pz_NewMonitor are almost identical, except for
the structure type and the function called to allocate the
underlying type (e.g. PR_NewLock and PR_NewMonitor).
Yet the logic in the error paths for the two are quite different.
If the call to the NSPR function to allocate the underlying type fails,
one function sets an error code but falls through (does not return),
but the other returns without setting an error code.
Please make them consistent.  Set the error code in both, and 
consistently fall through, or consistently return.  (I suggest
falling through).

Comment 10

[Biswatosh Chakraborty]

Attachment: For review. Implemented suggestions made in Comment #9

Patch for nsprstub.c remains same as in the previous patch(272654).

Changes made in comparision to the previous patch(272654) for nssilock.c:

1)Functions pz_NewLock() and pz_NewMonitor() are now similar in case of setting
error and in falling through. See previous comment( #9).

2)pz_NewLock() sets lock as NULL after calling PR_DELETE(lock). This is
unnecessary as PR_DELETE itself frees the argument(here it is "lock") 
and sets it to NULL. Hence, "lock = NULL" line is removed in the patch.
This way, it is now same as pz_NewCondVar() and pz_NewMonitor().

3)Vtrace() is changed in two functions,namely pz_NewCondVar() and pz_NewMonitor(). The change is done because the second argument to Vtrace()
tries to access a member of a structure with the pointer to the structure.
This pointer could be a NULL and it could give a crash.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 273389 [details] [diff] [review]
For review.  Implemented suggestions made in Comment #9

r+  Looks good.

What is the target of this bug?  3.12? or 3.11.x ?

Comment 12

[Christophe Ravel]

What would you suggest for the target version Nelson ?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I suggest 3.11.8

Comment 14

[Biswatosh Chakraborty]

Commiting the files(nsprstub.c and nssilock.c) to the HEAD...


Checking in nsprstub.c;
/cvsroot/mozilla/security/nss/lib/ckfw/nsprstub.c,v  <--  nsprstub.c
new revision: 1.7; previous revision: 1.6
done

Checking in nssilock.c;
/cvsroot/mozilla/security/nss/lib/util/nssilock.c,v  <--  nssilock.c
new revision: 1.3; previous revision: 1.2
done

Comment 15

[Biswatosh Chakraborty]

Comment on attachment 273389 [details] [diff] [review]
For review.  Implemented suggestions made in Comment #9

The patch for the branch NSS_3_11_BRANCH and the Head are same because the functions for which changes have been proposed are similarly implemented in both.

Comment 16

[Robert Relyea]

Comment on attachment 273389 [details] [diff] [review]
For review.  Implemented suggestions made in Comment #9

r+ I don't know if that's all of them, but the changed functions are correct (and the similiar error approach makes reading and reviewing the code easier).

bob

Comment 17

[Biswatosh Chakraborty]

Commiting nsprstub.c into NSS_3_11_BRANCH

Checking in nsprstub.c;
/cvsroot/mozilla/security/nss/lib/ckfw/nsprstub.c,v  <--  nsprstub.c
new revision: 1.6.28.1; previous revision: 1.6
done


Commiting nssilock.c into NSS_3_11_BRANCH

Checking in nssilock.c;
/cvsroot/mozilla/security/nss/lib/util/nssilock.c,v  <--  nssilock.c
new revision: 1.2.28.1; previous revision: 1.2
done

Comment 18

[Biswatosh Chakraborty]

The patches were reviewed and super reviewed and now have been
commited both to the Head and to the NSS_3_11_BRANCH. 
Hence, closing this bug.