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

[Thomas Stüfe]

Hi all,

please take a look at this next version of the fix.

http://cr.openjdk.java.net/~stuefe/webrevs/8141425-strerror-replacement/webrev.01/webrev/

I am the first to admit that this is quite ugly :(, but it is a pragmatic
solution.

Some details:

- I added os::strerror() as a drop-in replacement for ::strerror() - it
returns the text as before, but does not localize and does not handle
unknown error values; so it can be implemented using a simple static string
table and is always safe to use.

- I added os::errno_name() for those cases where one would want the
literalized errno value (e.g. "EINVAL") instead of the full text.

- I fixed all callsites in the hotspot which used ::strerror to use
os::strerror, or, in some few cases (mostly my own coding for AIX),
os::errno_name().

- To avoid including os.hpp in debug.hpp, I moved the implemntation of
assert_status to debug.cpp


Kind Regards, Thomas




On Tue, Mar 8, 2016 at 11:01 PM, Coleen Phillimore <
coleen.phillimore at oracle.com> wrote:

>
> This latest version seems okay (or at least better than the other
> workarounds).
>
> thanks,
> Coleen
>
>
> On 3/1/16 12:30 AM, David Holmes wrote:
>
>> 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
>>>
>>>
>>>
>