Closed Bug 1281724 Opened 5 years ago Closed 5 years ago

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

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(10 files, 2 obsolete files)

2.02 KB, patch
franziskus
: review+
ttaubert
: checked-in+
Details | Diff | Splinter Review
1.10 KB, patch
franziskus
: review+
ttaubert
: checked-in+
Details | Diff | Splinter Review
7.11 KB, patch
franziskus
: review+
ttaubert
: checked-in+
Details | Diff | Splinter Review
7.95 KB, patch
franziskus
: review+
ttaubert
: checked-in+
Details | Diff | Splinter Review
1.51 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
845 bytes, patch
franziskus
: review+
Details | Diff | Splinter Review
950 bytes, patch
franziskus
: review+
Details | Diff | Splinter Review
2.56 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
2.37 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
1.43 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
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).
Summary: Fix certutil leaks reported by LSan → Fix certutil leak reported by LSan
Attachment #8764500 - Flags: review?(franziskuskiefer)
Comment on attachment 8764500 [details] [diff] [review]
0001-Bug-1281724-Fix-certutil-leak-reported-by-LSan.patch

Wait, I think there's more.
Attachment #8764500 - Flags: review?(franziskuskiefer)
Summary: Fix certutil leak reported by LSan → Fix certutil/crlutil leaks reported by LSan
Summary: Fix certutil/crlutil leaks reported by LSan → Fix various leaks reported by LSan when running all.sh with NSS_TESTS=cert
Attachment #8764500 - Attachment is obsolete: true
Attachment #8764903 - Flags: review?(franziskuskiefer) → review+
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 :)
Attachment #8764904 - Flags: review?(franziskuskiefer) → review+
Attachment #8764905 - Flags: review?(franziskuskiefer) → review+
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.
Attachment #8764908 - Flags: review?(franziskuskiefer) → review+
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| :)
Component: Build → Libraries
Attachment #8764903 - Flags: checked-in+
Attachment #8764904 - Flags: checked-in+
Attachment #8764905 - Flags: checked-in+
Attachment #8764908 - Flags: checked-in+
No leak, but while staring at the code I found the arena was unused.
Attachment #8765414 - Flags: review?(franziskuskiefer)
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.
Attachment #8765416 - Flags: review?(franziskuskiefer)
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.
Attachment #8765429 - Flags: review?(franziskuskiefer)
That's the one you helped me find this morning :)
Attachment #8765430 - Flags: review?(franziskuskiefer)
Attachment #8765414 - Flags: review?(franziskuskiefer) → review+
Attachment #8765416 - Flags: review?(franziskuskiefer) → review+
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.
Attachment #8765429 - Flags: review?(franziskuskiefer)
Attachment #8765430 - Flags: review?(franziskuskiefer) → review+
Attachment #8765431 - Flags: review?(franziskuskiefer) → review+
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...
Attachment #8765432 - Flags: review+
Attachment #8765429 - Attachment is obsolete: true
Attachment #8765459 - Flags: review?(franziskuskiefer)
Attachment #8765459 - Flags: review?(franziskuskiefer) → review+
(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.
Duplicate of this bug: 658092
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.