1284828 - Enable S/MIME tests on LSan runs

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1284828-Enable-S-MIME-tests-on-LSan-runs.patch

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8768379 [details] [diff] [review]
0001-Bug-1284828-Enable-S-MIME-tests-on-LSan-runs.patch

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

r+ for lsan fixes with the p7content one fixed.
We should probably also fix the other cmsutil leaks, but feel free to file a follow up if you want to land this one first.

::: cmd/p7content/p7content.c
@@ +266,3 @@
>      }
>  
>      return 0;

I think you want to return error;

::: cmd/smimetools/cmsutil.c
@@ +1375,5 @@
>  
>      if (mode != CERTSONLY && !batch) {
>          rv = SECU_FileToItem(&input, inFile);
>          if (rv != SECSuccess) {
>              SECU_PrintError(progName, "unable to read infile");

If we exit here, nothing is cleaned up (e.g. inFile might well be open). There are a couple of other exit/returns we could clean up.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #2)
> We should probably also fix the other cmsutil leaks, but feel free to file a
> follow up if you want to land this one first.

> >          rv = SECU_FileToItem(&input, inFile);
> >          if (rv != SECSuccess) {
> >              SECU_PrintError(progName, "unable to read infile");
> 
> If we exit here, nothing is cleaned up (e.g. inFile might well be open).
> There are a couple of other exit/returns we could clean up.

I'm a little conflicted, while I would like to fix those properly I'm not too concerned about short-term leaks in command line utilities. It doesn't seem like the best use of our time to try and fix these, the cmd/ files are a huge mess and I spent more time trying to clean up tstclnt than I thought it would take. I'd rather fix what I run into until LSan stops complaining - after all we're much more interested in leaks in the library itself. And we can catch those by getting LSan running across all test suites.

Fixing those in a follow-up is certainly fine but I honestly don't see myself getting back to that anytime soon considering what other things we might/should work on.

WDYT?

Comment 4

[Franziskus Kiefer [:franziskus]]

> Fixing those in a follow-up is certainly fine but I honestly don't see myself getting back to that anytime soon considering what other things we might/should work on.

I agree, it's not the most useful thing to do. Just file a bug with P3 to fix them and we'll see if anyone gets to it.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #2)
> ::: cmd/p7content/p7content.c
> @@ +266,3 @@
> >      }
> >  
> >      return 0;
> 
> I think you want to return error;

Good catch, thanks.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/300d883fe07a