8210147: adjust some WSAGetLastError usages in windows network coding - was : RE: WSAGetLastError usage in jdk/src/java.base/windows/native/libnet/SocketInputStream.c

Wed Aug 29 09:22:19 UTC 2018

Hi Matthias,

On Wed, Aug 29, 2018 at 9:46 AM, Baesken, Matthias
<matthias.baesken at sap.com> wrote:
>
>> 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
>
> Hi Thomas,  according to
>
> https://docs.microsoft.com/en-us/windows/desktop/winsock/windows-sockets-error-codes-2
>
> both 0 and -1  are fine .
>
> Instead of  using
>
>    NET_ThrowNew(env,  -1, "Unable to allocate memory");
>
> We could also  call :
>
>    JNU_ThrowOutOfMemoryError(env, "Native heap allocation failed");
>
> (or would this be problematic because of compatibility?).
>
>

That should be fine too.

Best Regards, Thomas

>
> Btw  I opened a bug  :
>
> https://bugs.openjdk.java.net/browse/JDK-8210147
>
> for the issues .
>
> Best regards, Matthias
>
>
>
>> -----Original Message-----
>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>> Sent: Dienstag, 28. August 2018 18:33
>> 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
>>
>> 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