[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(s): 8148425: strerror() function is not thread-safe
From:       Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date:       2016-02-29 17:23:56
Message-ID: CAA-vtUxU7dbfkGWxbFEsCmAC-=5qNpi2tsOsOKMuNwi2k-S3jw () mail ! gmail ! com
[Download RAW message or body]

Hi David,

On Mon, Feb 29, 2016 at 4:44 AM, David Holmes <david.holmes@oracle.com>
wrote:

> Hi Thomas,
>
> On 27/02/2016 2:05 AM, Thomas St=C3=BCfe 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 t=
he
>> 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.


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

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?

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
>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]