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

[Dmitry Dmitriev]

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
>>>
>>
>