RFR: 8074895: os::getenv is inadequate
David Holmes
david.holmes at oracle.com
Fri Mar 13 00:12:31 UTC 2015
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.
> 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.
> 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.
Thanks,
David
> Jeremy
>
>
> Thanks,
> David
>
> Jeremy
>
>
More information about the hotspot-runtime-dev
mailing list