1285145 - Enable SSL tests on LSan runs

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1285145-Enable-SSL-tests-on-LSan-runs.patch

Here's a green try run:

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=22c9335403037763cb54d7d445cc634e587b7672

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8769880 [details] [diff] [review]
0001-Bug-1285145-Enable-SSL-tests-on-LSan-runs.patch

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

Can you check the selfserv thing? Otherwise looking good. The HMAC_Finish issue we can also do in a follow up.

::: cmd/selfserv/selfserv.c
@@ +730,5 @@
>      PZ_NotifyAllCondVar(jobQNotEmptyCv);
> +    PZ_Unlock(qLock);
> +
> +    /* Wait for worker threads to terminate. */
> +    for (i = 0; i < maxThreads; ++i) {

Do we always have maxThreads? Before threadCount was used. Sounds to me like we could get stuck in this loop if threadCount < maxThreads or get a null pointer problem when accessing slot->prThread

::: cmd/tstclnt/tstclnt.c
@@ +1238,5 @@
> +    } else {
> +        char *certDirTmp = certDir;
> +        certDir = SECU_ConfigDirectory(certDirTmp);
> +        PORT_Free(certDirTmp);
> +    }

not sure if I see how moving this solves a leak, but I also don't see anything wrong with it.

::: lib/ssl/ssl3ext.c
@@ +1492,5 @@
>          HMAC_Update(hmac_ctx, key_name, SESS_TICKET_KEY_NAME_LEN);
>          HMAC_Update(hmac_ctx, iv, sizeof(iv));
>          HMAC_Update(hmac_ctx, (unsigned char *)length_buf, 2);
>          HMAC_Update(hmac_ctx, ciphertext.data, ciphertext.len);
>          HMAC_Finish(hmac_ctx, computed_mac, &computed_mac_length,

not a leak but HMAC_Finish returns a SECStatus. We should check that.

@@ +1494,5 @@
>          HMAC_Update(hmac_ctx, (unsigned char *)length_buf, 2);
>          HMAC_Update(hmac_ctx, ciphertext.data, ciphertext.len);
>          HMAC_Finish(hmac_ctx, computed_mac, &computed_mac_length,
>                      sizeof(computed_mac));
> +        HMAC_Destroy(hmac_ctx, PR_FALSE);

all other cases destroy in loser. Maybe we should do that here as well.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #2)
> ::: cmd/selfserv/selfserv.c
> @@ +730,5 @@
> >      PZ_NotifyAllCondVar(jobQNotEmptyCv);
> > +    PZ_Unlock(qLock);
> > +
> > +    /* Wait for worker threads to terminate. */
> > +    for (i = 0; i < maxThreads; ++i) {
> 
> Do we always have maxThreads? Before threadCount was used. Sounds to me like
> we could get stuck in this loop if threadCount < maxThreads or get a null
> pointer problem when accessing slot->prThread

That's a good point. We'll always have |maxThreads| perThread structs but not all of them might have threads due to OOM. Null-checking |slot->prThread| should be sufficient.

> ::: cmd/tstclnt/tstclnt.c
> @@ +1238,5 @@
> > +    } else {
> > +        char *certDirTmp = certDir;
> > +        certDir = SECU_ConfigDirectory(certDirTmp);
> > +        PORT_Free(certDirTmp);
> > +    }
> 
> not sure if I see how moving this solves a leak, but I also don't see
> anything wrong with it.

Could have explained that some more. With pingServerFirst=true we would not reach the part of the code I moved up. We would leak |certDir| in that case. It's not the greatest solution as we don't need that config dir when pingServerFirst=true but everything else would have been uglier.

> >          HMAC_Finish(hmac_ctx, computed_mac, &computed_mac_length,
> 
> not a leak but HMAC_Finish returns a SECStatus. We should check that.

> > +        HMAC_Destroy(hmac_ctx, PR_FALSE);
> 
> all other cases destroy in loser. Maybe we should do that here as well.

Will do.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1285145-Enable-SSL-tests-on-LSan-runs.patch, v2

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/2530cf04ae82