#22 Memory leaks when using EnumVariant

1.12 Accepted
closed-accepted
nobody
COM Data (10)
5
2007-03-27
2007-03-05
Andreas Junghans
No

In the native implementation that handles EnumVariants there are two memory leaks.

The first one is in Variant.cpp where there is an extra AddRef (which must not be there because a previous QueryInterface call already incremented the reference).

The second one is in EnumVariant.cpp where a VariantClear call is missing. According to MS documentation, Variants received while iterating IEnumVARIANT must be cleared when no longer needed. The Variant value is copied in EnumVariant.cpp (which itself could probably be optimized - maybe I'll look at this later). After that, the original value is no longer needed, so it must be cleared.

I've performed some tests, and both are actual leaks, not just theoretical ones. The attached patch (against release 1.11.1) fixes them (also reported as bugs 1112667, 1465539, and 1569864).

Discussion

1 2 > >> (Page 1 of 2)
  • Patch against 1.11.1 release

     
  • clay_shooter
    clay_shooter
    2007-03-10

    Logged In: YES
    user_id=1189284
    Originator: NO

    Do you have any test cases that show this that the leak exists and has been fixed? I'm hesitant to accept a patch like this without test code.

     
  • clay_shooter
    clay_shooter
    2007-03-10

    Logged In: YES
    user_id=1189284
    Originator: NO

    Patch accepted but we really need test cases for this. It will show up in 1.12 prerelease 1

     
  • clay_shooter
    clay_shooter
    2007-03-10

    • milestone: --> 1.12 Accepted
    • labels: --> COM Data
    • status: open --> pending-accepted
     
  • Logged In: YES
    user_id=718191
    Originator: YES

    Here's a simple test. It should work on any Windows 2000 machine or up (you may have to be logged in as Administrator). Just take a look at the memory consumed by the Java process during the sleeping phase. The difference with and without the patch is obvious (you can also let the loop run more often to get a more pronounced effect).

    It's somewhat hard to reproduce the leak with arbitrary enums. This is most likely caused by caching inside the COM objects implementing IEnumVARIANT (so that there are only a few objects leaked, even if the iteration is performed thousands of times). However, it's easily reproducible with WMI scripting (as in the test code).

    Also, both changes in the patch are clearly necessary according to Microsoft's documentation:

    - "This function must call IUnknown::AddRef on the pointer it returns." (As a note to implementers of IUnknown::QueryInterface, http://msdn2.microsoft.com/en-us/library/ms682521.aspx\) This means that the additional AddRef in Variant.cpp/toEnumVariant is harmful, because no corresponding Release is ever called.

    - "To avoid problems when dealing with IEnumVARIANT, it is recommended that Skip() function is not used and AccessibleChildren() function with a non-zero iChildStart value is not called. Instead, use the AccessibleChildren() with an iChildStart of 0 to get all the children, or use IEnumVARIANT::Next() to retrieve them one by one (or in chunks), and manually skip over the ones that are not needed. ___VariantClear() must be called to clear the unused variants.___" (emphasis mine, http://support.microsoft.com/?scid=kb%3Ben-us%3B228504&x=14&y=11\)

    In the case of JACOB's EnumVariant.cpp, _all_ variants are unused (since their values are copied and they are never touched again afterwards), so they all must be cleared. And there definitly is a leak if they're not.
    File Added: TestEnumLeak.java

     
  •  
    Attachments
    • status: pending-accepted --> open-accepted
     
  • Logged In: YES
    user_id=718191
    Originator: YES

    Sorry, didn't mean to change the status.

     
    • status: open-accepted --> pending-accepted
     
  • Logged In: YES
    user_id=1312539
    Originator: NO

    This Tracker item was closed automatically by the system. It was
    previously set to a Pending status, and the original submitter
    did not respond within 14 days (the time period specified by
    the administrator of this Tracker).

     
1 2 > >> (Page 1 of 2)