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

[Jiangli Zhou]

Hi Dmitry,

My comment is really just an opinion about the style. The ‘key’ only needs to be a separate string when calling PropertyList_unique_add(), if the strcmp() comparisons are changed to strncmp() with ‘prop’ as the argument. That means we don’t need to allocate the ‘key' string until PropertyList_unique_add() is called. And the ‘key’ can be freed right after the PropertyList_unique_add() call. That probably would make the code more easier to read. Then the ‘_java_command_new’ and ‘_java_vendor_utl_bug_new’ allocation failure cases can just return with ‘false’ immediately. The ‘status’ would probably no longer be needed. As there is no issue with the correctness, please fell free to keep the existing code.

One minor issue with the code is the indentation at line 1023.

Thanks,
Jiangli


On Aug 24, 2015, at 6:21 AM, Dmitry Dmitriev <dmitry.dmitriev at oracle.com> 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