Closed Bug 1279399 Opened 5 years ago Closed 5 years ago

Coverity issues from bug 1266237

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mt, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

Attachments

(1 file, 1 obsolete file)

Attached patch Coverity fixes (obsolete) — Splinter Review
A few issues were introduced.  None of them are serious.
Attachment #8761892 - Flags: review?(franziskuskiefer)
Comment on attachment 8761892 [details] [diff] [review]
Coverity fixes

Review of attachment 8761892 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. But please fix line endings before landing (this is a windows patch).
Attachment #8761892 - Flags: review?(franziskuskiefer) → review+
Assignee: nobody → martin.thomson
Blocks: nss-coverity
Keywords: coverity
Comment on attachment 8761892 [details] [diff] [review]
Coverity fixes

Review of attachment 8761892 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3ecc.c
@@ +76,1 @@
>      SECITEM_AllocItem(arena, params, (2 + oidData->oid.len));

looking at this again this isn't correct. We call this in derive.c:799 without an arena and this allocate |params| here. We have to add an |if (!arena) PORT_Free(params)| at the end.
Attachment #8761892 - Flags: review+
Duplicate of this bug: 1280851
I think mt doesn't mind me stealing his patch here.

This is basically his patch with Franziskus' comment addressed. It has two additional changes in ssl_NamedGroup2ECParams():

* Check SECITEM_AllocItem's return value in case of OOM
* Check that (params != NULL), the function signature wouldn't work otherwise and we can silence Coverity too
Assignee: martin.thomson → ttaubert
Attachment #8761892 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8763473 - Flags: review?(franziskuskiefer)
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #2)
> ::: lib/ssl/ssl3ecc.c
> @@ +76,1 @@
> >      SECITEM_AllocItem(arena, params, (2 + oidData->oid.len));
> 
> looking at this again this isn't correct. We call this in derive.c:799
> without an arena and this allocate |params| here. We have to add an |if
> (!arena) PORT_Free(params)| at the end.

Just FTR, derive.c correctly handles the allocated data and frees it. Coverity barks because it thinks that (arena == NULL && params == NULL) which my patch addresses by null-checking |params|.
Attachment #8763473 - Flags: review?(franziskuskiefer) → review+
https://hg.mozilla.org/projects/nss/rev/668d06a34543
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.