Closed Bug 338688 Opened 18 years ago Closed 16 years ago
NSS allocation functions don't always set SEC
_ERROR _NO _MEMORY
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.
Correction: 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 → ---
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.
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).
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 #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
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 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). bob
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: 220.127.116.11; 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: 18.104.22.168; previous revision: 1.2 done
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.