Here's what I changed. I'm working with the fcs source bundle for b147,
27_jun_2011, so I may not have the latest source base.
Left base folder: new
Right base folder: b147
File: src\share\classes\java\net\DatagramSocket.java
186,189d185
< finally {
< if( !isBound() )
< close();
< }
234d229
< try {
236,239d230
< } finally {
< if( !isBound() )
< close();
< }
File: src\share\classes\java\net\MulticastSocket.java
165d164
< try {
167,170d165
< } finally {
< if( !isBound() )
< close();
< }
-----Original Message-----
From: Chris Hegarty [mailto:[email protected]]
Sent: Monday, August 29, 2011 2:33 PM
To: Salter, Thomas A
Cc: [email protected]
Subject: Re: Datagram socket leak
Ah ok. I finally get it.
In which case I think you original changes should be fine. Do you want
to make similar changes to MulticastSocket and post the diffs?
Also, I think a testcase would be useful here. I know it's not strictly
specified that the socket should be closed if the constructor throws,
but it does seem desirable.
-Chris.
On 08/29/11 07:14 PM, Salter, Thomas A wrote:
> I believe you're referring to the close() in the catch clause following the
> call to getImpl().bind. The problem I encountered was when the Datagram.bind
> threw an exception before it got that far. In my case, the checkListen was
> throwing a SecurityException, but any of the earlier throws would cause the
> same problem. The SecurityException wouldn't have been caught by the catch
> addressed by the CR in any case. We encountered this while running the TCK.
> One of its tests tries to create lots of sockets, all of them getting
> security violations until we hit a limit on the number of open sockets.
>
> public synchronized void bind(SocketAddress addr) throws SocketException
> {
> if (isClosed())
> throw new SocketException("Socket is closed");
> if (isBound())
> throw new SocketException("already bound");
> if (addr == null)
> addr = new InetSocketAddress(0);
> if (!(addr instanceof InetSocketAddress))
> throw new IllegalArgumentException("Unsupported address type!");
> InetSocketAddress epoint = (InetSocketAddress) addr;
> if (epoint.isUnresolved())
> throw new SocketException("Unresolved address");
> InetAddress iaddr = epoint.getAddress();
> int port = epoint.getPort();
> checkAddress(iaddr, "bind");
> SecurityManager sec = System.getSecurityManager();
> if (sec != null) {
> sec.checkListen(port);<<<<<<<<<<<<<<<<<<<<<<<<<< This throws a
> SecurityException
> }
> try {
> getImpl().bind(port, iaddr);
> } catch (SocketException e) {
> getImpl().close();
> throw e;
> }
> bound = true;
> }
>
> Tom.
>
>
> -----Original Message-----
> From: Chris Hegarty [mailto:[email protected]]
> Sent: Monday, August 29, 2011 1:56 PM
> To: Salter, Thomas A
> Cc: [email protected]
> Subject: Re: Datagram socket leak
>
> [take two!]
>
> Tom,
>
> This specific area of code was changed recently due to CR 7035556 [1],
> changeset [2], and this issue was discussed during the code review [3].
>
> Essentially, bind() already closes the impl internally before throwing
> the exception. Does this resolve the issue for you? Or do you still see
> potential to leak?
>
> -Chris
>
> [1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7035556
> [2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/07a12583d4ea
> [3] http://mail.openjdk.java.net/pipermail/net-dev/2011-July/003318.html
>
> On 08/29/11 03:27 PM, Salter, Thomas A wrote:
>> There appears to be a socket leak in both DatagramSocket and
>> MulticastSocket constructors. Both classes have constructors that create
>> a socket and then attempt to bind. The bind can fail with a variety of
>> exceptions none of which are caught by the constructor. Thus, the actual
>> system socket that was allocated by impl.create() is never closed.
>>
>> My fix was to wrap a try-finally around the bind call and call close()
>> if isBound is false.
>>
>> public DatagramSocket(SocketAddress bindaddr) throws SocketException {
>>
>> // create a datagram socket.
>>
>> createImpl();
>>
>> if (bindaddr != null) {
>>
>> try {
>>
>> bind(bindaddr);
>>
>> } finally {
>>
>> if( !isBound() )
>>
>> close();
>>
>> }
>>
>> }
>>
>> }
>>
>> Tom Salter
>>
>> Unisys Corporation
>>
