337014 - Coverity OOM crash [@ PORT_ArenaAlloc - PK11_PQG_ParamGenSeedLen][@ PORT_ArenaAlloc - PK11_PQG_ParamGenSeedLen] Dereferencing possibly NULL "varena"

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

Event returned_null: Function "PORT_NewArena" returned NULL value (checked 219 out of 231 times) [model]

Comment 1

[Alexei Volkov]

Attachment: check arena value after creation

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222753 [details] [diff] [review]
check arena value after creation

>     varena = PORT_NewArena(60);
>+    if (!varena) {
>+	PORT_SetError(SEC_ERROR_NO_MEMORY);
>+	goto loser;
>+    }        
>+

When I first reviewed this patch, I thought to myself: "it should be unecessary
to set SEC_ERROR_NO_MEMORY after an allocation failure, because all PORT_ 
allocation functions should set that before they return."  Then I examined 
PORT_NewArena and discovered to my chagrin that it does not.

So, it is correct to tset the error code here, as this patch does.
However, I think the "right fix" is not to add a PORT_SetError call after 
every call to PORT_NewArena, but rather is to fix PORT_NewArena.
I have filed a separate bug about that.

So, I give this patch r+ now, with the knowledge that once we fix PORT_NewArena,
coverity will begin to report dead code complaints, about the error code 
being set twice in a row to the same value.

Comment 3

[Alexei Volkov]

Integrated patch does not invoke PORT_SetError in case when varena is null (see comment #2).
 
trunk:
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pqg.c,v  <--  pk11pqg.c
new revision: 1.12; previous revision: 1.11

3.11 branch:
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pqg.c,v  <--  pk11pqg.c
new revision: 1.11.2.1; previous revision: 1.11

Comment 4

[timeless]

fwiw, coverity at present can't complain about double calls. although that isn't to say that a future version won't :).

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

CID 528.