RFR: 8074895: os::getenv is inadequate

[Jeremy Manson]

Thanks, David!  New rev:

http://cr.openjdk.java.net/~jmanson/8074895/webrev.01/

On Thu, Mar 12, 2015 at 5:12 PM, David Holmes <david.holmes at oracle.com>
wrote:

> On 13/03/2015 6:31 AM, Jeremy Manson wrote:
>
>> On Wed, Mar 11, 2015 at 7:39 PM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>     On 11/03/2015 7:08 AM, Jeremy Manson wrote:
>>         I'd like you to do a code review, please.  Original discussion:
>>
>>         http://mail.openjdk.java.net/__pipermail/serviceability-dev/
>> __2015-March/016851.html
>>         <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
>>         <https://bugs.openjdk.java.net/browse/JDK-8074895>
>>
>>         Webrev:
>>         http://cr.openjdk.java.net/~__jmanson/8074895/webrev.00/
>>         <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.
>>
>
> At this stage of the VM initialization we should be trying to be a good
> citizen and simply return JNI_ENOMEM, which will eventually make its way
> back to the caller of CreateJavaVM.
>
> Which also means your use of NEW_C_HEAP_ARRAY also needs changing as it
> will abort the VM. You want to use NEW_C_HEAP_ARRAY_RETURN_NULL and if NULL
> then return JNI_ENOMEM.
>

Done.


>
>      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.
>>
>
> I was wondering if an absurdly long environment variable value could
> overflow some other internal buffer somewhere.


Not sure why that would be different from the old approach, which also
called getenv(), just underneath a layer of abstraction.  I'm *trying* to
do the right checking on everything above that, but I haven't run anything
like valgrind to make sure.


>      Also you could now do the same for os::unsetenv :)
>>
>> Sadly, that really *does* seem undefined on Windows, AFAICT.
>>
>
> Yeah they use _putenv with an empty value to remove the variable
>

Ah, well.

Jeremy