Closed
Bug 1284828
Opened 8 years ago
Closed 8 years ago
Enable S/MIME tests on LSan runs
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.26
People
(Reporter: ttaubert, Assigned: ttaubert)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
11.12 KB,
patch
|
franziskus
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8768379 -
Flags: review?(franziskuskiefer)
Comment 2•8 years ago
|
||
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•8 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?
Comment 4•8 years ago
|
||
> 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 | ||
Comment 5•8 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 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=911e306c5762b4507de80a3d2038b1ecd9e5c186
Assignee | ||
Comment 7•8 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/300d883fe07a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.26
You need to log in
before you can comment on or make changes to this bug.
Description
•