Re: RFR(s): 8148425: strerror() function is not thread-safe
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. Regards, Volker On Fri, Feb 26, 2016 at 5:05 PM, Thomas Stüfe <thomas.stuefe@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/webr...
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
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. David -----
Regards, Volker
On Fri, Feb 26, 2016 at 5:05 PM, Thomas Stüfe <thomas.stuefe@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/webr...
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
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@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/webr...
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
Hi Volker, On Fri, Feb 26, 2016 at 8:00 PM, Volker Simonis <volker.simonis@gmail.com> wrote:
Hi Thomas,
it's good that somebody finally addresses this long standing issue :)
Thanks :)
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.
David already gave a good answer to this. For details see the comments in the bug report. In addition, there is the localization strerror(_s) provides: it is useless and actually counterproductive in log files intended for the VM developers themselves; but if those error messages get stuffed into Java Exception texts and presented to the user, they might make more sense. Kind Regards, Thomas
Regards, Volker
On Fri, Feb 26, 2016 at 5:05 PM, Thomas Stüfe <thomas.stuefe@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/webr...
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
participants (4)
-
David Holmes
-
Rob McKenna
-
Thomas Stüfe
-
Volker Simonis