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

[Dmitry Dmitriev]

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
>