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

[David Holmes]

On 27/08/2015 3:27 PM, Ioi Lam wrote:
> On the topic of goto, some people like to do this:
>
>     do {
>          if (...) {
>             break;
>          }
>          ...
>          if (...) {
>             break;
>          }
>     } while (0);
>     // "break" will "goto" here
>
> Will this be less of an eyesore than "goto"?

No! A goto by any other name ... :) Might as well just use a goto if you 
are going to resort to such an ugly structure just to avoid using goto.

David

> - Ioi
>
>
> On 8/26/15 2:57 PM, Coleen Phillimore wrote:
>>
>> +    char* tmp_key = AllocateHeap(key_len + 1, mtInternal);
>> +
>> +    if (tmp_key == NULL) {
>> +      return false;
>>    }
>>
>> AllocateHeap will call vm_exit_out_of_memory if it fails, and not
>> return NULL. You have to add AllocFailStrategy::RETURN_NULL
>>
>> Otherwise, this seems good.
>>
>> Thanks for not adding a goto.
>>
>> Coleen
>>
>>
>> On 8/26/15 2:05 PM, Dmitry Dmitriev wrote:
>>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>