RFR: 8132725: Memory leak in Arguments::add_property function
Ioi Lam
ioi.lam at oracle.com
Thu Aug 27 05:27:59 UTC 2015
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"?
- 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
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list