RFR[8237890]: 'DatagramPacket::getSocketAddress doesn't specify what happens if address or port are not set'

[Patrick Concannon]

Hi,

Could someone please review my webrev and CSR for JDK-8237890 
'DatagramPacket::getSocketAddress doesn't specify what happens if 
address or port are not set' ?


DatagramPacket::getSocketAddress is misleading in that it can throw an 
IllegalArgumentException even though it doesn't take any arguments. This 
can occur if the port of a DatagramPacket is not set before a call to 
getSocketAddress is made.


In the recent fixJDK-8236940 
, additional checks 
were addedto ensure DatagramPacket cannot besent to port 0. Following on 
from this update,the current fix changes the default port of a 
DatagramPacket from -1 to 0. An IllegalArgumentException will therefore 
no longer be thrown when getSocketAddress with no port set. Instead, an 
InetSocketAddress representing any local address with port 0 is returned.



bug: https://bugs.openjdk.java.net/browse/JDK-8237890
csr: https://bugs.openjdk.java.net/browse/JDK-8242483
webrev: http://cr.openjdk.java.net/~pconcannon/8237890/webrevs/webrev.00/



Kind regards,

Patrick

Re: RFR[8237890]: 'DatagramPacket::getSocketAddress doesn't specify what happens if address or port are not set'

[Alan Bateman]

On 10/04/2020 11:16, Patrick Concannon wrote:


Hi,

Could someone please review my webrev and CSR for JDK-8237890 
'DatagramPacket::getSocketAddress doesn't specify what happens if 
address or port are not set' ?


DatagramPacket::getSocketAddress is misleading in that it can throw an 
IllegalArgumentException even though it doesn't take any arguments. 
This can occur if the port of a DatagramPacket is not set before a 
call to getSocketAddress is made.


In the recent fixJDK-8236940 
, additional checks 
were addedto ensure DatagramPacket cannot besent to port 0. Following 
on from this update,the current fix changes the default port of a 
DatagramPacket from -1 to 0. An IllegalArgumentException will 
therefore no longer be thrown when getSocketAddress with no port set. 
Instead, an InetSocketAddress representing any local address with port 
0 is returned.



bug: https://bugs.openjdk.java.net/browse/JDK-8237890
csr: https://bugs.openjdk.java.net/browse/JDK-8242483
webrev: http://cr.openjdk.java.net/~pconcannon/8237890/webrevs/webrev.00/

The default value for address and port are null and 0 (resp.) so you 
could just remove L93-94.


In the getSocketAddress() spec it might be better to say "Returns" 
rather than "Gets" so that its consistent with the getAddress and 
getPort methods.


Can you look at DatagramSocketAdaptor L202 where the comment is "throws 
IAE if port not set". I assume that comment should be removed.


Do we need to re-visit or withdraw the release note JDK-8237530 as IAE 
is there is no longer a "port out of range" condition?


-Alan

Re: RFR[8237890]: 'DatagramPacket::getSocketAddress doesn't specify what happens if address or port are not set'

[mark sheppard]

Hi Patrick,
if I understand the change correctly, you wish to eliminate the 
IllegalArgumentException and return an InetSocketAddress
based on the current set values for address and port. Because you have changed 
the default value of the port to 0, the getSocketAddress
will now return a SocketAddress with a wildcard address (assuming that address 
has not yet been set) and an ephemeral port, rather than the Exception caused 
by the port == -1.  Calling new InetSocketAddress(null, 0) will mean a wildcard 
address and an ephemeral port.
Port 0 is reserved for system selection of ephemeral port.  Thus, I think this 
change is now introducing some dubious semantics ?
Additionally if you were to call getSocketAddress multiple times you would get 
different non equal SocketAdress objects i.e. each one would have a different 
ephemeral port.

A possible approach is that an invocation of getSocketAddress should return 
null if the address or the port is not set, OR alternatively throw an 
IllegalStateException with either "port not set" or "address not set", as the 
DatagramPacket is in an unusable state for sending and the remote address is 
not set?

If we look at DatagramSocket::send() method , I suspect that there is another 
side effect to your change the check


if (packetPort < 0 || packetPort > 
0x)
throw new IllegalArgumentException("port out of range:" + 
packetPort);

will allow a DatagramPacket with port == 0  to continue its porcessing. But 
sending to port 0 is not allowed afaik
so should this be:
  if (packetPort <= 0 || packetPort > 0x)
  throw new IllegalArgumentException("port out of range: " + 
packetPort);

regards
Mark



From: net-dev  on behalf of Patrick Concannon 

Sent: Friday 10 April 2020 10:16
To: OpenJDK Network Dev list 
Subject: RFR[8237890]: 'DatagramPacket::getSocketAddress doesn't specify what 
happens if address or port are not set'


Hi,

Could someone please review my webrev and CSR for JDK-8237890 
'DatagramPacket::getSocketAddress doesn't specify what happens if address or 
port are not set' ?

DatagramPacket::getSocketAddress is misleading in that it can throw an 
IllegalArgumentException even though it doesn't take any arguments. This can 
occur if the port of a DatagramPacket is not set before a call to 
getSocketAddress is made.

In the recent fix 
JDK-8236940, additional 
checks were added to ensure DatagramPacket cannot be sent to port 0. Following 
on from this update, the current fix changes the default port of a 
DatagramPacket from -1 to 0. An IllegalArgumentException will therefore no 
longer be thrown when getSocketAddress with no port set. Instead, an 
InetSocketAddress representing any local address with port 0 is returned.

bug: https://bugs.openjdk.java.net/browse/JDK-8237890
csr: https://bugs.openjdk.java.net/browse/JDK-8242483
webrev: http://cr.openjdk.java.net/~pconcannon/8237890/webrevs/webrev.00/



Kind regards,

Patrick