RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Dmitry Samersoff
dmitry.samersoff at oracle.com
Thu Mar 27 19:16:15 UTC 2014
Vladimir,
I can push it to RT next week.
-Dmitry
On 2014-03-27 23:01, Vladimir Kozlov wrote:
> On 3/27/14 11:53 AM, Dmitry Samersoff wrote:
>> Goetz,
>>
>> Looks good for me!
>
> +1.
>
> Dmitry,
>
> Do you want me to push it into Comp repo or you will do it into RT repo?
>
> Thanks,
> Vladimir
>
>>
>> -Dmitry
>>
>>
>> On 2014-03-27 18:56, Lindenmaier, Goetz wrote:
>>> Hi Dmitry,
>>>
>>> thanks for the thorough review and catching the forgotten FREEs!
>>> I had a look at set_boot_path(). That will never return false,
>>> as it also calls NEW_C_HEAP_ARRAY that will abort the VM.
>>> In addition it would be wrong to return, there should be a
>>> call to vm_exit_...
>>> So I removed the test and return statement.
>>> I added the other missing FREEs.
>>>
>>> I also implemented your proposal with only one sprintf
>>> for the ld_library_path, and did so for bsd, aix and linux
>>> to get it more similar. Solaris seems too different to
>>> adapt to that scheme.
>>>
>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.03/
>>>
>>> Best regards,
>>> Goetz.
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>>> Sent: Donnerstag, 27. März 2014 10:49
>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net;
>>> ppc-aix-port-dev at openjdk.java.net
>>> Subject: Re: RFR(M): 8038201: Clean up misleading usage of malloc()
>>> in init_system_properties_values()
>>>
>>> Goetz,
>>>
>>> Thank you for doing it - code is much cleaner now.
>>>
>>> os_aix.cpp:
>>> Did you forget FREE_C_HEAP_ARRAY() at ll.548 and ll.576?
>>>
>>> os_bsd.cpp:
>>> missed FREE_C_HEAP_ARRAY() at ll. 389, ll. 477
>>>
>>> ll.407 sprintf could be moved to ll.420 under else
>>>
>>>
>>> ll.497 (and below) a trick like one below:
>>>
>>> char *l = ::getenv("JAVA_LIBRARY_PATH");
>>> char *v = ::getenv("DYLD_LIBRARY_PATH");
>>> char *v_colon = ":";
>>>
>>> if (l != NULL || v != NULL) {
>>>
>>> if (v == NULL){ v = "", v_colon = ""; };
>>> if (l == NULL){ l = "", v_colon = ""; };
>>>
>>> t = (char *) ... /* allocate memory for all strings */
>>>
>>> sprintf(t, "%s%s%s:%s", v,v_colon, l, ld_library_path);
>>> if (ld_library_path != buf){ ... /* free ld_library_path */ }
>>> ld_library_path = t;
>>> }
>>>
>>> might be useful to assemble the path in one shot,
>>> without extra copying
>>>
>>>
>>> ll.520 As you appending constant value ":.", you can just
>>> take it into account at ll. 495
>>>
>>>
>>> os_linux.cpp:
>>> missed FREE_C_HEAP_ARRAY() at ll. 400
>>>
>>> os_solaris.cpp:
>>> missed FREE_C_HEAP_ARRAY() at ll. 714
>>>
>>>
>>> -Dmitry
>>>
>>> On 2014-03-27 12:54, Lindenmaier, Goetz wrote:
>>>> Hi,
>>>>
>>>> please have a look at this new webrev:
>>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.02/
>>>>
>>>> Dmitry, sorry I forgot to reply to all in my last mail. I copied
>>>> the missing parts in here, below.
>>>>
>>>> Avoiding big stack allocations I wanted to remove the buf[MAXPATHLEN]
>>>> at the beginning of the function, too. That's why I said I can't
>>>> get along
>>>> with a single allocation. strlen(v) is only known later.
>>>>
>>>> I could redesign the whole code, computing sizes first, and then
>>>> doing a single allocation, but I think that doesn't help the
>>>> readability of the code.
>>>>
>>>> So now I compute the max size of all buffers I can estimate at the
>>>> beginning
>>>> and do a single allocation for them.
>>>> Also, bsd contains an extra section for __APPLE__.
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>>
>>>> Goetz,
>>>>
>>>>> The allocation in 556 must know about the length of the
>>>>> getenv("LIBPATH") result, which may contain several paths.
>>>>
>>>> You can use
>>>>
>>>> MAX(
>>>> strlen(v) + 1 + sizeof(DEFAULT_LIBPATH) + 1,
>>>> MAXPATHLEN + sizeof(EXTENSIONS_DIR) + sizeof(ENDORSED_DIR)
>>>> )
>>>>
>>>> as the size for a buffer at ll. 556 and have one allocation.
>>>>
>>>> But I'm fine with two different allocations.
>>>>
>>>> Thank you for addressing other concerns.
>>>>
>>>> -Dmitry
>>>>
>>>> On 2014-03-25 17:55, Lindenmaier, Goetz wrote:
>>>>> Hi Dmitry,
>>>>>
>>>>> see my comments inline.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>>>>>> Sent: Dienstag, 25. März 2014 11:12
>>>>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net;
>>>>>> ppc-aix-port-dev at openjdk.java.net
>>>>>> Subject: Re: RFR(M): 8038201: Clean up misleading usage of
>>>>>> malloc() in init_system_properties_values()
>>>>>>
>>>>>> Goetz,
>>>>>>
>>>>>> os_aix.cpp: 565
>>>>>>
>>>>>> It might be better to allocate one buffer that satisfy all three
>>>>>> sprintf() calls at ll. 556 and free it at ll. 574
>>>>> I need at least two allocations. The first one has a statically
>>>>> known size. At
>>>>> least more or less, as I know there will be only one path of
>>>>> unknown size in the buffer.
>>>>> The allocation in 556 must know about the length of the
>>>>> getenv("LIBPATH") result,
>>>>> which may contain several paths.
>>>>> Also, in the other files, paths are concatenated, so at least two
>>>>> buffers are needed.
>>>>>
>>>>>> os_bsd.cpp: 350
>>>>>>
>>>>>> I would prefer to have the single #ifdef __APPLE__ block, it makes
>>>>>> code much more readable.
>>>>> I'll fix this.
>>>>>
>>>>>> os_bsd.cpp: 485
>>>>>>
>>>>>> you replace malloc to on-stack allocation of more than 4096 bytes. I
>>>>>> think this allocation should be done through NEW_C_HEAP_ARRAY.
>>>>> > see also comments to os_aix above - you can allocate large enough
>>>>>> buffer once and than reuse it.
>>>>> Is there a boundary at 4k? There was a char buf[MAXPATHLEN] in the
>>>>> code anyways, so I thought a few bytes more would not matter.
>>>>> The space required by the existing buf can be reused by the C
>>>>> compiler.
>>>>>
>>>>> But I'll fix that and only use allocated memory.
>>>>> I'll remove the buf that was already there and replace all spaces
>>>>> I can estimate by MAXPATHLEN + some offset by a single, allocated
>>>>> buffer.
>>>>> I'll do this the same on all four platforms.
>>>>>
>>>>>> os_linux.cpp: 434
>>>>>> the same comments as above.
>>>>>>
>>>>>> os_solaris.cpp: 728
>>>>>>
>>>>>> As soon as you doing cleanup, I would love to have this code
>>>>>> changed
>>>>>> a bit. It comes from solaris man page long time ago and doesn't
>>>>>> respect
>>>>>> hotspot convention of using underscore to indicate class global
>>>>>> variables.
>>>>>>
>>>>>> Could you change it to something like:
>>>>>> Dl_serinfo info_sz, *info;
>>>>> Yes, I'll fix that.
>>>>>
>>>>> Best regards,
>>>>> Goetz.
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> if (dlinfo(RTLD_SELF, RTLD_DI_SERINFOSIZE, (void *)&info_sz) == -1) {
>>>> ...
>>>>
>>>> info = (Dl_serinfo*)NEW_C_HEAP_ARRAY(char, info_sz.dls_size,
>>>> mtInternal);
>>>> ...
>>>>
>>>> os_solaris.cpp: 829
>>>> The same comment about on-stack allocation as for os_bsd.cpp
>>>>
>>>> -Dmitry
>>>>
>>>> On 2014-03-25 01:50, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>> please review and test this change. I please need a sponsor.
>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.00/
>>>>>
>>>>> This change addresses the implementation of
>>>>> init_system_properties_values
>>>>> on aix, linux, solaris and bsd.
>>>>> In init_system_properties_values a macro was defined mapping malloc to
>>>>> NEW_C_HEAP_ARRAY. NEW_C_HEAP_ARRAY checks for successful allocation
>>>>> or exits the VM. The code of this method handles the allocated
>>>>> pointers as
>>>>> allocated with real malloc, i.e., there were checks for NULL and
>>>>> calls to free().
>>>>>
>>>>> This change replaces the macro malloc with NEW_C_HEAP_ARRAY and
>>>>> removes
>>>>> the unnecessary checks making the code clearer. Also, it uses a
>>>>> local array where
>>>>> possible.
>>>>>
>>>>> The allocated memory is passed to calls that end up at
>>>>> SystemProperty::set_value().
>>>>> set_value copies the strings passed. Thus the memory allocated in
>>>>> init_system_properties_values must be freed after these calls.
>>>>> Most of these
>>>>> frees were missing.
>>>>>
>>>>> This change adds the missing frees.
>>>>>
>>>>> Testing this change I ran into a warning in ppc code which I fixed,
>>>>> too.
>>>>> I did some local test. I'll get broader tests by Wednesday.
>>>>>
>>>>> Best regards,
>>>>> Goetz.
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the hotspot-dev
mailing list