Closed Bug 338688 Opened 18 years ago Closed 17 years ago

NSS allocation functions don't always set SEC_ERROR_NO_MEMORY


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: nelson, Assigned: biswatosh2001)



(1 file, 4 obsolete files)

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

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.
Priority: -- → P2
  I erroneously wrote: "the global SSL allocation failure counter".
I should have written: "the global NSS allocation failure counter".
Retargetting all P2s to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3
Target Milestone: 3.11.3 → ---
Assignee: nobody → biswatosh.chakraborty
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?
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.
Attachment #271208 - Attachment is obsolete: true
Attachment #272463 - Flags: review?(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.
Attachment #272463 - Flags: review?(neil.williams) → review-
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).  
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).
Attachment #272463 - Attachment is obsolete: true
Attachment #272652 - Flags: review?(nelson)
Attachment #272652 - Attachment is obsolete: true
Attachment #272652 - Flags: review?(nelson)
Attached patch For review. (obsolete) — Splinter Review
Sorry for giving a wrong patch. I request this patch for review. Remarks on this patch given in the previous comment.
Attachment #272654 - Flags: review?
Attachment #272654 - Flags: review? → review?(nelson)
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).
Attachment #272654 - Flags: review?(nelson) → review-
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.
Attachment #272654 - Attachment is obsolete: true
Attachment #273389 - Flags: review?
Attachment #273389 - Flags: review? → review?(nelson)
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 ?
Attachment #273389 - Flags: review?(nelson) → review+
What would you suggest for the target version Nelson ?
I suggest 3.11.8
Target Milestone: --- → 3.11.8
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

Checking in nssilock.c;
/cvsroot/mozilla/security/nss/lib/util/nssilock.c,v  <--  nssilock.c
new revision: 1.3; previous revision: 1.2
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.
Attachment #273389 - Flags: superreview?(rrelyea)
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).

Attachment #273389 - Flags: superreview?(rrelyea) → superreview+
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:; previous revision: 1.6

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:; previous revision: 1.2

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.
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.