RFR: 8132725: Memory leak in Arguments::add_property function
Ioi Lam
ioi.lam at oracle.com
Thu Aug 27 14:35:59 UTC 2015
Hmmm, that sounds fine then. Thanks for the explanation.
- Ioi
On 8/27/15 7:31 AM, Dmitry Dmitriev wrote:
> 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