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

[Rob McKenna]

On 27/02/16 11:20, David Holmes wrote:
> On 27/02/2016 5:00 AM, Volker Simonis wrote:
> >Hi Thomas,
> >
> >it's good that somebody finally addresses this long standing issue :)
> >
> >However I wonder if it would be possible to align this effort with the
> >core libraries group (CC'ed). They already fix this issue with:
> >
> >8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString
> >https://bugs.openjdk.java.net/browse/JDK-8133249
> >
> >I would be nice if we could use the same version in hotspot and the
> >core libraries.
> 
> I don't find this:
> 
> +#if defined(LINUX) && (defined(_GNU_SOURCE) || \
> +         (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE < 200112L \
> +             && defined(_XOPEN_SOURCE) && _XOPEN_SOURCE < 600))
> +extern int __xpg_strerror_r(int, char *, size_t);
> +#define strerror_r(a, b, c) __xpg_strerror_r((a), (b), (c))
> +#endif
> 
> particularly appealing at all - you can't even decode it without having a
> POSIX and XOpen version history in front of you :(
> 
> And a version that requires a buffer to be passed in is problematic in a
> number of usage contexts in hotspot - see the comments in the bug report. A
> simplified version that converts symbolic error values to their string
> equivalent is much more appealing - albeit fixing an issue that "should" be
> handled at the library level.

Agreed, its horrific. Unfortunately any fix for strerror thread safety is effectively a kludge on top of an ugly library situation. This fix seems to be going down the route of reimplementing sys_errlist (which iirc Solaris has removed and Linux has deprecated) I didn't feel confident taking that path with my fix but this may be a case of what works for hotspot may be different to what works for corelibs.

    -Rob

> 
> David
> -----
> 
> >Regards,
> >Volker
> >
> >
> >On Fri, Feb 26, 2016 at 5:05 PM, Thomas Stüfe <thomas.stuefe at gmail.com> 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.
> >>
> >>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