memory leak in pkix_CheckCert function

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 315876 [details] [diff] [review]
Patch v1(remove debug code that leaks memory)

the leak was detected by pkix object leak test. Here is the stack:

        [1] PR_Malloc() at line 467 in "prmem.c"
        [2] PKIX_PL_Malloc() at line 72 in "pkix_pl_mem.c"
        [3] pkix_UTF16_to_EscASCII() at line 697 in "pkix_pl_common.c"
        [4] PKIX_PL_String_GetEncoded() at line 616 in "pkix_pl_string.c"
        [5] pkix_CheckCert() at line 282 in "pkix_validate.c"
        [6] pkix_CheckChain() at line 955 in "pkix_validate.c"
        [7] pkix_Build_ValidateEntireChain() at line 1631 in "pkix_build.c"
        [8] pkix_BuildForwardDepthFirstSearch() at line 2852 in "pkix_build.c"
        [9] pkix_Build_InitiateBuildChain() at line 4193 in "pkix_build.c"
        [10] PKIX_BuildChain() at line 4383 in "pkix_build.c"
        [11] cert_BuildAndValidateChain() at line 793 in "certvfypkix.c"
        [12] cert_VerifyCertChainPkix() at line 1236 in "certvfypkix.c"
        [13] cert_VerifyCertChain() at line 870 in "certvfy.c"
        [14] CERT_VerifyCertificate() at line 1288 in "certvfy.c"
        [15] main() at line 407 in "vfychain.c"

The leak is in debug code that logs cert validation error to a logger.
Attachment #315876 - Flags: review?(nelson)
(Assignee)

Updated

9 years ago
Blocks: 397832
Priority: -- → P1
Whiteboard: PKIX
(Assignee)

Comment 1

9 years ago
Created attachment 316023 [details] [diff] [review]
Resub the patch. Try to reproduce bugzilla bug.
Attachment #316023 - Flags: review?(nelson)
(Assignee)

Comment 2

9 years ago
Comment on attachment 316023 [details] [diff] [review]
Resub the patch. Try to reproduce bugzilla bug. 

here is the comment.
Attachment #316023 - Flags: review?(nelson)
(Assignee)

Comment 3

9 years ago
Created attachment 316035 [details] [diff] [review]
One more test patch

The patch to fix mem leak
        [1] PR_Malloc() at line 467 in "prmem.c"
        [2] PKIX_PL_Malloc() at line 72 in "pkix_pl_mem.c"
        [3] pkix_UTF16_to_EscASCII() at line 697 in "pkix_pl_common.c"
        [4] PKIX_PL_String_GetEncoded() at line 616 in "pkix_pl_string.c"
        [5] pkix_CheckCert() at line 282 in "pkix_validate.c"
        [6] pkix_CheckChain() at line 955 in "pkix_validate.c"
        [7] pkix_Build_ValidateEntireChain() at line 1631 in "pkix_build.c"
        [8] pkix_BuildForwardDepthFirstSearch() at line 2852 in "pkix_build.c"
        [9] pkix_Build_InitiateBuildChain() at line 4193 in "pkix_build.c"
        [10] PKIX_BuildChain() at line 4383 in "pkix_build.c"
        [11] cert_BuildAndValidateChain() at line 793 in "certvfypkix.c"
        [12] cert_VerifyCertChainPkix() at line 1236 in "certvfypkix.c"
        [13] cert_VerifyCertChain() at line 870 in "certvfy.c"
        [14] CERT_VerifyCertificate() at line 1288 in "certvfy.c"
        [15] main() at line 407 in "vfychain.c"
Attachment #316023 - Attachment is obsolete: true
Attachment #316035 - Flags: review?(nelson)
Attachment #316035 - Attachment is obsolete: true
Attachment #316035 - Flags: review?(nelson)
Attachment #315876 - Flags: review?(nelson) → review+
(Assignee)

Comment 4

9 years ago
patch is integrated.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

9 years ago
Comment on attachment 315876 [details] [diff] [review]
Patch v1(remove debug code that leaks memory)

Nominating the bug for 3.12.0. 

checkerCheck(a pointer to pkix_PolicyChecker_Check) will throw an error in case when cert does not pass the check. In other words a leak is created every time when cert fails validation.
Attachment #315876 - Flags: superreview?(kengert)

Comment 6

9 years ago
Comment on attachment 315876 [details] [diff] [review]
Patch v1(remove debug code that leaks memory)

You remove a lot of code, is that all equivalent to PKIX_CHECK ?
Attachment #315876 - Flags: superreview?(kengert) → superreview+

Comment 7

9 years ago
Adding Bob to CC list, in order to give him a chance to veto 3.12.0 branch landing.
(Assignee)

Comment 8

9 years ago
(In reply to comment #6)
> (From update of attachment 315876 [details] [diff] [review])
> You remove a lot of code, is that all equivalent to PKIX_CHECK ?
> 
Kai, PKIX_CHECK does not do error conversion to a string and it does not log it. The removed code should only be executed for debug purposes. In other cases PKIX_CHECK will do the job.

Comment 9

9 years ago
"The leak is in debug code that logs cert validation error to a logger."

Does the leak exist if DEBUG isn't defined? (It looks like by debug code you mean code that's always on for debugging, not code that is controlled by ifdef DEBUG).

Anyway if the leak can occur in Optimized builds, then this patch looks pretty safe (the only error that patch could have is not freeing enough in the error path, which is the current case) -- ok for 3.12.0.

If the leak only exists in DEBUG builds, I would be inclined not to take it for 3.12.0. 

Definitely take this patch for 3.12.1 of course;).

bob.
You need to log in before you can comment on or make changes to this bug.