Closed
Bug 1280844
Opened 8 years ago
Closed 8 years ago
Fix null-check-after-deref issues found in NSS with µchex
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(firefox50 affected)
RESOLVED
FIXED
3.26
Tracking | Status | |
---|---|---|
firefox50 | --- | affected |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(9 files)
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mt
:
review+
|
Details |
µchex is a static analysis tool described in this paper: How to Build Static Checking Systems Using Orders of Magnitude Less Code Fraser Brown, Andres Nötzli, Dawson Engler ASPLOS '16 I've been in contact with the authors and they've given me access to the result of an up-to-date run of the null-check-after-deref issues found in NSS. There are two categories of problem: - Redundant checks. It would be safe to leave these in the code, but I have removed them all. - Missing/bad checks. These are actively dangerous.
Assignee | ||
Comment 1•8 years ago
|
||
This patch: - Adds some missing null checks after allocations. - Moves some derefs of pointers after null checks of those same pointers. - Removes some redundant null checks for pointers that are guaranteed to be non-null. Review commit: https://reviewboard.mozilla.org/r/59582/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59582/
Attachment #8763420 -
Flags: review?(martin.thomson)
Attachment #8763421 -
Flags: review?(martin.thomson)
Attachment #8763422 -
Flags: review?(martin.thomson)
Attachment #8763423 -
Flags: review?(martin.thomson)
Attachment #8763424 -
Flags: review?(martin.thomson)
Attachment #8763425 -
Flags: review?(martin.thomson)
Attachment #8763426 -
Flags: review?(martin.thomson)
Attachment #8763427 -
Flags: review?(martin.thomson)
Attachment #8763428 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 2•8 years ago
|
||
The assert() assumes that |from| and |to| are non-null, which the subsequent check does not assume. It's also redundant w.r.t. the subsequent checks, which will cause informative termination on failure. It's also the only assert() in this file. Review commit: https://reviewboard.mozilla.org/r/59584/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59584/
Assignee | ||
Comment 3•8 years ago
|
||
This patch fixes things so that we don't deref file before checking it. Review commit: https://reviewboard.mozilla.org/r/59586/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59586/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59588/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59588/
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59590/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59590/
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59592/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59592/
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59594/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59594/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59596/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59596/
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59598/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59598/
Comment 10•8 years ago
|
||
Comment on attachment 8763422 [details] Bug 1280844 - Fix a null check in parse.c. . https://reviewboard.mozilla.org/r/59586/#review56682 Not sure if this is even compiled...
Attachment #8763422 -
Flags: review?(martin.thomson) → review+
Updated•8 years ago
|
Attachment #8763420 -
Flags: review?(martin.thomson) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8763420 [details] Bug 1280844 - Fix up null checks in cmd/. . https://reviewboard.mozilla.org/r/59582/#review56684 ::: security/nss/cmd/certcgi/certcgi.c (Diff revision 1) > - if (arena) { > - PORT_FreeArena(arena, PR_FALSE); > + PORT_FreeArena(arena, PR_FALSE); > - } The general pattern in NSS is to have a redundant check here, but I guess that blindly following the rules is not necessary. ::: security/nss/cmd/crlutil/crlgen.c:520 (Diff revision 1) > + extHandle = crlGenData->crlExtHandle; > rv = CERT_AddExtension(extHandle, SEC_OID_X509_CRL_NUMBER, &encodedItem, > (*dataArr[1] == '1') ? PR_TRUE : PR_FALSE, > PR_TRUE); Maybe just remove the temporary instead.
Updated•8 years ago
|
Attachment #8763421 -
Flags: review?(martin.thomson) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8763421 [details] Bug 1280844 - Remove useless assert() in pathsub.c. . https://reviewboard.mozilla.org/r/59584/#review56686
Comment 13•8 years ago
|
||
Comment on attachment 8763423 [details] Bug 1280844 - Remove some redundant null checks in lib/. . https://reviewboard.mozilla.org/r/59588/#review56688
Attachment #8763423 -
Flags: review?(martin.thomson) → review+
Comment 14•8 years ago
|
||
Comment on attachment 8763424 [details] Bug 1280844 - Move some null checks earlier in lib/. . https://reviewboard.mozilla.org/r/59590/#review56690 ::: security/nss/lib/ssl/sslmutex.c:414 (Diff revision 1) > - if (!pMutex || (hMutex = pMutex->u.sslMutx) == 0 || > + if ((hMutex = pMutex->u.sslMutx) == 0 || hMutex == INVALID_HANDLE_VALUE) { > - hMutex == INVALID_HANDLE_VALUE) { Isn't this change sufficient? This isn't a public function, and we have an assert that pMutex is non-null.
Attachment #8763424 -
Flags: review?(martin.thomson)
Updated•8 years ago
|
Attachment #8763425 -
Flags: review?(martin.thomson) → review+
Comment 15•8 years ago
|
||
Comment on attachment 8763425 [details] Bug 1280844 - Remove some more redundant null checks in lib/. . https://reviewboard.mozilla.org/r/59592/#review56692
Assignee | ||
Comment 16•8 years ago
|
||
https://reviewboard.mozilla.org/r/59590/#review56690 > Isn't this change sufficient? This isn't a public function, and we have an assert that pMutex is non-null. Probably, but NSS is very cautious with argument handling and this kind of double checking (assert non-null and then return if null) is prevalent, so I stuck with it.
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8763420 [details] Bug 1280844 - Fix up null checks in cmd/. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59582/diff/1-2/
Attachment #8763424 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8763421 [details] Bug 1280844 - Remove useless assert() in pathsub.c. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59584/diff/1-2/
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8763422 [details] Bug 1280844 - Fix a null check in parse.c. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59586/diff/1-2/
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8763423 [details] Bug 1280844 - Remove some redundant null checks in lib/. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59588/diff/1-2/
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8763424 [details] Bug 1280844 - Move some null checks earlier in lib/. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59590/diff/1-2/
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8763425 [details] Bug 1280844 - Remove some more redundant null checks in lib/. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59592/diff/1-2/
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8763426 [details] Bug 1280844 - Remove even more redundant null checks in lib/. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59594/diff/1-2/
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8763427 [details] Bug 1280844 - Fix a null check in p12d.c. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59596/diff/1-2/
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8763428 [details] Bug 1280844 - Remove still more redundant null checks in lib/. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59598/diff/1-2/
Updated•8 years ago
|
Attachment #8763426 -
Flags: review?(martin.thomson) → review+
Comment 26•8 years ago
|
||
Comment on attachment 8763426 [details] Bug 1280844 - Remove even more redundant null checks in lib/. . https://reviewboard.mozilla.org/r/59594/#review56818
Updated•8 years ago
|
Attachment #8763427 -
Flags: review?(martin.thomson) → review+
Comment 27•8 years ago
|
||
Comment on attachment 8763427 [details] Bug 1280844 - Fix a null check in p12d.c. . https://reviewboard.mozilla.org/r/59596/#review56820 ::: security/nss/lib/pkcs12/p12d.c:1619 (Diff revision 2) > if(!bag->attribs) { > return SECFailure; > } > > bag->attribs[i] = PORT_ArenaZNew(bag->arena, sec_PKCS12Attribute); > - if(!bag->attribs) { > + if(!bag->attribs[i]) { Nice.
Updated•8 years ago
|
Attachment #8763428 -
Flags: review?(martin.thomson) → review+
Comment 28•8 years ago
|
||
Comment on attachment 8763428 [details] Bug 1280844 - Remove still more redundant null checks in lib/. . https://reviewboard.mozilla.org/r/59598/#review56822
Comment 29•8 years ago
|
||
Comment on attachment 8763424 [details] Bug 1280844 - Move some null checks earlier in lib/. . https://reviewboard.mozilla.org/r/59590/#review56824 Excess paranoia is harmless I guess.
Attachment #8763424 -
Flags: review?(martin.thomson) → review+
Comment 30•8 years ago
|
||
remote: https://hg.mozilla.org/projects/nss/rev/a534095c4347 remote: https://hg.mozilla.org/projects/nss/rev/d46ba3e226e1 remote: https://hg.mozilla.org/projects/nss/rev/1d90aaeb3df8 remote: https://hg.mozilla.org/projects/nss/rev/839ebb15e92f remote: https://hg.mozilla.org/projects/nss/rev/89779ff5d079 remote: https://hg.mozilla.org/projects/nss/rev/cbb357c8e651 remote: https://hg.mozilla.org/projects/nss/rev/5afcbd7d80f8 remote: https://hg.mozilla.org/projects/nss/rev/e8aeeeec31c0 remote: https://hg.mozilla.org/projects/nss/rev/f01aa49c7629
Assignee: nobody → n.nethercote
Status: NEW → RESOLVED
Closed: 8 years ago
Component: General → Libraries
Product: Core → NSS
Resolution: --- → FIXED
Target Milestone: --- → 3.26
Version: Trunk → trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•