Re: WSAGetLastError usage in jdk/src/java.base/windows/native/libnet/SocketInputStream.c
On Tue, Aug 28, 2018 at 5:56 PM, Baesken, Matthias
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{
> ..
> 396ReplyBuffer = (VOID *)malloc(ReplySize);
> 397if (ReplyBuffer == NULL) {
> 398IcmpCloseHandle(hIcmpFile);
> 399NET_ThrowNew(env, WSAGetLastError(), "Unable to allocate memory");
> 400return 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 WSAx 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{
> .
> 326ReplyBuffer = (VOID *)malloc(ReplySize);
> 327if (ReplyBuffer == NULL) {
> 328IcmpCloseHandle(hIcmpFile);
> 329NET_ThrowNew(env, WSAGetLastError(), "Unable to allocate memory");
> 330return 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
>> Sent: Dienstag, 28. August 2018 14:23
>> To: Baesken, Matthias
>> Cc: net-dev
>> 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
>> 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
>> >
>> >
>> >
>> >
>> >
>> > 120nread = recv(fd, bufP, len, 0);
>> >
>> > 121if (nread > 0) {
>> >
>> > 122(*env)->SetByteArrayRegion(env, data, off, nread, (jbyte
>> > *)bufP);
>> >
>> > 123} else {
>> >
>> > 124if (nread < 0) {
>> >
>> > 125// Check if the socket has been closed since we last
>> > checked.
>> >
>> > 126// This could be a reason for recv failing.
>> >
>> > 127if ((*env)->GetIntField(env, fdObj, IO_fd_fdID) == -1) {
>> >
>> > 128JNU_ThrowByName(env, "java/net/SocketException", "Socket
>> > closed");
>> >
>> > 129} else {
>> >
>> > 130switch (WSAGetLastError()) {
>> >
>> >
>> >
>> > to :
>> >
>> >
>> >
>> > 120nread = recv(fd, bufP, len, 0);
>> >
>> > 121if (nread > 0) {
>> >
>> > 122(*env)->SetByteArrayRegion(env, data, off, nread, (jbyte
>> > *)bufP);
>> >
>> > 123} else {
>> >
>> > 124if (nread < 0) {
>> >
>> > 125int err = WSAGetLastError();
>> >
>> > ...
>> >
>> >switch (err) {
>> >
>> >
>> >
>> >
>> >
>> > Thanks and best regards, Matthias
RE: WSAGetLastError usage in jdk/src/java.base/windows/native/libnet/SocketInputStream.c
> 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{
..
396ReplyBuffer = (VOID *)malloc(ReplySize);
397if (ReplyBuffer == NULL) {
398IcmpCloseHandle(hIcmpFile);
399NET_ThrowNew(env, WSAGetLastError(), "Unable to allocate memory");
400return JNI_FALSE;
401}
jdk/src/java.base/windows/native/libnet/Inet4AddressImpl.c
306static jboolean
307ping4(JNIEnv *env, HANDLE hIcmpFile, SOCKETADDRESS *sa,
308 SOCKETADDRESS *netif, jint timeout)
309{
.
326ReplyBuffer = (VOID *)malloc(ReplySize);
327if (ReplyBuffer == NULL) {
328IcmpCloseHandle(hIcmpFile);
329NET_ThrowNew(env, WSAGetLastError(), "Unable to allocate memory");
330return JNI_FALSE;
331}
Best regards, Matthias
> -Original Message-
> From: Thomas Stüfe
> Sent: Dienstag, 28. August 2018 14:23
> To: Baesken, Matthias
> Cc: net-dev
> 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
> 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
> >
> >
> >
> >
> >
> > 120nread = recv(fd, bufP, len, 0);
> >
> > 121if (nread > 0) {
> >
> > 122(*env)->SetByteArrayRegion(env, data, off, nread, (jbyte *)bufP);
> >
> > 123} else {
> >
> > 124if (nread < 0) {
> >
> > 125// Check if the socket has been closed since we last checked.
> >
> > 126// This could be a reason for recv failing.
> >
> > 127if ((*env)->GetIntField(env, fdObj, IO_fd_fdID) == -1) {
> >
> > 128JNU_ThrowByName(env, "java/net/SocketException", "Socket
> > closed");
> >
> > 129} else {
> >
> > 130switch (WSAGetLastError()) {
> >
> >
> >
> > to :
> >
> >
> >
> > 120nread = recv(fd, bufP, len, 0);
> >
> > 121if (nread > 0) {
> >
> > 122(*env)->SetByteArrayRegion(env, data, off, nread, (jbyte *)bufP);
> >
> > 123} else {
> >
> > 124if (nread < 0) {
> >
> > 125int err = WSAGetLastError();
> >
> > ...
> >
> >switch (err) {
> >
> >
> >
> >
> >
> > Thanks and best regards, Matthias
Re: [XS] RFR: 8209994: windows: Java_java_net_NetworkInterface_getAll misses releasing interface-list
+3 Thanks, Brian On Aug 28, 2018, at 12:19 AM, Baesken, Matthias wrote: > looking at the coding, your sceanario ***should*** not happen ; however > to be on the safe side it is for sure better to do the initialization > you propose. > > Looking a bit more at the coding, there is a > Java_java_net_NetworkInterface_getAll_XP that has similar issues (missing > free_netif calls in case of "early" returns ). > I adjusted this as well in the second webrev : > > http://cr.openjdk.java.net/~mbaesken/webrevs/8209994.1/
RE: [XS] RFR: 8209994: windows: Java_java_net_NetworkInterface_getAll misses releasing interface-list
Thanks Chris ! > -Original Message- > From: Chris Hegarty > Sent: Dienstag, 28. August 2018 16:30 > To: Baesken, Matthias > Cc: Volker Simonis ; net-dev d...@openjdk.java.net>; Brian Burkhalter > Subject: Re: [XS] RFR: 8209994: windows: > Java_java_net_NetworkInterface_getAll misses releasing interface-list > > > > On 28 Aug 2018, at 15:25, Baesken, Matthias > wrote: > > > > Thanks Volki ! > > Can I have a second review please ? > > Reviewed. > > -Chris. > > > Best regards, Matthias > > > >> -Original Message- > >> From: Volker Simonis > >> Sent: Dienstag, 28. August 2018 14:07 > >> To: Baesken, Matthias > >> Cc: net-dev ; Chris Hegarty > >> ; Brian Burkhalter > >> > >> Subject: Re: [XS] RFR: 8209994: windows: > >> Java_java_net_NetworkInterface_getAll misses releasing interface-list > >> > >> Thanks for updating the change. > >> > >> Looks good now! > >> > >> Regards, > >> Volker > >> On Tue, Aug 28, 2018 at 9:19 AM Baesken, Matthias > >> wrote: > >>> > > the change looks good but I think you should also initialize 'ifList' > in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its > value is undefined and if 'enumInterfaces()' returns with an error > without assigning 'ifList' you may end up calling 'free_netif()' with > an undefined, non-NULL value. > > >>> > >>> > >>> Hi Volki, > >>> looking at the coding, your sceanario ***should*** not happen ; > >> however to be on the safe side it is for sure better to do the > >> initialization > >>> you propose. > >>> > >>> Looking a bit more at the coding, there is a > >> Java_java_net_NetworkInterface_getAll_XP that has similar issues > (missing > >> free_netif calls in case of "early" returns ). > >>> I adjusted this as well in the second webrev : > >>> > >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8209994.1/ > >>> > >>> > >>> Best regards , Matthias > >>> > >>> > -Original Message- > From: Volker Simonis > Sent: Montag, 27. August 2018 17:38 > To: Baesken, Matthias > Cc: net-dev > Subject: Re: [XS] RFR: 8209994: windows: > Java_java_net_NetworkInterface_getAll misses releasing interface-list > > Hi Matthias, > > the change looks good but I think you should also initialize 'ifList' > in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its > value is undefined and if 'enumInterfaces()' returns with an error > without assigning 'ifList' you may end up calling 'free_netif()' with > an undefined, non-NULL value. > > Best regards, > Volker > > On Mon, Aug 27, 2018 at 5:13 PM Baesken, Matthias > wrote: > > > > Hello, please review this small fix ; > > > > > > > > When returning from Java_java_net_NetworkInterface_getAll > >> (windows > version), we have to free resources to avoid leaks. > > > > In some special cases this is not done . > > > > > > > > > > > > Bug : > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8209994 > > > > > > > > change : > > > > > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8209994/ > > > > > > > > > > > > > > > > Thanks, Matthias
Re: [XS] RFR: 8209994: windows: Java_java_net_NetworkInterface_getAll misses releasing interface-list
> On 28 Aug 2018, at 15:25, Baesken, Matthias wrote: > > Thanks Volki ! > Can I have a second review please ? Reviewed. -Chris. > Best regards, Matthias > >> -Original Message- >> From: Volker Simonis >> Sent: Dienstag, 28. August 2018 14:07 >> To: Baesken, Matthias >> Cc: net-dev ; Chris Hegarty >> ; Brian Burkhalter >> >> Subject: Re: [XS] RFR: 8209994: windows: >> Java_java_net_NetworkInterface_getAll misses releasing interface-list >> >> Thanks for updating the change. >> >> Looks good now! >> >> Regards, >> Volker >> On Tue, Aug 28, 2018 at 9:19 AM Baesken, Matthias >> wrote: >>> the change looks good but I think you should also initialize 'ifList' in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its value is undefined and if 'enumInterfaces()' returns with an error without assigning 'ifList' you may end up calling 'free_netif()' with an undefined, non-NULL value. >>> >>> >>> Hi Volki, >>> looking at the coding, your sceanario ***should*** not happen ; >> however to be on the safe side it is for sure better to do the initialization >>> you propose. >>> >>> Looking a bit more at the coding, there is a >> Java_java_net_NetworkInterface_getAll_XP that has similar issues (missing >> free_netif calls in case of "early" returns ). >>> I adjusted this as well in the second webrev : >>> >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8209994.1/ >>> >>> >>> Best regards , Matthias >>> >>> -Original Message- From: Volker Simonis Sent: Montag, 27. August 2018 17:38 To: Baesken, Matthias Cc: net-dev Subject: Re: [XS] RFR: 8209994: windows: Java_java_net_NetworkInterface_getAll misses releasing interface-list Hi Matthias, the change looks good but I think you should also initialize 'ifList' in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its value is undefined and if 'enumInterfaces()' returns with an error without assigning 'ifList' you may end up calling 'free_netif()' with an undefined, non-NULL value. Best regards, Volker On Mon, Aug 27, 2018 at 5:13 PM Baesken, Matthias wrote: > > Hello, please review this small fix ; > > > > When returning from Java_java_net_NetworkInterface_getAll >> (windows version), we have to free resources to avoid leaks. > > In some special cases this is not done . > > > > > > Bug : > > > > https://bugs.openjdk.java.net/browse/JDK-8209994 > > > > change : > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8209994/ > > > > > > > > Thanks, Matthias
RE: [XS] RFR: 8209994: windows: Java_java_net_NetworkInterface_getAll misses releasing interface-list
Thanks Volki ! Can I have a second review please ? Best regards, Matthias > -Original Message- > From: Volker Simonis > Sent: Dienstag, 28. August 2018 14:07 > To: Baesken, Matthias > Cc: net-dev ; Chris Hegarty > ; Brian Burkhalter > > Subject: Re: [XS] RFR: 8209994: windows: > Java_java_net_NetworkInterface_getAll misses releasing interface-list > > Thanks for updating the change. > > Looks good now! > > Regards, > Volker > On Tue, Aug 28, 2018 at 9:19 AM Baesken, Matthias > wrote: > > > > > > > > the change looks good but I think you should also initialize 'ifList' > > > in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its > > > value is undefined and if 'enumInterfaces()' returns with an error > > > without assigning 'ifList' you may end up calling 'free_netif()' with > > > an undefined, non-NULL value. > > > > > > > > > Hi Volki, > > looking at the coding, your sceanario ***should*** not happen ; > however to be on the safe side it is for sure better to do the initialization > > you propose. > > > > Looking a bit more at the coding, there is a > Java_java_net_NetworkInterface_getAll_XP that has similar issues (missing > free_netif calls in case of "early" returns ). > > I adjusted this as well in the second webrev : > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8209994.1/ > > > > > > Best regards , Matthias > > > > > > > -Original Message- > > > From: Volker Simonis > > > Sent: Montag, 27. August 2018 17:38 > > > To: Baesken, Matthias > > > Cc: net-dev > > > Subject: Re: [XS] RFR: 8209994: windows: > > > Java_java_net_NetworkInterface_getAll misses releasing interface-list > > > > > > Hi Matthias, > > > > > > the change looks good but I think you should also initialize 'ifList' > > > in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its > > > value is undefined and if 'enumInterfaces()' returns with an error > > > without assigning 'ifList' you may end up calling 'free_netif()' with > > > an undefined, non-NULL value. > > > > > > Best regards, > > > Volker > > > > > > On Mon, Aug 27, 2018 at 5:13 PM Baesken, Matthias > > > wrote: > > > > > > > > Hello, please review this small fix ; > > > > > > > > > > > > > > > > When returning from Java_java_net_NetworkInterface_getAll > (windows > > > version), we have to free resources to avoid leaks. > > > > > > > > In some special cases this is not done . > > > > > > > > > > > > > > > > > > > > > > > > Bug : > > > > > > > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8209994 > > > > > > > > > > > > > > > > change : > > > > > > > > > > > > > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8209994/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, Matthias
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
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
>
>
>
>
>
> 120nread = recv(fd, bufP, len, 0);
>
> 121if (nread > 0) {
>
> 122(*env)->SetByteArrayRegion(env, data, off, nread, (jbyte *)bufP);
>
> 123} else {
>
> 124if (nread < 0) {
>
> 125// Check if the socket has been closed since we last checked.
>
> 126// This could be a reason for recv failing.
>
> 127if ((*env)->GetIntField(env, fdObj, IO_fd_fdID) == -1) {
>
> 128JNU_ThrowByName(env, "java/net/SocketException", "Socket
> closed");
>
> 129} else {
>
> 130switch (WSAGetLastError()) {
>
>
>
> to :
>
>
>
> 120nread = recv(fd, bufP, len, 0);
>
> 121if (nread > 0) {
>
> 122(*env)->SetByteArrayRegion(env, data, off, nread, (jbyte *)bufP);
>
> 123} else {
>
> 124if (nread < 0) {
>
> 125int err = WSAGetLastError();
>
> ...
>
>switch (err) {
>
>
>
>
>
> Thanks and best regards, Matthias
WSAGetLastError usage in jdk/src/java.base/windows/native/libnet/SocketInputStream.c
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
120nread = recv(fd, bufP, len, 0);
121if (nread > 0) {
122(*env)->SetByteArrayRegion(env, data, off, nread, (jbyte *)bufP);
123} else {
124if (nread < 0) {
125// Check if the socket has been closed since we last checked.
126// This could be a reason for recv failing.
127if ((*env)->GetIntField(env, fdObj, IO_fd_fdID) == -1) {
128JNU_ThrowByName(env, "java/net/SocketException", "Socket
closed");
129} else {
130switch (WSAGetLastError()) {
to :
120nread = recv(fd, bufP, len, 0);
121if (nread > 0) {
122(*env)->SetByteArrayRegion(env, data, off, nread, (jbyte *)bufP);
123} else {
124if (nread < 0) {
125int err = WSAGetLastError();
...
switch (err) {
Thanks and best regards, Matthias
Re: [XS] RFR: 8209994: windows: Java_java_net_NetworkInterface_getAll misses releasing interface-list
Thanks for updating the change. Looks good now! Regards, Volker On Tue, Aug 28, 2018 at 9:19 AM Baesken, Matthias wrote: > > > > > the change looks good but I think you should also initialize 'ifList' > > in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its > > value is undefined and if 'enumInterfaces()' returns with an error > > without assigning 'ifList' you may end up calling 'free_netif()' with > > an undefined, non-NULL value. > > > > > Hi Volki, > looking at the coding, your sceanario ***should*** not happen ; however > to be on the safe side it is for sure better to do the initialization > you propose. > > Looking a bit more at the coding, there is a > Java_java_net_NetworkInterface_getAll_XP that has similar issues (missing > free_netif calls in case of "early" returns ). > I adjusted this as well in the second webrev : > > http://cr.openjdk.java.net/~mbaesken/webrevs/8209994.1/ > > > Best regards , Matthias > > > > -Original Message- > > From: Volker Simonis > > Sent: Montag, 27. August 2018 17:38 > > To: Baesken, Matthias > > Cc: net-dev > > Subject: Re: [XS] RFR: 8209994: windows: > > Java_java_net_NetworkInterface_getAll misses releasing interface-list > > > > Hi Matthias, > > > > the change looks good but I think you should also initialize 'ifList' > > in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its > > value is undefined and if 'enumInterfaces()' returns with an error > > without assigning 'ifList' you may end up calling 'free_netif()' with > > an undefined, non-NULL value. > > > > Best regards, > > Volker > > > > On Mon, Aug 27, 2018 at 5:13 PM Baesken, Matthias > > wrote: > > > > > > Hello, please review this small fix ; > > > > > > > > > > > > When returning from Java_java_net_NetworkInterface_getAll (windows > > version), we have to free resources to avoid leaks. > > > > > > In some special cases this is not done . > > > > > > > > > > > > > > > > > > Bug : > > > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8209994 > > > > > > > > > > > > change : > > > > > > > > > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8209994/ > > > > > > > > > > > > > > > > > > > > > > > > Thanks, Matthias
RE: [XS] RFR: 8209994: windows: Java_java_net_NetworkInterface_getAll misses releasing interface-list
> > the change looks good but I think you should also initialize 'ifList' > in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its > value is undefined and if 'enumInterfaces()' returns with an error > without assigning 'ifList' you may end up calling 'free_netif()' with > an undefined, non-NULL value. > Hi Volki, looking at the coding, your sceanario ***should*** not happen ; however to be on the safe side it is for sure better to do the initialization you propose. Looking a bit more at the coding, there is a Java_java_net_NetworkInterface_getAll_XP that has similar issues (missing free_netif calls in case of "early" returns ). I adjusted this as well in the second webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8209994.1/ Best regards , Matthias > -Original Message- > From: Volker Simonis > Sent: Montag, 27. August 2018 17:38 > To: Baesken, Matthias > Cc: net-dev > Subject: Re: [XS] RFR: 8209994: windows: > Java_java_net_NetworkInterface_getAll misses releasing interface-list > > Hi Matthias, > > the change looks good but I think you should also initialize 'ifList' > in 'Java_java_net_NetworkInterface_getAll()' with NULL otherwise its > value is undefined and if 'enumInterfaces()' returns with an error > without assigning 'ifList' you may end up calling 'free_netif()' with > an undefined, non-NULL value. > > Best regards, > Volker > > On Mon, Aug 27, 2018 at 5:13 PM Baesken, Matthias > wrote: > > > > Hello, please review this small fix ; > > > > > > > > When returning from Java_java_net_NetworkInterface_getAll (windows > version), we have to free resources to avoid leaks. > > > > In some special cases this is not done . > > > > > > > > > > > > Bug : > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8209994 > > > > > > > > change : > > > > > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8209994/ > > > > > > > > > > > > > > > > Thanks, Matthias
