RFR: 8170544: Fix code scan findings in libnet

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8170544: Fix code scan findings in libnet

Langer, Christoph

Hi,

 

please review the following change:

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8170544

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/

 

With this change I change some function signatures to use SOCKETADDRESS * instead of struct sockaddr * where appropriate. This fixes some code scan findings, which reported the fact that it’s dangerous to pass struct socaddr * pointers and then cast it to the wider struct sockaddr_in6. I think this makes the code cleaner and spares some casts at several places.

 

Please review and run through JPRT testing.

 

Thanks

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8170544: Fix code scan findings in libnet

Michael McMahon
I will run the change through JPRT Christoph

Thanks
Michael

On 30/11/2016, 16:32, Langer, Christoph wrote:

Hi,

 

please review the following change:

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8170544

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/

 

With this change I change some function signatures to use SOCKETADDRESS * instead of struct sockaddr * where appropriate. This fixes some code scan findings, which reported the fact that it’s dangerous to pass struct socaddr * pointers and then cast it to the wider struct sockaddr_in6. I think this makes the code cleaner and spares some casts at several places.

 

Please review and run through JPRT testing.

 

Thanks

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8170544: Fix code scan findings in libnet

Langer, Christoph

Thanks, Michael.

 

Maybe you can run all the 3 together:

 

http://cr.openjdk.java.net/~clanger/webrevs/8167457.1/

http://cr.openjdk.java.net/~clanger/webrevs/8167420.2/

http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/

 

8170544 comes atop 8167420 (though it’s just a small clash).

 

Best regards

Christoph

 

 

From: Michael McMahon [mailto:[hidden email]]
Sent: Mittwoch, 30. November 2016 17:57
To: Langer, Christoph <[hidden email]>
Cc: [hidden email]; Chris Hegarty <[hidden email]>; Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR: 8170544: Fix code scan findings in libnet

 

I will run the change through JPRT Christoph

Thanks
Michael

On 30/11/2016, 16:32, Langer, Christoph wrote:

Hi,

 

please review the following change:

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8170544

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/

 

With this change I change some function signatures to use SOCKETADDRESS * instead of struct sockaddr * where appropriate. This fixes some code scan findings, which reported the fact that it’s dangerous to pass struct socaddr * pointers and then cast it to the wider struct sockaddr_in6. I think this makes the code cleaner and spares some casts at several places.

 

Please review and run through JPRT testing.

 

Thanks

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8170544: Fix code scan findings in libnet

Langer, Christoph
In reply to this post by Michael McMahon

Hi Michael,

 

I have just updated http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/ in place. The old version caused a compile warning on Linux.

 

Best regards

Christoph

 

From: Langer, Christoph
Sent: Mittwoch, 30. November 2016 19:01
To: 'Michael McMahon' <[hidden email]>
Cc: [hidden email]; Chris Hegarty <[hidden email]>; Lindenmaier, Goetz <[hidden email]>
Subject: RE: RFR: 8170544: Fix code scan findings in libnet

 

Thanks, Michael.

 

Maybe you can run all the 3 together:

 

http://cr.openjdk.java.net/~clanger/webrevs/8167457.1/

http://cr.openjdk.java.net/~clanger/webrevs/8167420.2/

http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/

 

8170544 comes atop 8167420 (though it’s just a small clash).

 

Best regards

Christoph

 

 

From: Michael McMahon [[hidden email]]
Sent: Mittwoch, 30. November 2016 17:57
To: Langer, Christoph <[hidden email]>
Cc: [hidden email]; Chris Hegarty <[hidden email]>; Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR: 8170544: Fix code scan findings in libnet

 

I will run the change through JPRT Christoph

Thanks
Michael

On 30/11/2016, 16:32, Langer, Christoph wrote:

Hi,

 

please review the following change:

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8170544

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/

 

With this change I change some function signatures to use SOCKETADDRESS * instead of struct sockaddr * where appropriate. This fixes some code scan findings, which reported the fact that it’s dangerous to pass struct socaddr * pointers and then cast it to the wider struct sockaddr_in6. I think this makes the code cleaner and spares some casts at several places.

 

Please review and run through JPRT testing.

 

Thanks

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8170544: Fix code scan findings in libnet

Michael McMahon
Okay. I'll run the three patches through jprt again

Michael

On 01/12/2016, 11:53, Langer, Christoph wrote:

Hi Michael,

 

I have just updated http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/ in place. The old version caused a compile warning on Linux.

 

Best regards

Christoph

 

From: Langer, Christoph
Sent: Mittwoch, 30. November 2016 19:01
To: 'Michael McMahon' [hidden email]
Cc: [hidden email]; Chris Hegarty [hidden email]; Lindenmaier, Goetz [hidden email]
Subject: RE: RFR: 8170544: Fix code scan findings in libnet

 

Thanks, Michael.

 

Maybe you can run all the 3 together:

 

http://cr.openjdk.java.net/~clanger/webrevs/8167457.1/

http://cr.openjdk.java.net/~clanger/webrevs/8167420.2/

http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/

 

8170544 comes atop 8167420 (though it’s just a small clash).

 

Best regards

Christoph

 

 

From: Michael McMahon [[hidden email]]
Sent: Mittwoch, 30. November 2016 17:57
To: Langer, Christoph <[hidden email]>
Cc: [hidden email]; Chris Hegarty <[hidden email]>; Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR: 8170544: Fix code scan findings in libnet

 

I will run the change through JPRT Christoph

Thanks
Michael

On 30/11/2016, 16:32, Langer, Christoph wrote:

Hi,

 

please review the following change:

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8170544

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/

 

With this change I change some function signatures to use SOCKETADDRESS * instead of struct sockaddr * where appropriate. This fixes some code scan findings, which reported the fact that it’s dangerous to pass struct socaddr * pointers and then cast it to the wider struct sockaddr_in6. I think this makes the code cleaner and spares some casts at several places.

 

Please review and run through JPRT testing.

 

Thanks

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8170544: Fix code scan findings in libnet

Lindenmaier, Goetz
In reply to this post by Langer, Christoph
Hi Christoph,

I had a look at your change.
It's a bit confusing that you call the variable 'sa' everywhere, as
a field of it has the same name. sa.sa looks confusing.
But it's good that you name it everywhere the same, and
that you went through all this code.

Some details:

http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/share/native/libnet/net_util.c.udiff.html
+     * check now to whether we have IPv6 on this platform and if the
superfluous 'to'

http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/native/libnet/NetworkInterface.c.udiff.html
For the records: you add closing the connection in an error case.

http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/native/libnet/net_util_md.c.udiff.html
Is it safe to do memset here?  I think memset in the ipv4/v6 cases with the corresponding sizes is safer.
Len is not passed in all the times. Else you could memset with len.
(You change len to '0' in http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/native/libnio/ch/DatagramChannelImpl.c.udiff.html )
If you are sure you always pass a full SOCKETADDRESS this is fine, though.
Same holds for the windows size.
Overall, the len field is quite superfluous now, isn't it?  But this should not be changed
in this change I think.

http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/windows/native/libnet/TwoStacksPlainDatagramSocketImpl.c.udiff.html
Please add spaces around '=': fd=-1

Otherwise looks good.
I ran the change through our code scan, the issues are all gone now.

Best regards,
  Goetz.




> -----Original Message-----
> From: Langer, Christoph
> Sent: Donnerstag, 1. Dezember 2016 12:54
> To: Michael McMahon <[hidden email]>
> Cc: [hidden email]; Chris Hegarty <[hidden email]>;
> Lindenmaier, Goetz <[hidden email]>
> Subject: RE: RFR: 8170544: Fix code scan findings in libnet
>
> Hi Michael,
>
>
>
> I have just updated http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/ in
> place. The old version caused a compile warning on Linux.
>
>
>
> Best regards
>
> Christoph
>
>
>
> From: Langer, Christoph
> Sent: Mittwoch, 30. November 2016 19:01
> To: 'Michael McMahon' <[hidden email]>
> Cc: [hidden email]; Chris Hegarty <[hidden email]>;
> Lindenmaier, Goetz <[hidden email]>
> Subject: RE: RFR: 8170544: Fix code scan findings in libnet
>
>
>
> Thanks, Michael.
>
>
>
> Maybe you can run all the 3 together:
>
>
>
> http://cr.openjdk.java.net/~clanger/webrevs/8167457.1/
>
> http://cr.openjdk.java.net/~clanger/webrevs/8167420.2/
> <http://cr.openjdk.java.net/~clanger/webrevs/8167420.2/>
>
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/
>
>
>
> 8170544 comes atop 8167420 (though it's just a small clash).
>
>
>
> Best regards
>
> Christoph
>
>
>
>
>
> From: Michael McMahon [mailto:[hidden email]]
> Sent: Mittwoch, 30. November 2016 17:57
> To: Langer, Christoph <[hidden email]
> <mailto:[hidden email]> >
> Cc: [hidden email] <mailto:[hidden email]> ; Chris
> Hegarty <[hidden email] <mailto:[hidden email]> >;
> Lindenmaier, Goetz <[hidden email]
> <mailto:[hidden email]> >
> Subject: Re: RFR: 8170544: Fix code scan findings in libnet
>
>
>
> I will run the change through JPRT Christoph
>
> Thanks
> Michael
>
> On 30/11/2016, 16:32, Langer, Christoph wrote:
>
> Hi,
>
>
>
> please review the following change:
>
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8170544
>
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/
> <http://cr.openjdk.java.net/%7Eclanger/webrevs/8170544.0/>
>
>
>
> With this change I change some function signatures to use
> SOCKETADDRESS * instead of struct sockaddr * where appropriate. This fixes
> some code scan findings, which reported the fact that it's dangerous to pass
> struct socaddr * pointers and then cast it to the wider struct sockaddr_in6. I
> think this makes the code cleaner and spares some casts at several places.
>
>
>
> Please review and run through JPRT testing.
>
>
>
> Thanks
>
> Christoph
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8170544: Fix code scan findings in libnet

Langer, Christoph
Hi Goetz,

thanks for reviewing this.

I have addressed your comments in a new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170544.1/

Here's the details:

> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/share/na
> tive/libnet/net_util.c.udiff.html
> +     * check now to whether we have IPv6 on this platform and if the
> superfluous 'to'

Removed.

> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/nati
> ve/libnet/NetworkInterface.c.udiff.html
> For the records: you add closing the connection in an error case.

Yes.

> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/nati
> ve/libnet/net_util_md.c.udiff.html
> Is it safe to do memset here?  I think memset in the ipv4/v6 cases with the
> corresponding sizes is safer.
> Len is not passed in all the times. Else you could memset with len.
> (You change len to '0' in
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/nati
> ve/libnio/ch/DatagramChannelImpl.c.udiff.html )
> If you are sure you always pass a full SOCKETADDRESS this is fine, though.
> Same holds for the windows size.
> Overall, the len field is quite superfluous now, isn't it?  But this should not be
> changed in this change I think.

I think this is okay as is. NET_InetAddressToSockaddr takes a pointer to SOCKETADDRESS and then works on the pointed to memory (sizeof SOCKETADDRESS). Parameter len is only used as output, not as input that specifies the length of the incoming buffer. I have updated the documentation in http://cr.openjdk.java.net/~clanger/webrevs/8170544.1/src/java.base/share/native/libnet/net_util.h.cdiff.html

> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/windows
> /native/libnet/TwoStacksPlainDatagramSocketImpl.c.udiff.html
> Please add spaces around '=': fd=-1

Done in all places.

Best regards
Christoph

Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8170544: Fix code scan findings in libnet

Lindenmaier, Goetz
Hi Christoph,

Thanks for the fixes, looks good now!

Best regards,
  Goetz.



> -----Original Message-----
> From: Langer, Christoph
> Sent: Donnerstag, 29. Dezember 2016 14:21
> To: Lindenmaier, Goetz <[hidden email]>
> Cc: [hidden email]; Chris Hegarty <[hidden email]>;
> Michael McMahon <[hidden email]>
> Subject: RE: RFR: 8170544: Fix code scan findings in libnet
>
> Hi Goetz,
>
> thanks for reviewing this.
>
> I have addressed your comments in a new webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.1/
>
> Here's the details:
>
> >
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/share/na
> > tive/libnet/net_util.c.udiff.html
> > +     * check now to whether we have IPv6 on this platform and if the
> > superfluous 'to'
>
> Removed.
>
> >
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/nati
> > ve/libnet/NetworkInterface.c.udiff.html
> > For the records: you add closing the connection in an error case.
>
> Yes.
>
> >
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/nati
> > ve/libnet/net_util_md.c.udiff.html
> > Is it safe to do memset here?  I think memset in the ipv4/v6 cases with the
> > corresponding sizes is safer.
> > Len is not passed in all the times. Else you could memset with len.
> > (You change len to '0' in
> >
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/unix/nati
> > ve/libnio/ch/DatagramChannelImpl.c.udiff.html )
> > If you are sure you always pass a full SOCKETADDRESS this is fine, though.
> > Same holds for the windows size.
> > Overall, the len field is quite superfluous now, isn't it?  But this should not be
> > changed in this change I think.
>
> I think this is okay as is. NET_InetAddressToSockaddr takes a pointer to
> SOCKETADDRESS and then works on the pointed to memory (sizeof
> SOCKETADDRESS). Parameter len is only used as output, not as input that
> specifies the length of the incoming buffer. I have updated the documentation
> in
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.1/src/java.base/share/na
> tive/libnet/net_util.h.cdiff.html
>
> >
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.0/src/java.base/windows
> > /native/libnet/TwoStacksPlainDatagramSocketImpl.c.udiff.html
> > Please add spaces around '=': fd=-1
>
> Done in all places.
>
> Best regards
> Christoph

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8170544: Fix code scan findings in libnet

Chris Hegarty
In reply to this post by Langer, Christoph

> On 29 Dec 2016, at 13:20, Langer, Christoph <[hidden email]> wrote:
>
> Hi Goetz,
>
> thanks for reviewing this.
>
> I have addressed your comments in a new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170544.1/

This mainly looks fine. Just a few comments:

1) NetworkInterface.c

    I’m not sure that the close is really necessary, since a JNI pending
    exception can only be thrown is sock is less than 0. I think just
    removing the ' && (*env)->ExceptionOccurred(env)’ from the original
    if statement should be sufficient, no?

2) net_util.c  

    getInet6Address_scopeid_set should CHECK_NULL_RETURN(holder, JNI_FALSE)?
    getInet6Address_scopeid now returns an unsigned in, why CHECK_NULL_RETURN(holder, -1)?

    Some of this, existing, code seems a little dubious.

-Chris.
Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8170544: Fix code scan findings in libnet

Langer, Christoph
Hi Chris,

thanks for looking.

> 1) NetworkInterface.c
>
>     I’m not sure that the close is really necessary, since a JNI pending
>     exception can only be thrown is sock is less than 0. I think just
>     removing the ' && (*env)->ExceptionOccurred(env)’ from the original
>     if statement should be sufficient, no?

I think you are right. An exception can only occur if socket is less than 0. There is one case where socket could be less than 0 and no exception occurred. In that case we'd probably see an exception later on. But I think it would be fine to return NULL in case socket is < 0. Generally, this coding needs to be revisited in order to make it work for IPv6 only systems. We should do that when finishing up bug 8148424 [1].

> 2) net_util.c
>
>     getInet6Address_scopeid_set should CHECK_NULL_RETURN(holder, JNI_FALSE)?
>     getInet6Address_scopeid now returns an unsigned in, why CHECK_NULL_RETURN(holder, -1)?
>
>     Some of this, existing, code seems a little dubious.
>

Good catch. The CHECK_NULL_RETURN macros need adaption. The reason why getInet6Address_scopeid should return unsigned int is that the struct sockaddr_in6 is also using unsigned int for the scope, e.g. on Linux [2] or Windows [3].

I've addressed your points in http://cr.openjdk.java.net/~clanger/webrevs/8170544.2/ Would you want to run this through JPRT again?

I would go and push it towards the end of the week.

Best regards
Christoph

[1] https://bugs.openjdk.java.net/browse/JDK-8148424
[2] http://man7.org/linux/man-pages/man7/ipv6.7.html
[3] https://msdn.microsoft.com/en-us/library/windows/hardware/ff570824(v=vs.85).aspx


Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8170544: Fix code scan findings in libnet

Langer, Christoph
In reply to this post by Chris Hegarty
Hi,

I finally pushed the change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/f3115622562a

Best regards
Christoph

> -----Original Message-----
> From: Langer, Christoph
> Sent: Mittwoch, 11. Januar 2017 16:50
> To: 'Chris Hegarty' <[hidden email]>
> Cc: Lindenmaier, Goetz <[hidden email]>; net-
> [hidden email]; Michael McMahon <[hidden email]>
> Subject: RE: RFR: 8170544: Fix code scan findings in libnet
>
> Hi Chris,
>
> thanks for looking.
>
> > 1) NetworkInterface.c
> >
> >     I’m not sure that the close is really necessary, since a JNI pending
> >     exception can only be thrown is sock is less than 0. I think just
> >     removing the ' && (*env)->ExceptionOccurred(env)’ from the original
> >     if statement should be sufficient, no?
>
> I think you are right. An exception can only occur if socket is less than 0. There
> is one case where socket could be less than 0 and no exception occurred. In
> that case we'd probably see an exception later on. But I think it would be fine to
> return NULL in case socket is < 0. Generally, this coding needs to be revisited in
> order to make it work for IPv6 only systems. We should do that when finishing
> up bug 8148424 [1].
>
> > 2) net_util.c
> >
> >     getInet6Address_scopeid_set should CHECK_NULL_RETURN(holder,
> JNI_FALSE)?
> >     getInet6Address_scopeid now returns an unsigned in, why
> CHECK_NULL_RETURN(holder, -1)?
> >
> >     Some of this, existing, code seems a little dubious.
> >
>
> Good catch. The CHECK_NULL_RETURN macros need adaption. The reason why
> getInet6Address_scopeid should return unsigned int is that the struct
> sockaddr_in6 is also using unsigned int for the scope, e.g. on Linux [2] or
> Windows [3].
>
> I've addressed your points in
> http://cr.openjdk.java.net/~clanger/webrevs/8170544.2/ Would you want to
> run this through JPRT again?
>
> I would go and push it towards the end of the week.
>
> Best regards
> Christoph
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8148424
> [2] http://man7.org/linux/man-pages/man7/ipv6.7.html
> [3] https://msdn.microsoft.com/en-
> us/library/windows/hardware/ff570824(v=vs.85).aspx
>