RFR: 8074895: os::getenv is inadequate

[Jeremy Manson]

On Wed, Mar 11, 2015 at 7:39 PM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Jeremy,
>
>
> On 11/03/2015 7:08 AM, Jeremy Manson wrote:
>
>> Hi, David,
>>
>> I'd like you to do a code review, please.  Original discussion:
>>
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/
>> 2015-March/016851.html
>>
>> Per this discussion, I'm just getting rid of the os::getenv interface,
>> and using straight getenv() everywhere.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8074895
>>
>> Webrev:
>> http://cr.openjdk.java.net/~jmanson/8074895/webrev.00/
>>
>
> In arguments.cpp the headless property setting could have been left using
> a local buffer rather than introducing NEW_C_HEAP_ARRAY. The value should
> only be true or false so there's no size issue regarding the local buffer.
>

Will do, but I won't bother with the rev until you tell me what to do with:


> 3566   if ((buffer = os::strdup(buffer)) == NULL) {
> 3567     return JNI_OK;
> 3568   }
>
> So this silently hides failures?
>

Yes.  Is there a best practice in Hotspot when you fail to allocate
memory?  I haven't noticed it, but I'm happy to, for example, die horribly.


> The only minor concern I have is whether os::print_environment_variables
> is now susceptible to any issues caused by arbitrarily long environment
> variables? But I suspect there is already a maximum permitted length for
> environment variables.
>

Which issues did you have in mind?  I guess if you were printing to a
string, the string would have to use extra memory.  But the getenv() was
being called anyway, so that's not much of an issue.


> Also you could now do the same for os::unsetenv :)
>

Sadly, that really *does* seem undefined on Windows, AFAICT.

Jeremy

>
> Thanks,
> David
>
>  Jeremy
>>
>