Closed Bug 1000548 Opened 7 years ago Closed 6 years ago

Leaking arenas allocated in mozilla::pkix::CheckIssuerIndependentProperties

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: mccr8, Assigned: cviecco)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, Whiteboard: [lsan][MemShrink][qa-])

Attachments

(3 files, 2 obsolete files)

Attached file stacks for leaking allocations (obsolete) —
A number of these show up in an LSAN run of M5.  See the attached logs, though the stacks all look similar.
These also show up in BC3, which is leaking at least half a meg of stuff.
Whiteboard: [lsan] → [lsan][MemShrink]
Attached file leaking stacks
Attachment #8411388 - Attachment is obsolete: true
Attached patch plarena-pkix (obsolete) — Splinter Review
Assignee: nobody → cviecco
Attachment #8435230 - Flags: review?(dkeeler)
Comment on attachment 8435230 [details] [diff] [review]
plarena-pkix

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

Great. Thanks, NSS.
r=me with comments addressed.

::: security/pkix/include/pkix/pkixtypes.h
@@ +34,5 @@
>  
>  namespace mozilla { namespace pkix {
>  
> +inline void
> +ArenaFalseCleaner(PLArenaPool *arena) {

PORT_FreeArena_false(PLArenaPool* arena) {

Also include a comment like "PL_FreeArenaPool can't be used because it doesn't actually free the memory, which doesn't work well with memory analysis tools"
Attachment #8435230 - Flags: review?(dkeeler) → review+
keeping r+ from keeler.
Attachment #8435230 - Attachment is obsolete: true
Attachment #8435267 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4058f94dc21a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8435267 [details] [diff] [review]
plarena-pkix (v1.1)

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

::: security/pkix/include/pkix/pkixtypes.h
@@ +34,5 @@
>  
>  namespace mozilla { namespace pkix {
>  
> +inline void
> +ArenaFalseCleaner(PLArenaPool *arena) {

It looks like I wasn't clear in my review. I'm still concerned about two issues here:
1. The argument list should be "PLArenaPool* arena" to be in line with our agreed-upon style.
2. My understanding is that a "cleaner" is a slightly different paradigm (i.e. using a separate object that, when it goes out of scope, cleans up state for the target object). For our scoped types, we don't have the second object, so I'm not sure the name makes much sense. When we have needed to define a wrapper around an NSS type destructor, we've used the style NSS_destructorName_param (e.g. "PK11_DestroyContext_true"). See ScopedNSSTypes.h. Thus, I think this function should be called PORT_FreeArena_false. This has the added benefit of making it a bit more clear in the typedef how this scoped type gets destructed.
I would appreciate it if you pushed a follow-up patch to fix this (preferably before next week's merge).
Please see comment 9.
Flags: needinfo?(cviecco)
Filed up Bug 1021797 to address. Sorry if I misread the review.
Flags: needinfo?(cviecco)
Does this need to be backported anywhere earlier than 32?
Looks like it landed in 29: http://hg.mozilla.org/mozilla-central/rev/a8cbe1b47eb8
NB: If anyone prepares a branch patch, it should be folded with the patch from bug 1021797.
This should be uplifted to 31 when this became part of the default path (mozilla::pkix)
Comment on attachment 8435267 [details] [diff] [review]
plarena-pkix (v1.1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 915930 
User impact if declined: Memory leak due to incorrect cleanup call in scoped pointer.
Testing completed (on m-c, etc.):  Yes it landed on 32 on 2006-06-05 but forgot to uplift to the then aurora(31) now beta
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None
Attachment #8435267 - Flags: approval-mozilla-beta?
Camilo, it might be better to prepare a new branch patch with the patch from bug 1021797 before asking for uplift approval. Ryan generally checks in approved patches if they apply cleanly, and I would like this to land on beta with the fixup.
Flags: needinfo?(cviecco)
Comment on attachment 8435267 [details] [diff] [review]
plarena-pkix (v1.1)

Agreed, removing the approval request (for now)
Attachment #8435267 - Flags: approval-mozilla-beta?
Flags: needinfo?(cviecco)
beta try(with folding of 1021797)
https://tbpl.mozilla.org/?tree=Try&rev=4ac6f2ca2895
Comment on attachment 8440152 [details] [diff] [review]
plarena-fix-FF-31

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  915930
User impact if declined: Memory leak during certificate validation
Testing completed (on m-c, etc.): Landed in central just before uplift.
Risk to taking this patch (and alternatives if risky):  Little risk, the change is on what function is called for the scoped pointer cleaner for plarenas. 
String or IDL/UUID changes made by this patch: None
Attachment #8440152 - Flags: review+
Attachment #8440152 - Flags: approval-mozilla-beta?
Attachment #8440152 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [lsan][MemShrink] → [lsan][MemShrink][qa-]
You need to log in before you can comment on or make changes to this bug.