[Bug 20684] New: Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Summary: Memory leak in CryptGenKey/CryptDestroyKey?
           Product: Wine
           Version: 1.1.32
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Keywords: download, source, testcase
          Severity: normal
          Priority: P2
         Component: rsaenh
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

Created an attachment (id=24711)
 --> (http://bugs.winehq.org/attachment.cgi?id=24711)
minimal test showing problem

Chromium's unit_tests.exe's SignatureCreatorTest.BasicTest
seems to show a leak in CryptGenKey():

972 bytes in 1 blocks are definitely lost in loss record 836 of 921
   at RtlAllocateHeap (heap.c:1423)
   by ???
   by ???
   by ???
   by CryptGenKey (crypt.c:1434)
   by base::RSAPrivateKey::Create (rsa_private_key_win.cc:35)
   by SignatureCreatorTest_BasicTest_Test::TestBody
(signature_creator_unittest.cc:15)

I've extracted a minimal test case into the attached patch.  
To repeat, apply the patch and then run the rsaenh test under valgrind.
It complains

972 bytes in 1 blocks are definitely lost in loss record 463 of 518
   at RtlAllocateHeap (heap.c:1423)
   by new_object (handle.c:362)
   by new_key (rsaenh.c:834)
   by RSAENH_CPGenKey (rsaenh.c:3064)
   by CryptGenKey (crypt.c:1434)
   by func_rsaenh (rsaenh.c:2546)
   by run_test (test.h:535)
   by main (test.h:585)

It seems the reference count of the generated key starts off one
too high, or something?

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

--- Comment #1 from Juan Lang   2009-11-12 20:05:47 ---
Shouldn't you be calling CryptReleaseContext before deleting the keyset?

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |INVALID

--- Comment #2 from Juan Lang   2009-11-13 10:37:21 ---
Let me rephrase that not to be rhetorical:  you should be calling
CryptReleaseContext.  That's the source of the leak.

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Dan Kegel  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |

--- Comment #3 from Dan Kegel   2009-11-13 12:51:32 ---
Adding that to the little test case gets rid of the leak there,
but the real code already has the proper number of calls to
CryptReleaseContext, and leaks anyway, so maybe something more subtle is
going on.

To repeat:
$ wget http://kegel.com/wine/chromium/chromium-tests.tar.bz2
$ tar -xjvf chromium-tests.tar.bz2
$ valgrind --leak-check=full --trace-children=yes wine
src/chrome/Debug/base_unittests --gtest_filter=SignatureCreatorTest.*

I added log statements to show where the keys and context were acquired and
released, and will attach log files.

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Dan Kegel  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24711|0                           |1
        is obsolete|                            |

--- Comment #4 from Dan Kegel   2009-11-13 12:53:46 ---
Created an attachment (id=24720)
 --> (http://bugs.winehq.org/attachment.cgi?id=24720)
Output of valgrind --trace-children=yes --leak-check=full wine
base_unittests.exe --gtest_filter=SignatureCreatorTest.*

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

--- Comment #5 from Dan Kegel   2009-11-13 12:56:06 ---
Created an attachment (id=24721)
 --> (http://bugs.winehq.org/attachment.cgi?id=24721)
rzipped log of WINEDEBUG=+crypt,+handle,warn+heap,+relay wine
base_unittests.exe --gtest_filter=SignatureCreatorTest.*

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

--- Comment #6 from Juan Lang   2009-11-19 19:59:48 ---
Color me skeptical.  You didn't catch the error in the minimal test case, so
I'm not certain you will have done so in the large test suite.  Static analysis
might be a better tool here.

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |INVALID

--- Comment #7 from Juan Lang   2009-11-19 20:53:05 ---
In fact, I don't think this is an appropriate use of bugzilla.  All you can say
is that something is leaking, but you can't be certain whether it's your test
suite, the chromium code, or rsaenh.  A bug should show a difference in
behavior with Windows, and so far this doesn't.  Feel free to reopen if you can
narrow it down to a (small) test that you're certain shouldn't leak, and we can
investigate that one.

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #8 from Dmitry Timoshkov   2009-11-23 06:33:40 ---
Closing invalid.

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CLOSED                      |REOPENED
         Resolution|INVALID                     |

--- Comment #9 from Juan Lang   2009-12-07 16:07:00 ---
I'm going to reopen this now that Dan's valgrind logs include leaks for the
Wine test suite, and rsaenh shows leaks.  E.g. here's the latest log:
http://kegel.com/wine/valgrind/logs/2009-12-07-08.39/vg-rsaenh_rsaenh.txt

It includes leaks such as:
 972 bytes in 1 blocks are definitely lost 
    at  notify_alloc (heap.c:247)
    by  RtlAllocateHeap (heap.c:1697)
    by  ???
    by  ???
    by  ???
    by  ???
    by  ???
    by  ???
    by  ???
    by  CryptAcquireContextW (crypt.c:505)
    by  CryptAcquireContextA (crypt.c:573)
    by  test_key_initialization (rsaenh.c:2528)
    by  func_rsaenh (rsaenh.c:2564)
    by  run_test (test.h:535)
    by  main (test.h:585)

The referenced line of test_key_initialization is included in this body:
    result = CryptAcquireContext(&prov2, szContainer, szProvider,
PROV_RSA_FULL, 0);
    ok(result, "%08x\n", GetLastError());
    result = CryptGetUserKey(prov2, AT_KEYEXCHANGE, &hKey);
    ok(result, "%08x\n", GetLastError());
    if (result) CryptDestroyKey(hKey);
    CryptReleaseContext(prov2, 0);

It appears to me as though the reference counting is valid here, so the report
of a leak seems accurate.

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

--- Comment #10 from Juan Lang   2009-12-09 14:14:59 ---
Commit 035c323054c23c83203b7ac9b8d9bc7a050336ba improved one leak (in
CPSignHash), and commit d69b00d630200ce013a3a8a97eea987e1165e89a removed some
leaks in the tests, see the diffs in the subsequent log:
http://kegel.com/wine/valgrind/logs/2009-12-07-08.39/diff-rsaenh_rsaenh.txt
(Ignore all the reads of uninitialized values, those were likely the result of
a slightly broken build.)

And commits 71880e48184ec66fbb04d074404c5c3e8f32fe35 and
71b4ac9c71dfa7736a7e7cff09d5899c209fd7fe seem to have helped too, see the diffs
in today's log:
http://kegel.com/wine/valgrind/logs/2009-12-09-09.00/diff-rsaenh_rsaenh.txt

Still, a few seem to remain:
http://kegel.com/wine/valgrind/logs/2009-12-09-09.00/vg-rsaenh_rsaenh.txt

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

--- Comment #11 from Juan Lang   2009-12-09 16:45:43 ---
I sent a patch which I hope will fix the remaining leaks:
http://www.winehq.org/pipermail/wine-patches/2009-December/082514.html

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

--- Comment #12 from Juan Lang   2009-12-10 11:49:28 ---
Patch was committed (f9a475c808b3bbc4980f3225dfa2a7397df9f4db), awaiting new
results with baited breath :)

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #13 from Juan Lang   2009-12-10 22:53:57 ---
This appears to be fixed.  Today's valgrind logs
(http://kegel.com/wine/valgrind/logs/2009-12-10-17.26/) show no errors in the
rsaenh tests, and the majority of the crypt32 errors have disappeared too.

[Bug 20684] Memory leak in CryptGenKey/CryptDestroyKey?

http://bugs.winehq.org/show_bug.cgi?id=20684

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #14 from Alexandre Julliard   2009-12-18 13:07:53 ---
Closing bugs fixed in 1.1.35.