Enable S/MIME tests on LSan runs

RESOLVED FIXED in 3.26

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8768379 [details] [diff] [review]
0001-Bug-1284828-Enable-S-MIME-tests-on-LSan-runs.patch
Attachment #8768379 - Flags: review?(franziskuskiefer)
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.
Attachment #8768379 - Flags: review?(franziskuskiefer) → review+
(Assignee)

Comment 3

2 years ago
(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?
> 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.
(Assignee)

Updated

2 years ago
Depends on: 1285246
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/projects/nss/rev/300d883fe07a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.26
You need to log in before you can comment on or make changes to this bug.