1000548 - Leaking arenas allocated in mozilla::pkix::CheckIssuerIndependentProperties

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Andrew McCreight [:mccr8]]

Attachment: stacks for leaking allocations

A number of these show up in an LSAN run of M5.  See the attached logs, though the stacks all look similar.

Comment 1

[Andrew McCreight [:mccr8]]

These also show up in BC3, which is leaking at least half a meg of stuff.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: leaking stacks

Comment 3

[Camilo Viecco (:cviecco)]

Attachment: plarena-pkix

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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"

Comment 5

[Camilo Viecco (:cviecco)]

 https://tbpl.mozilla.org/?tree=Try&rev=97a8f8f5e95a

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: plarena-pkix (v1.1)

keeping r+ from keeler.

Comment 7

[Camilo Viecco (:cviecco)]

pushed to inbound
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4058f94dc21a

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4058f94dc21a

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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).

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Please see comment 9.

Comment 11

[Camilo Viecco (:cviecco)]

Filed up Bug 1021797 to address. Sorry if I misread the review.

Comment 12

[Andrew McCreight [:mccr8]]

Does this need to be backported anywhere earlier than 32?

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 14

[Camilo Viecco (:cviecco)]

This should be uplifted to 31 when this became part of the default path (mozilla::pkix)

Comment 15

[Camilo Viecco (:cviecco)]

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

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 17

[Camilo Viecco (:cviecco)]

Comment on attachment 8435267 [details] [diff] [review]
plarena-pkix (v1.1)

Agreed, removing the approval request (for now)

Comment 18

[Camilo Viecco (:cviecco)]

beta try(with folding of 1021797)
https://tbpl.mozilla.org/?tree=Try&rev=4ac6f2ca2895

Comment 19

[Camilo Viecco (:cviecco)]

Attachment: plarena-fix-FF-31

Comment 20

[Camilo Viecco (:cviecco)]

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

Comment 21

[Camilo Viecco (:cviecco)]

pushed to beta (ff31): 
 https://hg.mozilla.org/releases/mozilla-beta/rev/76fe5489c1d9