Closed
Bug 338688
Opened 19 years ago
Closed 17 years ago
NSS allocation functions don't always set SEC_ERROR_NO_MEMORY
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.8
People
(Reporter: nelson, Assigned: biswatosh2001)
Details
Attachments
(1 file, 4 obsolete files)
9.26 KB,
patch
|
nelson
:
review+
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
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.
Updated•19 years ago
|
Priority: -- → P2
Reporter | ||
Comment 1•19 years ago
|
||
Correction:
I erroneously wrote: "the global SSL allocation failure counter".
I should have written: "the global NSS allocation failure counter".
Reporter | ||
Updated•18 years ago
|
Target Milestone: 3.11.3 → ---
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → biswatosh.chakraborty
Assignee | ||
Comment 3•17 years ago
|
||
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?
Assignee | ||
Comment 4•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 5•17 years ago
|
||
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-
Reporter | ||
Comment 6•17 years ago
|
||
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).
Assignee | ||
Comment 7•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #272652 -
Attachment is obsolete: true
Attachment #272652 -
Flags: review?(nelson)
Assignee | ||
Comment 8•17 years ago
|
||
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?
Assignee | ||
Updated•17 years ago
|
Attachment #272654 -
Flags: review? → review?(nelson)
Reporter | ||
Comment 9•17 years ago
|
||
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-
Assignee | ||
Comment 10•17 years ago
|
||
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?
Assignee | ||
Updated•17 years ago
|
Attachment #273389 -
Flags: review? → review?(nelson)
Reporter | ||
Comment 11•17 years ago
|
||
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+
Comment 12•17 years ago
|
||
What would you suggest for the target version Nelson ?
Reporter | ||
Comment 13•17 years ago
|
||
I suggest 3.11.8
Assignee | ||
Updated•17 years ago
|
Target Milestone: --- → 3.11.8
Assignee | ||
Comment 14•17 years ago
|
||
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
Assignee | ||
Comment 15•17 years ago
|
||
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 16•17 years ago
|
||
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+
Assignee | ||
Comment 17•17 years ago
|
||
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
Assignee | ||
Comment 18•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•