RFR: 8074895: os::getenv is inadequate

[David Holmes]

Hi Jeremy,

On 14/03/2015 3:00 AM, Jeremy Manson wrote:
> Thanks, David!  New rev:
>
> http://cr.openjdk.java.net/~jmanson/8074895/webrev.01/

Looks good. Please update copyright dates in memTracker.cpp and vmError.cpp.

Further response below ...

> On Thu, Mar 12, 2015 at 5:12 PM, David Holmes <david.holmes at oracle.com
> <mailto: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>
>         <mailto: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>
>
>         <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>
>                  <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/>
>
>         <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.

The old approach limited the size of anything that getenv returned 
before passing it further into the VM. The new approach doesn't. But 
following through the code to see how the variables get used I can't see 
anything that makes any assumptions about the potential length of the 
variable's value. So this looks okay.

Thanks,
David

>
>              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