RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString
Ivan Gerasimov
ivan.gerasimov at oracle.com
Thu Sep 24 15:31:56 UTC 2015
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
>>
>
More information about the core-libs-dev
mailing list