1284187 - New coverity issues in Snapshot 164273

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

** CID 1362987:    (USE_AFTER_FREE)
/lib/jar/jarver.c: 355 in jar_parse_any()
/lib/jar/jarver.c: 355 in jar_parse_any()


________________________________________________________________________________________________________
*** CID 1362987:    (USE_AFTER_FREE)
/lib/jar/jarver.c: 355 in jar_parse_any()
349                 }
350             }
351
352             if (type != jarTypeMF) {
353                 PORT_Free(met->header);
354                 if (type != jarTypeSF || !jar->globalmeta) {
>>>     CID 1362987:    (USE_AFTER_FREE)
>>>     Calling "PORT_Free_Util" frees pointer "met->info" which has already been freed.
355                     PORT_Free(met->info);
356                 }
357                 PORT_Free(met);
358             }
359         }
360
/lib/jar/jarver.c: 355 in jar_parse_any()
349                 }
350             }
351
352             if (type != jarTypeMF) {
353                 PORT_Free(met->header);
354                 if (type != jarTypeSF || !jar->globalmeta) {
>>>     CID 1362987:    (USE_AFTER_FREE)
>>>     Passing freed pointer "met->info" as an argument to "PORT_Free_Util".
355                     PORT_Free(met->info);
356                 }
357                 PORT_Free(met);
358             }
359         }
360

** CID 1362986:  Resource leaks  (RESOURCE_LEAK)
/cmd/ecperf/ecperf.c: 311 in hexString2SECItem()


________________________________________________________________________________________________________
*** CID 1362986:  Resource leaks  (RESOURCE_LEAK)
/cmd/ecperf/ecperf.c: 311 in hexString2SECItem()
305                 tmp = str[i] - '0';
306             else if ((str[i] >= 'a') && (str[i] <= 'f'))
307                 tmp = str[i] - 'a' + 10;
308             else if ((str[i] >= 'A') && (str[i] <= 'F'))
309                 tmp = str[i] - 'A' + 10;
310             else
>>>     CID 1362986:  Resource leaks  (RESOURCE_LEAK)
>>>     Returning without freeing "item" leaks the storage that it points to.
311                 return NULL;
312
313             byteval = byteval * 16 + tmp;
314             if ((i % 2) != 0) {
315                 item->data[i / 2] = byteval;
316                 byteval = 0;

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: coverity-snapshot-164273.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8767561 [details] [diff] [review]
coverity-snapshot-164273.patch

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

::: cmd/ecperf/ecperf.c
@@ +312,1 @@
>              return NULL;

Coverity complains because it assumes that (arena == NULL && item == NULL). We could assert both to be non-null at the top of the function to silence it. That's how I would love the SECITEM* functions to behave anyway :)

::: lib/jar/jarver.c
@@ +355,1 @@
>                  PORT_Free(met->info);

This won't work because |met->info| isn't set to NULL by PORT_Free() above. How about changing the condition to |if (!(type == jarTypeSF && jar->globalmeta) && (sf_md5 || sf_sha1)) {?

It's a little messy but so we'd handle the free() call on line 348, as well as the possibility that we'll never enter the block on line 361 and so free(met->info) to not leak.

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: coverity-snapshot-164273.patch

(In reply to Tim Taubert [:ttaubert] from comment #2)
> Comment on attachment 8767561 [details] [diff] [review]
> coverity-snapshot-164273.patch
> 
> Review of attachment 8767561 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: cmd/ecperf/ecperf.c
> @@ +312,1 @@
> >              return NULL;
> 
> Coverity complains because it assumes that (arena == NULL && item == NULL).
> We could assert both to be non-null at the top of the function to silence
> it. That's how I would love the SECITEM* functions to behave anyway :)

Well, item is probably NULL. But we can assert arena (btw, I don't think covertiy is intelligent enough to detect use of the arena).

> ::: lib/jar/jarver.c
> @@ +355,1 @@
> >                  PORT_Free(met->info);
> 
> This won't work because |met->info| isn't set to NULL by PORT_Free() above.
> How about changing the condition to |if (!(type == jarTypeSF &&
> jar->globalmeta) && (sf_md5 || sf_sha1)) {?
> 
> It's a little messy but so we'd handle the free() call on line 348, as well
> as the possibility that we'll never enter the block on line 361 and so
> free(met->info) to not leak.

how about |met->info = NULL| in addition to the PORT_Free?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8767659 [details] [diff] [review]
coverity-snapshot-164273.patch

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

::: cmd/ecperf/ecperf.c
@@ +284,5 @@
>      int i = 0;
>      int byteval = 0;
>      int tmp = PORT_Strlen(str);
>  
> +    PORT_Assert(arena);

|item| is never null, all callers in ecperf.c pass a stack pointer. Although I think Coverity will stop complaining with arena != NULL.

Comment 5

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/1887efbdceda