RFR(s): 8148425: strerror() function is not thread-safe

[David Holmes]

Hi Thomas,

On 1/03/2016 3:23 AM, Thomas Stüfe wrote:
> Hi David,
>
> On Mon, Feb 29, 2016 at 4:44 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi Thomas,
>
>     On 27/02/2016 2:05 AM, Thomas Stüfe wrote:
>
>         Hi,
>
>         please take a look at this proposed fix:
>
>         Bug: https://bugs.openjdk.java.net/browse/JDK-8148425
>         Webrev:
>         http://cr.openjdk.java.net/~stuefe/webrevs/8141425-strerror-replacement/webrev.00/webrev/
>
>         This adds a replacement function os::strerror() as a drop-in for
>         strerror(), which has a number of issues.
>
>         strerror() is unsafe to use and may cause (and has caused)
>         crashes in
>         multithreaded scenarios. It is also not ideal for log files
>         because of the
>         implicit localization of the error messages.
>
>         For details please see the discussion under the bug report.
>
>
>     I just came across strerror_l, which is required to be thread-safe.
>     Is that a possible alternative? (I'm not sure how locale's are
>     obtained).
>
>
> Sorry, I think this API is glibc only. At least I cannot find this in
> our AIX headers, nor on Solaris.

It is a POSIX API:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/strerror_l.html

added in 2006 but part of POSIX.1-2008. But as per Dalibor's link not 
necessarily available:

"This function is missing on some platforms: glibc 2.3.6, Mac OS X 10.5, 
FreeBSD 6.0, NetBSD 5.0, OpenBSD 3.8, Minix 3.1.8, AIX 5.1, HP-UX 11, 
IRIX 6.5, OSF/1 5.1, Solaris 11 2011-11, Cygwin, mingw, MSVC 9, Interix 
3.5, BeOS. "

Pity.


>     Otherwise what you have seems okay - though I do dislike having to
>     duplicate all that details already buried in the system
>     headers/library. Not sure we need to the long text at the VM level -
>     which would simplify things a bit.
>
>
> I agree, I dislike this too. Like everyone else in this thread. But I
> think this is a pragmatic solution.
>
> I am a bit stuck here - should we really get rid of the long text
> feature? There are some callsites of strerror() in the hotspot where
> arguably the long text is better suited:
> - in assert_status() (see debug.hpp) - this ends up in the header of
> error files, if this suddenly changes to a literalized errno, people may
> be upset

I added assert_status purely to expand on what the failing status was :) 
I'm happy to see EINVAL in that context rather than "invalid argument" 
(or whatever) :)

> - when failing to write a heap dump file - see services/heapDumper.cpp.
> Which ends as printout on the command line, I think.

Maybe ... if it is an error the user can fix.

> The safe option would be to provide both variants (short and long text).
> Or, provide the safe, short variant for all logging calls - when
> "EINVAL" is enough, and let users continue to use strerror() for those
> few cases where the long text is needed.
>
> What do you think?

Trying to decide what makes me least unhappy. :) Given you've already 
done the work I guess having long and short forms is okay.

Thanks,
David

> Thanks, Thomas
>
>     Thanks,
>     David
>
>
>         Please note that I did not yet change any call sites, although
>         all call
>         sites in the os namespace should already use the new function. I
>         wanted to
>         see whether there would be any general objections.
>
>         Kind Regards, Thomas
>
>