RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()

[Lindenmaier, Goetz]

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 source code.