1281724 - Fix various leaks reported by LSan when running all.sh with NSS_TESTS=cert

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

This occurs when building with ASan/LSan and running all.sh with NSS_TESTS=cert.

==17248==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 5 byte(s) in 1 object(s) allocated from:
    #0 0x7fa42e38c54a in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x9854a)
    #1 0x7fa42d1e8e17 in PR_Malloc ../../../../pr/src/malloc/prmem.c:435
    #2 0x7fa42d8cb1fd in PORT_Alloc_Util /home/worker/nss/lib/util/secport.c:86
    #3 0x7fa42d8c6391 in sec_asn1e_allocate_item /home/worker/nss/lib/util/secasn1e.c:1509
    #4 0x7fa42d8c655a in SEC_ASN1EncodeItem_Util /home/worker/nss/lib/util/secasn1e.c:1537
    #5 0x7fa42dc5667d in CERT_EncodeBasicConstraintValue /home/worker/nss/lib/certdb/xbsconst.c:81
    #6 0x43647b in SECU_EncodeAndAddExtensionValue /home/worker/nss/cmd/lib/secutil.c:3680
    #7 0x409ff4 in AddBasicConstraint /home/worker/nss/cmd/certutil/certext.c:942
    #8 0x40e2b5 in AddExtensions /home/worker/nss/cmd/certutil/certext.c:2017
    #9 0x418bb4 in CreateCert /home/worker/nss/cmd/certutil/certutil.c:1997
    #10 0x41f46a in certutil_main /home/worker/nss/cmd/certutil/certutil.c:3440
    #11 0x420469 in main /home/worker/nss/cmd/certutil/certutil.c:3658
    #12 0x7fa42cbe182f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: 5 byte(s) leaked in 1 allocation(s).

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1281724-Fix-certutil-leak-reported-by-LSan.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8764500 [details] [diff] [review]
0001-Bug-1281724-Fix-certutil-leak-reported-by-LSan.patch

Wait, I think there's more.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1281724-Remove-optimization-that-makes-it-hard-t.patch

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1281724-SECU_DerSignDataCRL-should-copy-signatur.patch

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1281724-Fix-various-leaks-in-crlutil.patch

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Bug-1281724-Various-leak-fixes-in-utility-commands.patch

Comment 7

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8764904 [details] [diff] [review]
0003-Bug-1281724-SECU_DerSignDataCRL-should-copy-signatur.patch

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

r+ with those nits fixed.

::: cmd/lib/secutil.c
@@ +3564,5 @@
>      PORT_Memset(sd, 0, sizeof(*sd));
>      sd->data.data = buf;
>      sd->data.len = len;
> +    rv = SECITEM_CopyItem(arena, &sd->signature, &it);
> +    if (rv)

please use an explicit check != SECSuccess and use braces (you can also fix the one further down).

@@ +3567,5 @@
> +    rv = SECITEM_CopyItem(arena, &sd->signature, &it);
> +    if (rv)
> +        goto loser;
> +
> +    SECITEM_FreeItem (&it, PR_FALSE);

nit: space

@@ +3568,5 @@
> +    if (rv)
> +        goto loser;
> +
> +    SECITEM_FreeItem (&it, PR_FALSE);
> +    sd->signature.len *= 8; /* convert to bit string */

let's hope the compiler is intelligent enough to convert this to a shift :)

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8764908 [details] [diff] [review]
0005-Bug-1281724-Various-leak-fixes-in-utility-commands.patch

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

::: lib/pki/pki3hack.c
@@ +1351,5 @@
>  	    	nssrv = PR_FAILURE;
>  	    }
>  	}
>      }
> +    nssTrust_Destroy(nssTrust);

it says  /* caller made sure nssTrust isn't NULL */ further up, which I don't think is true. A null check after getting nssTrust can't hurt in any case.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8764908 [details] [diff] [review]
0005-Bug-1281724-Various-leak-fixes-in-utility-commands.patch

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

::: lib/pki/pki3hack.c
@@ +1351,5 @@
>  	    	nssrv = PR_FAILURE;
>  	    }
>  	}
>      }
> +    nssTrust_Destroy(nssTrust);

Yeah, that doesn't seem true. Will fix.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #7)
> ::: cmd/lib/secutil.c
> @@ +3564,5 @@
> >      PORT_Memset(sd, 0, sizeof(*sd));
> >      sd->data.data = buf;
> >      sd->data.len = len;
> > +    rv = SECITEM_CopyItem(arena, &sd->signature, &it);
> > +    if (rv)
> 
> please use an explicit check != SECSuccess and use braces (you can also fix
> the one further down).

Done.

> @@ +3568,5 @@
> > +    if (rv)
> > +        goto loser;
> > +
> > +    SECITEM_FreeItem (&it, PR_FALSE);
> > +    sd->signature.len *= 8; /* convert to bit string */
> 
> let's hope the compiler is intelligent enough to convert this to a shift :)

I'm quite sure they are, but I changed it back to |len <<= 3| :)

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/32f6e389a328
https://hg.mozilla.org/projects/nss/rev/dc751ee81d93
https://hg.mozilla.org/projects/nss/rev/52c28c890e14
https://hg.mozilla.org/projects/nss/rev/c5d32f972f76

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1281724-Remove-unused-arena-in-UpdateV4DB.patch

No leak, but while staring at the code I found the arena was unused.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1281724-Make-sftk_InitFreeList-a-no-op-when-the-.patch

The two free lists are static and we leak the lock if we simply overwrite it if we don't check whether the list was already initialized.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Bug-1281724-Fix-leak-in-crlutil-s-crlgen.c.patch

We're leaking hash table entry data as that's only destroyed when we fail to remove the entry from the hash table, which really doesn't make sense.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-1281724-Don-t-leak-certificates-in-lg_SetTrustAt.patch

That's the one you helped me find this morning :)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0007-Bug-1281724-Free-cert-extensions-on-failure.patch

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0008-Bug-1281724-Don-t-leak-OIDs-by-copying-them-when-cop.patch

Comment 18

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8765429 [details] [diff] [review]
0005-Bug-1281724-Fix-leak-in-crlutil-s-crlgen.c.patch

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

::: cmd/crlutil/crlgen.c
@@ +73,5 @@
>      data = crlgen_FindEntry(crlGenData, certId);
>      if (!data)
>          return SECSuccess;
> +    if (!PL_HashTableRemove(crlGenData->entryDataHashTable, certId))
> +        return SECFailure;

don't we want to always destroy data? Getting here means data != null and if we simply return here, data gets leaked.

Comment 19

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8765432 [details] [diff] [review]
0008-Bug-1281724-Don-t-leak-OIDs-by-copying-them-when-cop.patch

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

::: lib/certdb/certxutl.c
@@ +197,4 @@
>      if (copyData) {
> +        rv = SECITEM_CopyItem(handle->ownerArena, &ext->id, oid);
> +        if (rv) {
> +            return (SECFailure);

we have to remove all those return () at some point...

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1281724-Fix-leak-in-crlutil-s-crlgen.c.patch, v2

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #19)
> >      if (copyData) {
> > +        rv = SECITEM_CopyItem(handle->ownerArena, &ext->id, oid);
> > +        if (rv) {
> > +            return (SECFailure);
> 
> we have to remove all those return () at some point...

Would be nice if this were something clang-format did, but it doesn't modify the AST, right?

Comment 22

[Franziskus Kiefer [:franziskus]]

> Would be nice if this were something clang-format did, but it doesn't modify the AST, right?

yep, clang-format only does things with spaces and line breaks. But it shouldn't be too hard to write a script for this.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/33fbb461134c
https://hg.mozilla.org/projects/nss/rev/58cc2dabcd72
https://hg.mozilla.org/projects/nss/rev/e2570a09c898
https://hg.mozilla.org/projects/nss/rev/a6d37c4946c8
https://hg.mozilla.org/projects/nss/rev/99490dafa939
https://hg.mozilla.org/projects/nss/rev/20b54133cffb

Comment 25

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.