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

[Ioi Lam]

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