RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Dmitry Samersoff
dmitry.samersoff at oracle.com
Tue Apr 1 12:50:53 UTC 2014
Goetz,
Push done.
Let it bakes for some times in JDK9 (at least two nightlies) than I'll
push it to 8u20
-Dmitry
On 2014-03-27 23:52, Lindenmaier, Goetz wrote:
> Thanks Dmitry and Vladimir!
>
> Best regards,
> Goetz.
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Thursday, March 27, 2014 8:18 PM
> To: Dmitry Samersoff; 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()
>
> Thank you, Dmitry
>
> We need to backport it into 8u20 too.
>
> Regards,
> Vladimir
>
> On 3/27/14 12:16 PM, Dmitry Samersoff wrote:
>> 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 ppc-aix-port-dev
mailing list