RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Dmitry Samersoff
dmitry.samersoff at oracle.com
Tue Apr 8 22:08:18 UTC 2014
Vladimir,
OK. Thank you for clarification.
-Dmitry
On 2014-04-09 01:43, Vladimir Kozlov wrote:
> On 4/8/14 2:19 PM, Dmitry Samersoff wrote:
>> Vladimir,
>>
>> On 2014-04-09 00:32, Vladimir Kozlov wrote:
>>
>>> Could you tell when you are planning to backport these changes into
>>> 8u20?
>>
>> This week. Do I need any additional approvals?
>
> No need for approval but, please, send new [8u20] RFR to hotspot-dev
> with links to jbs and jdk9 changeset and say that it applies cleanly (if
> it is). I will review it and you can push.
> It is to let people know that you backport it.
>
> Thanks,
> Vladimir
>
>>
>> -Dmitry
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 4/1/14 5:50 AM, Dmitry Samersoff wrote:
>>>> 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 hotspot-dev
mailing list