RFR: 8132725: Memory leak in Arguments::add_property function
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Aug 26 21:57:27 UTC 2015
+ 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