RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString

[Ivan Gerasimov]

Thanks Rob!

The webrev looks cleaner.

jni_util.h:
Wouldn't it be better to move this block directly to the jni_util_md.c file?
  390 #if defined(LINUX) && (defined(_GNU_SOURCE) || \
  391          (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE < 200112L \
  392              && defined(_XOPEN_SOURCE) && _XOPEN_SOURCE < 600))
  393 extern int __xpg_strerror_r(int, char *, size_t);
  394 #endif
I don't see the __xpg_strerror_r() function used anywhere outside that 
.c file anyway.


TwoStacksPlainDatagramSocketImpl.c:
It wasn't introduced by your fix, but it seems that a new-line isn't 
necessary here:
2218         sprintf(errmsg, "error getting socket option: %s\n", tmpbuf);

In all other places JNU_ThrowByName() uses non-new-line-terminated messages.

Sincerely yours,
Ivan

On 24.09.2015 17:10, Rob McKenna wrote:
> Hi folks,
>
> Uploaded a newer version:
>
> http://cr.openjdk.java.net/~robm/8133249/webrev.02/
>
> To address Rogers question, this needed to be separate from 
> getLastErrorString because of the return type and the err arg, but I 
> like the idea of avoiding the creation of a new file. Also, all of the 
> files that require the new function (now called getErrorString) 
> already include jni_util.c so it makes sense to put this in 
> jni_util_md.c with getLastErrorString.
>
> As per Christos' suggestion I've mapped strerr_r to __xpg_strerror_r 
> instead of using the gnu strerror_r. I'd be interested to hear 
> feedback on this change.
>
> I've corrected the oversights around the jli sources and 
> PlainDatagramSocket. The jli stuff shouldn't have been there in the 
> first place.
>
> Ivan, I've kept the return type. As you note, I should have been using 
> it in ProcessImpl_md.c and the extra information from the return is 
> potentially useful.
>
>     -Rob
>
> On 23/09/15 14:14, Ivan Gerasimov wrote:
>> Hi Rob!
>>
>> On 21.09.2015 18:53, Rob McKenna wrote:
>>> Hi folks,
>>>
>>> Requesting a review of this change which switches corelibs usages of
>>> the thread-unsafe strerror over to strerror_r/strerror_s:
>>>
>>> http://cr.openjdk.java.net/~robm/8133249/webrev.01/
>>>
>>
>> I think it would be better to have  jdk_strerror(errno, tmpbuf,
>> sizeof(tmpbuf))  instead of  jdk_strerror(errno, tmpbuf, (size_t) 1024),
>> just for the sake of avoiding constant duplication.
>> I don't see the return code of jdk_strerror() is ever used. I suggest
>> using it in ProcessImpl_md.c (see below), or just making this function
>> return void.
>>
>> jni_util_md.c:
>> - why can't we just call  jdk_strerror(errno, buf, len)  and get rid of
>> the temp buffer?
>>
>> java_md_common.c:
>> - commented out #include @ line 26
>> - the change in JLI_ReportErrorMessageSys() looks incomplete
>>
>> ProcessImpl_md.c:
>> - would it make sense to check if jdk_strerror() == EINVAL instead of
>> comparing the strings?
>>
>> PlainDatagramSocketImpl.c:
>> - #include added, but no other changes were made.
>>
>> TwoStacksPlainDatagramSocketImpl.c:
>> - buf is declared to be char[1024], but only up to 255 chars are used.
>> - would be better to move the buf declaration closer to its use, maybe
>> next to char errmsg[300] line?
>>
>> jdk_strerror.c:
>> - minor nit: extra space after strerror_r at lines 46 and 63.
>>
>> Sincerely yours,
>> Ivan
>>
>