1257885 - Fix some unchecked return values

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

Fixing some unchecked return values that coverity found.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: checkSecItemReturns.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8732223 [details] [diff] [review]
checkSecItemReturns.patch

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

::: cmd/bltest/blapitest.c
@@ +979,5 @@
>  	    break;
>  	}
>  	if (in->data[in->len-1] == '\n') --in->len;
>  	if (in->data[in->len-1] == '\r') --in->len;
> +	CHECKERROR(SECITEM_CopyItem(arena, &input->buf, in), __LINE__);

I'm not sure that using CHECKERROR() is the right thing here, a few lines above we check the return value of SECU_FileToItem() and in that case simply return SECFailure, instead of calling exit() as CHECKERROR() would do. Also problematic though seems that not a single caller of setupIO() checks its return value.

@@ +1062,5 @@
>  
>  void
>  bltestCopyIO(PLArenaPool *arena, bltestIO *dest, bltestIO *src)
>  {
> +    CHECKERROR(SECITEM_CopyItem(arena, &dest->buf, &src->buf), __LINE__);

It might make more sense to have bltestCopyIO() return a SECStatus, its call sites seem to usually check for a failure along the way (rv |= ...) and probably don't expect the program to exit.

::: cmd/crlutil/crlutil.c
@@ +55,5 @@
>              }
>              CERT_DestroyName(certName);
>          }
>  
> +        if (rv != SECSuccess || !derName.len || !derName.data) {

I think we should probably check |rv| separately and print a different error message? SECITEM_CopyItem() would probably only fail when OOM, that shouldn't occur too often when testing.

::: lib/pkcs12/p12dec.c
@@ +63,5 @@
>  	}
>  	pfx->old = PR_TRUE;
> +	rv = SGN_CopyDigestInfo(pfx->poolp, &pfx->macData.safeMac, &pfx->old_safeMac);
> +	if(rv != SECSuccess) {
> +	    PORT_SetError(SEC_ERROR_PKCS12_DECODING_PFX);

This should probably be SEC_ERROR_NO_MEMORY.

@@ +69,5 @@
> +	    return NULL;
> +	}
> +	rv = SECITEM_CopyItem(pfx->poolp, &pfx->macData.macSalt, &pfx->old_macSalt);
> +	if(rv != SECSuccess) {
> +	    PORT_SetError(SEC_ERROR_PKCS12_DECODING_PFX);

SEC_ERROR_NO_MEMORY here too.

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: checkSecItemReturns.patch

this should address everything.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8733286 [details] [diff] [review]
checkSecItemReturns.patch

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

::: cmd/crlutil/crlutil.c
@@ +56,5 @@
>              CERT_DestroyName(certName);
>          }
>  
> +        if (rv != SECSuccess) {
> +            SECU_PrintError(progName, "SECITEM_CopyItem failed, out of memorey");

*memory :)

Comment 5

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/4ebc95c6d748