Datagram socket leak
Michael McMahon
michael.x.mcmahon at oracle.com
Wed Sep 7 09:24:26 PDT 2011
Hi all,
I've posted a webrev for this at:
http://cr.openjdk.java.net/~michaelm/7085981/webrev.1/
After checking Socket and ServerSocket, I believe they are not actually
affected by this issue. The IllegalArgumentExceptions are thrown before the
impl is created in both cases.
Thanks
Michael.
On 01/09/11 18:33, Michael McMahon wrote:
> Right. That would work assuming createImpl() doesn't leave the socket
> open
> if it throws an exception, which seems to be the case.
>
> On 01/09/11 18:05, Salter, Thomas A wrote:
>> Maybe I posted a bad patch, but my intent was to do the try-finally
>> after checking for a non-null bind address.
>>
>> public DatagramSocket(SocketAddress bindaddr) throws
>> SocketException {
>> // create a datagram socket.
>> createImpl();
>> if (bindaddr != null) {
>> try {
>> bind(bindaddr);
>> } finally {
>> if( !isBound() )
>> close();
>> }
>> }
>> }
>>
>> On a related note, I've also noticed that the Socket and ServerSocket
>> constructors can throw without closing the implementation, but this
>> only happens with IllegalArgumentException or other
>> RuntimeExceptions. I'm not sure what your policy is on cleaning up
>> after runtime exceptions.
>>
> The SocketImpl finalizer does eventually clean up. But, I agree we
> shouldn't rely on that
> especially in cases like the IAE explicitly thrown in the constructor.
> We'll include those
> cases in the fix for this bug as well.
>
> Michael.
>> -----Original Message-----
>> From: Michael McMahon [mailto:michael.x.mcmahon at oracle.com]
>> Sent: Thursday, September 01, 2011 12:47 PM
>> To: Salter, Thomas A
>> Cc: net-dev at openjdk.java.net
>> Subject: Re: Datagram socket leak
>>
>> Thomas,
>>
>> Thanks for pointing this out. We overlooked this in the recent change in
>> this
>> area. One thing though, in the second change to DatagramSocket we can't
>> just check for isBound() since the socket might legitimately be unbound
>> (bindaddr is null). All I can think is that we catch the exception and
>> re throw
>> it, after closing, rather than use a finally() in that case. I have
>> created a bug report (7085981)
>> to track this. I'll post a webrev for it soon.
>>
>> - Michael.
>>
>> On 29/08/11 20:01, Salter, Thomas A wrote:
>>> 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:chris.hegarty at oracle.com]
>>> Sent: Monday, August 29, 2011 2:33 PM
>>> To: Salter, Thomas A
>>> Cc: net-dev at openjdk.java.net
>>> 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:chris.hegarty at oracle.com]
>>>> Sent: Monday, August 29, 2011 1:56 PM
>>>> To: Salter, Thomas A
>>>> Cc: net-dev at openjdk.java.net
>>>> 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
>>>>>
>
More information about the net-dev
mailing list