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

[Rob McKenna]

Thanks Ivan, I'll push with these changes.

	-Rob

On 24/09/15 16:31, Ivan Gerasimov wrote:
> 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
>>>
>>
>