WSAGetLastError usage in jdk/src/java.base/windows/native/libnet/SocketInputStream.c

[Thomas Stüfe]

On Tue, Aug 28, 2018 at 5:56 PM, Baesken, Matthias
<matthias.baesken at sap.com> wrote:
>> not strictly necessary, since WSAGetLastError() only refers to socket calls
>
> I'll try to find out more about this.
>
>> However, I think your proposal is fine for code cleanliness sake.
>
> Yes , then I will do it for code cleanliness sake (and because MS recommends anyway to get the error immediately).
>
> When looking  at the   WSAGetLastError()   usages in the whole JDK  Windows  code I found 2 strange  usages ,
>   Both times   it looks like  WSAGetLastError()   is used to   get the error of the malloc call ,  in case this really works,
>   WSAGetLastError  might indeed alias in some way to GetLastError :
>
> jdk/src/java.base/windows/native/libnet/Inet6AddressImpl.c
>
> 384static jboolean
> 385ping6(JNIEnv *env, HANDLE hIcmpFile, SOCKETADDRESS *sa,
> 386      SOCKETADDRESS *netif, jint timeout)
> 387{
> ..............
> 396    ReplyBuffer = (VOID *)malloc(ReplySize);
> 397    if (ReplyBuffer == NULL) {
> 398        IcmpCloseHandle(hIcmpFile);
> 399        NET_ThrowNew(env, WSAGetLastError(), "Unable to allocate memory");
> 400        return JNI_FALSE;
> 401    }
>

Thats plain wrong.

One should use errno instead, however, the Windows variant of
NET_ThrowNew expects a WSA error code for the error number, it does
not handle errno.

What I would do to keep the change simple is just passing 0 or -1 as
errorNum argument to NET_ThrowNew - in that case it will say
"Unspecified socket error" in the exception. Just make sure that 0 or
-1, whatever you use, is not one of the WSAxxxxx constants, see
winsock_errors[] table in net_util_md.c.

>
> jdk/src/java.base/windows/native/libnet/Inet4AddressImpl.c
>
> 306static jboolean
> 307ping4(JNIEnv *env, HANDLE hIcmpFile, SOCKETADDRESS *sa,
> 308      SOCKETADDRESS *netif, jint timeout)
> 309{
> .........
> 326    ReplyBuffer = (VOID *)malloc(ReplySize);
> 327    if (ReplyBuffer == NULL) {
> 328        IcmpCloseHandle(hIcmpFile);
> 329        NET_ThrowNew(env, WSAGetLastError(), "Unable to allocate memory");
> 330        return JNI_FALSE;
> 331    }
>
>

Yes, this may be wrong if WSAGetLastError() uses GetLastError()
internally, so I would move the WSAGetLastError() call up.

Good catches!

..Thomas

> Best regards, Matthias
>
>
>
>
>> -----Original Message-----
>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>> Sent: Dienstag, 28. August 2018 14:23
>> To: Baesken, Matthias <matthias.baesken at sap.com>
>> Cc: net-dev <net-dev at openjdk.java.net>
>> Subject: Re: WSAGetLastError usage in
>> jdk/src/java.base/windows/native/libnet/SocketInputStream.c
>>
>> Hi Matthias,
>>
>> not strictly necessary, since WSAGetLastError() only refers to socket
>> calls and GetIntField() is unlikely to call socket functions...
>> However, I think your proposal is fine for code cleanliness sake.
>>
>> Best Regards, Thomas
>>
>> On Tue, Aug 28, 2018 at 2:13 PM, Baesken, Matthias
>> <matthias.baesken at sap.com> wrote:
>> > Hello,
>> >
>> > the MSDN docu about WSAGetLastError warns to get the error-code
>> > ***immediately*** after occurance.
>> >
>> > See :
>> >
>> >
>> >
>> > https://msdn.microsoft.com/de-
>> de/library/windows/desktop/ms741580(v=vs.85).aspx
>> >
>> >
>> >
>> > " ... If a function call's return value indicates that error or other
>> > relevant data was returned in the error code,
>> >
>> > WSAGetLastError should be called immediately ..."
>> >
>> >
>> >
>> > However in windows SocketInputStream.c , this was not done; we noticed
>> very
>> > seldom errors because of this (not reproducible however) so
>> >
>> > we had a fix for this in our code base for a long time.
>> >
>> >
>> >
>> > Should we change this as well in OpenJDK , for example from :
>> >
>> >
>> >
>> > jdk/src/java.base/windows/native/libnet/SocketInputStream.c
>> >
>> >
>> >
>> >
>> >
>> > 120    nread = recv(fd, bufP, len, 0);
>> >
>> > 121    if (nread > 0) {
>> >
>> > 122        (*env)->SetByteArrayRegion(env, data, off, nread, (jbyte *)bufP);
>> >
>> > 123    } else {
>> >
>> > 124        if (nread < 0) {
>> >
>> > 125            // Check if the socket has been closed since we last checked.
>> >
>> > 126            // This could be a reason for recv failing.
>> >
>> > 127            if ((*env)->GetIntField(env, fdObj, IO_fd_fdID) == -1) {
>> >
>> > 128                JNU_ThrowByName(env, "java/net/SocketException", "Socket
>> > closed");
>> >
>> > 129            } else {
>> >
>> > 130                switch (WSAGetLastError()) {
>> >
>> >
>> >
>> > to  :
>> >
>> >
>> >
>> > 120    nread = recv(fd, bufP, len, 0);
>> >
>> > 121    if (nread > 0) {
>> >
>> > 122        (*env)->SetByteArrayRegion(env, data, off, nread, (jbyte *)bufP);
>> >
>> > 123    } else {
>> >
>> > 124        if (nread < 0) {
>> >
>> > 125            int err = WSAGetLastError();
>> >
>> > ...
>> >
>> >                    switch (err) {
>> >
>> >
>> >
>> >
>> >
>> > Thanks and best regards, Matthias