Closed Bug 429230 Opened 16 years ago Closed 16 years ago

memory leak in pkix_CheckCert function

Categories

(NSS :: Libraries, defect, P1)

3.12
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alvolkov.bgs, Assigned: alvolkov.bgs)

References

Details

(Whiteboard: PKIX)

Attachments

(1 file, 2 obsolete files)

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)
Blocks: 397832
Priority: -- → P1
Whiteboard: PKIX
Attachment #316023 - Flags: review?(nelson)
Comment on attachment 316023 [details] [diff] [review]
Resub the patch. Try to reproduce bugzilla bug. 

here is the comment.
Attachment #316023 - Flags: review?(nelson)
Attached patch One more test patch (obsolete) — Splinter Review
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+
patch is integrated.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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 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+
Adding Bob to CC list, in order to give him a chance to veto 3.12.0 branch landing.
(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.
"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.

Attachment

General

Creator:
Created:
Updated:
Size: