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)

defect
Not set
normal

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)

Unchecked return value (in NSC_GenerateKey) and dead code (in sftk_CryptInit) in pkcs11c.c
Attached patch 1234521-v2.patch (obsolete) — Splinter Review
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)
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 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-
If you want to attach a fixed version of that version of that patch, I can r+ and land it.
Attachment #8715229 - Attachment is obsolete: true
Attachment #8716996 - Flags: review?(kaie)
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
We can save another bit of code by wrapping the *phKey = key->handle; assignment inside if(crv==CKR_OK).
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-
Attached patch patch v4Splinter Review
Attachment #8716392 - Attachment is obsolete: true
Attachment #8719619 - Flags: review?(paul.bignier)
Attachment #8719619 - Flags: review?(paul.bignier) → review+
https://hg.mozilla.org/projects/nss/rev/ff8696cc41bb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Future
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: