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 on attachment 8764500 [details] [diff] [review] 0001-Bug-1281724-Fix-certutil-leak-reported-by-LSan.patch Wait, I think there's more.

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

(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| :)

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

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

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.

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.

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

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

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

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

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

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