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 07:46:23 UTC 2018

> 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?).

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