1279399 - Coverity issues from bug 1266237

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

Attachment: Coverity fixes

A few issues were introduced.  None of them are serious.

Comment 1

[Franziskus Kiefer [:franziskus]]

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).

Comment 2

[Franziskus Kiefer [:franziskus]]

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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1279399-Fix-Coverity-issues-from-bug-1266237.patch, v2

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(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|.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/668d06a34543