Last Comment Bug 429230 - memory leak in pkix_CheckCert function
: memory leak in pkix_CheckCert function
Status: RESOLVED FIXED
PKIX
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P1 normal (vote)
: 3.12
Assigned To: Alexei Volkov
:
Mentors:
Depends on:
Blocks: 397832
  Show dependency treegraph
 
Reported: 2008-04-15 16:49 PDT by Alexei Volkov
Modified: 2008-04-22 15:47 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1(remove debug code that leaks memory) (2.42 KB, patch)
2008-04-15 16:49 PDT, Alexei Volkov
nelson: review+
kaie: superreview+
Details | Diff | Review
Resub the patch. Try to reproduce bugzilla bug. (2.42 KB, patch)
2008-04-16 09:11 PDT, Alexei Volkov
no flags Details | Diff | Review
One more test patch (2.42 KB, patch)
2008-04-16 09:36 PDT, Alexei Volkov
no flags Details | Diff | Review

Description Alexei Volkov 2008-04-15 16:49:49 PDT
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.
Comment 1 Alexei Volkov 2008-04-16 09:11:10 PDT
Created attachment 316023 [details] [diff] [review]
Resub the patch. Try to reproduce bugzilla bug.
Comment 2 Alexei Volkov 2008-04-16 09:26:13 PDT
Comment on attachment 316023 [details] [diff] [review]
Resub the patch. Try to reproduce bugzilla bug. 

here is the comment.
Comment 3 Alexei Volkov 2008-04-16 09:36:30 PDT
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"
Comment 4 Alexei Volkov 2008-04-16 13:58:22 PDT
patch is integrated.
Comment 5 Alexei Volkov 2008-04-18 15:46:09 PDT
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) 2008-04-22 08:04:09 PDT
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) 2008-04-22 08:05:32 PDT
Adding Bob to CC list, in order to give him a chance to veto 3.12.0 branch landing.
Comment 8 Alexei Volkov 2008-04-22 13:59:36 PDT
(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 2008-04-22 15:47:00 PDT
"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.

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