RFR: 8132725: Memory leak in Arguments::add_property function
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Thu Aug 27 14:31:26 UTC 2015
Hi Ioi,
In this fix I changed 'key' type from 'char*' to 'const char*'. This
allow me to assign without casting passed 'prop' value to the 'key' if
'prop' doesn't contain a value. Thus, I need a temporary variable to
extract key if passed property contains value.
So, to get rid of 'tmp_key' I need to change type of the 'key' back to
'char*' and cast 'prop' to 'char*' in case when no value is passed in
'prop'. But I think it safer to leave 'key' as 'const char*'. What you
think about that?
Thank you,
Dmitry
On 27.08.2015 17:21, Ioi Lam wrote:
> Hi Dmitry,
>
> Maybe you can also get rid of tmp_key?
>
> - Ioi
>
> On 8/27/15 4:43 AM, Dmitry Dmitriev wrote:
>> Hello Coleen,
>>
>> Thank you for review and hint about AllocateHeap. I remove check for
>> 'tmp_key'. In this case new code behave as old code, i.e. call
>> 'vm_exit_out_of_memory' if it fails. Also, I change 'os::strdup' in
>> 'add_property' function to 'os::strdup_check_oom' to achieve the same
>> thing, i.e. behave as old code. In these case we don't need 'status'
>> variable.
>>
>> webrev 03: http://cr.openjdk.java.net/~ddmitriev/8132725/webrev.03/
>> <http://cr.openjdk.java.net/%7Eddmitriev/8132725/webrev.03/>
>> webrev 03 vs 02:
>> http://cr.openjdk.java.net/~ddmitriev/8132725/webrev.03.vs.02/
>> <http://cr.openjdk.java.net/%7Eddmitriev/8132725/webrev.03.vs.02/>
>>
>> Thank you,
>> Dmitry
>>
>> On 27.08.2015 0:57, 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list