RFR: 8132725: Memory leak in Arguments::add_property function

[Dmitry Dmitriev]

Hello,

Still need a Reviewer. Can someone review this patch? Thank you!

Dmitry

On 25.08.2015 15:27, Dmitry Dmitriev wrote:
> Hi Ioi,
>
> Thank you for review and sponsorship! Still need a Reviewer please.
>
> I added assert. Also I fix indention on line 1023 and change "char 
> *var_name" to "char* var_name" to match style which used in this 
> function.
>
> webrev 02: http://cr.openjdk.java.net/~ddmitriev/8132725/webrev.02/ 
> <http://cr.openjdk.java.net/%7Eddmitriev/8132725/webrev.02/>
> webrev 02 vs 01: 
> http://cr.openjdk.java.net/~ddmitriev/8132725/webrev.02.vs.01/ 
> <http://cr.openjdk.java.net/%7Eddmitriev/8132725/webrev.02.vs.01/>
>
> Thanks,
> Dmitry
>
> On 24.08.2015 20:42, Ioi Lam wrote:
>> Hi Dmitry,
>>
>> The new changes look good.
>>
>> For defensive programming, I would suggest adding an assert here:
>>
>> 1035         if (_java_vendor_url_bug != DEFAULT_VENDOR_URL_BUG) {
>>                     assert(_java_vendor_url_bug != NULL, "......");
>> 1036           os::free((void *)_java_vendor_url_bug);
>>
>> I can sponsor the change, but we still need a Reviewer for this change.
>>
>> Thanks
>> - Ioi
>>
>> On 8/24/15 6:21 AM, Dmitry Dmitriev wrote:
>>> Hi Ioi,
>>>
>>> Thank you for comments! Please, see my answers inline.
>>>
>>> On 24.08.2015 2:13, Ioi Lam wrote:
>>>> Hi Dmitry,
>>>>
>>>> Is this change part of 8132725?
>>>>
>>>> 3904   jint code = set_aggressive_opts_flags();
>>>> 3905   if (code != JNI_OK) {
>>>> 3906     return code;
>>>> 3907   }
>>> Yes, set_aggressive_opts_flags not check return value of 
>>> add_property function, so I add check to the 
>>> set_aggressive_opts_flags()(lines 1911-1913 in new arguments.cpp) 
>>> and thus now it returns jint.
>>>
>>>>
>>>>
>>>> 1041       if (_java_vendor_url_bug != DEFAULT_VENDOR_URL_BUG) {
>>>>
>>>> >> also check (_java_vendor_url_bug != NULL) for sanity?
>>> I think that this is unnecessary in this case, because 
>>> _java_vendor_url_bug can not be NULL. _java_vendor_url_bug 
>>> initialized to DEFAULT_VENDOR_URL_BUG and changed only in 
>>> add_property function. Before new value is assigned to 
>>> _java_vendor_url_bug it's check for not NULL. Thus, I think that 
>>> check (_java_vendor_url_bug != NULL) is unnecessary in this case.
>>>
>>>>
>>>>
>>>> Also, there's a lot of duplicated "if (eq != NULL) { FreeHeap((void 
>>>> *)key);}". Maybe these can be consolidated with a "goto"? I know 
>>>> lots of people haye goto but it will make the clean up less error 
>>>> prone:
>>> Thank you for this proposal. Since "goto" is not widely used in 
>>> Hotspot code I decided to refactor current implementation to avoid 
>>> duplication of "if (eq != NULL) { FreeHeap((void *)key);}".
>>>
>>>>
>>>> bool Arguments::add_property(const char* prop) {
>>>>     ....
>>>>     bool status = false;
>>>>     ....
>>>>        char *_java_command_new = os::strdup(value, mtInternal);
>>>>        if (_java_command_new == NULL) {
>>>>          goto done;
>>>>        }else {
>>>>          if (_java_command != NULL) {
>>>>            os::free(_java_command);
>>>>          }
>>>>          _java_command = _java_command_new;
>>>>        }
>>>>     ....
>>>>     }
>>>>     // Create new property and add at the end of the list
>>>>     PropertyList_unique_add(&_system_properties, key, value);
>>>>   }
>>>>   status = true;
>>>>
>>>> done:
>>>>    if (key != prop) {
>>>>      // SystemProperty copy passed value, thus free previously 
>>>> allocated
>>>>      // memory
>>>>     FreeHeap((void *)key);
>>>>    }
>>>>    return status;
>>>> }
>>>>
>>>> Also, using (key != prop) would make the code clearer than (eq != 
>>>> NULL).
>>> Fixed!
>>>
>>> webrev 01: http://cr.openjdk.java.net/~ddmitriev/8132725/webrev.01/ 
>>> <http://cr.openjdk.java.net/%7Eddmitriev/8132725/webrev.01/>
>>> webrev 01 vs 00: 
>>> http://cr.openjdk.java.net/~ddmitriev/8132725/webrev.01.vs.00/ 
>>> <http://cr.openjdk.java.net/%7Eddmitriev/8132725/webrev.01.vs.00/>
>>>
>>> Thank you,
>>> Dmitry
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>> On 8/13/15 12:55 AM, Dmitry Dmitriev wrote:
>>>>> Hello,
>>>>>
>>>>> Please review this fix which remove memory leak in 
>>>>> Arguments::add_property function. Also, I need a sponsor for this 
>>>>> fix, who can push it.
>>>>>
>>>>> Arguments::add_property function allocate memory for key and 
>>>>> value. Then key and values are passed to the 
>>>>> PropertyList_unique_add function which use SystemProperty class to 
>>>>> add or update property value. SystemProperty class maintains it's 
>>>>> own copy of key and value and thus copy passed key and value. 
>>>>> Therefore key and value must be freed in add_property 
>>>>> function(with exception for value in case of "java.vendor.url.bug" 
>>>>> and "sun.java.command" properties).
>>>>>
>>>>> In this fix I allocate memory only for key when passed property 
>>>>> contains value. If passed property not contains value, then I not 
>>>>> allocate memory for key and use passed property string. Value also 
>>>>> extracted from passed property string instead of allocating. To 
>>>>> accomplish that I changed declaration of "value" in several 
>>>>> functions from "char *" to  "const char *" since value is not 
>>>>> modified in these functions(PropertyList_* functions, 
>>>>> SystemProperty class methods).
>>>>>
>>>>> Processing of "java.vendor.url.bug" and "sun.java.command" 
>>>>> properties also corrected. Now when these properties redefined, 
>>>>> then code checks if memory was allocated for special variables of 
>>>>> these properties(checking that not contains default value) and 
>>>>> free it.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~ddmitriev/8132725/webrev.00/ 
>>>>> <http://cr.openjdk.java.net/%7Eddmitriev/8132725/webrev.00/>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8132725
>>>>> Tested: JPRT(hotspot test set), hotspot all, vm.quick
>>>>>
>>>>> Thanks,
>>>>> Dmitry
>>>>
>>>
>>
>