Closed
Bug 1234521
Opened 8 years ago
Closed 8 years ago
[CID 221892][CID 982225] unchecked return value and dead code in pkcs11c.c
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(firefox46 affected)
RESOLVED
FIXED
Future
Tracking | Status | |
---|---|---|
firefox46 | --- | affected |
People
(Reporter: franziskus, Assigned: KaiE)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: CID221892,CID982225)
Attachments
(2 files, 2 obsolete files)
4.39 KB,
patch
|
KaiE
:
review-
|
Details | Diff | Splinter Review |
4.31 KB,
patch
|
paul.bignier
:
review+
|
Details | Diff | Splinter Review |
Unchecked return value (in NSC_GenerateKey) and dead code (in sftk_CryptInit) in pkcs11c.c
Comment 1•8 years ago
|
||
Attachment #8715229 -
Flags: review?(kaie)
Assignee | ||
Comment 2•8 years ago
|
||
Paul, thanks for your patch. It seems right. However, I'm not happy with the multiple duplication of the cleanup code. What do you think about this alternative approach?
Attachment #8716392 -
Flags: review?(paul.bignier)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8715229 [details] [diff] [review] 0001-Coverity-221892-982225-Check-return-value-remove-dea.patch r- for undesirable code duplication
Attachment #8715229 -
Flags: review?(kaie) → review-
Comment 4•8 years ago
|
||
Comment on attachment 8716392 [details] [diff] [review] 1234521-v2.patch Review of attachment 8716392 [details] [diff] [review]: ----------------------------------------------------------------- Yes I like your method. However line 4066 needs if (crv != CKR_OK) { sftk_FreeObject(key); return crv; } like it is done above, otherwise you'll corrupt your input with the next line. Lines 488X are fine but look confusing because of the indentation, but maybe this is just my Geany editor (which shows two consecutive parentheses on the same column), I hope it shows properly on yours.
Attachment #8716392 -
Flags: review?(paul.bignier) → review-
Assignee | ||
Comment 5•8 years ago
|
||
If you want to attach a fixed version of that version of that patch, I can r+ and land it.
Comment 6•8 years ago
|
||
Attachment #8715229 -
Attachment is obsolete: true
Attachment #8716996 -
Flags: review?(kaie)
Assignee | ||
Comment 7•8 years ago
|
||
In the future, if possible please create patches against the master NSS repository, because Firefox uses older snapshots. Your line if (crv != CKR_OK) { sftk_FreeObject(key); return crv; } uses an incorrect indent, which is easy to get wrong, because we're still using a mix of tabs and spaces. After line if (crv == CKR_OK) { I indented the block that follows, which might have caused the new whitespace that confused you. I also use, geany with these settings: prefs/editor/indentation: width 4, detect width from file, type: spaces, detect type from file
Assignee | ||
Comment 8•8 years ago
|
||
We can save another bit of code by wrapping the *phKey = key->handle; assignment inside if(crv==CKR_OK).
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8716996 [details] [diff] [review] 0001-CID-221892-CID-982225-unchecked-return-value-and-dea.patch code is correct, r- only for wrong indent. I'll attach a patch with fixed tab/space indenting, plus avoiding the slight code duplication.
Attachment #8716996 -
Flags: review?(kaie) → review-
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8716392 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8719619 -
Flags: review?(paul.bignier)
Updated•8 years ago
|
Attachment #8719619 -
Flags: review?(paul.bignier) → review+
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/ff8696cc41bb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Future
Updated•8 years ago
|
Assignee: nobody → kaie
You need to log in
before you can comment on or make changes to this bug.
Description
•