429230 - memory leak in pkix_CheckCert function

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

Attachment: 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.

Comment 1

[Alexei Volkov]

Attachment: Resub the patch. Try to reproduce bugzilla bug.

Comment 2

[Alexei Volkov]

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

here is the comment.

Comment 3

[Alexei Volkov]

Attachment: 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"

Comment 4

[Alexei Volkov]

patch is integrated.

Comment 5

[Alexei Volkov]

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.

Comment 6

[Kai Engert [:KaiE:]]

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 ?

Comment 7

[Kai Engert [:KaiE:]]

Adding Bob to CC list, in order to give him a chance to veto 3.12.0 branch landing.

Comment 8

[Alexei Volkov]

(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

[Robert Relyea]

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