[SNMP4J] Another race condition fix forDefaultUdpTransportMapping

[Paul Marquis]

The outer try/catch block already catches IOException, so it will be  
handled there.  SocketException and ClosedChannelException are  
subclasses of IOException, so they will be handled in the same catch  
block as IOException.

On Sep 27, 2010, at 5:47 AM, Tosoni wrote:

> Oh yes, you are correct and I am wrong ! I apologize.
> But then we should catch IOException or ClosedChannelException ?
> And do the same for the other uses of 'socket' in this function ?
>
>> From: Paul Marquis [mailto:paul at viridity.com]
> (snip)
>>
>> If another thread closes where you say, you should get an
>> IOException or SocketException, not a NullPointerException,
>> when receive() is called.  The socket member variable may
>> change to null, but socketCopy will not.
>>
>> On Sep 27, 2010, at 3:38 AM, Tosoni wrote:
>>
>>> Sorry guys, but I don't believe your tries fix the race for real.
>>>
>>> The problem is that a socket.close() can occur just before the
>>> socket.receive()
>>> If you just test the socket value, this does not forbit a
>>> socket.close()
>>> happening *after*
>>> your test and *before* the socket.receive().> >           try {
>>>>>           DatagramSocket socketCopy = socket;
>>>>>           if (socketCopy == null) {
>>>>>             stop = true;
>>>>>             continue;
>>>>>           }
>>> 	*** what if another thread closes at this point ? ***
>>>>>           socketCopy.receive(packet);
>>>>>         }
>>>
>>> The real fix must use some semaphore or mutex which embraces:
>>> 	on one hand, both the test of 'socket==null' and the receive()
>>> 	on the other hand, both the change 'socket=null' and the close()
>>>
>>> My own try (I do not suggest it as a correct fix) is a quick-and-
>>> dirty:
>>>         try {
>>>           socket.receive(packet);
>>>         }
>>>         catch (InterruptedIOException iiox) {
>>>           if (iiox.bytesTransferred <= 0) {
>>>             continue;
>>>           }
>>>         }
>>> +          // UGLY FIX: socket close/receive synchro
>>> +          catch (NullPointerException iiox) {
>>> +        	  // This handles the close race (stop=false but
>>> socket=null)
>>> +              continue;
>>> +		}
>>>
>>>
>>> Hmm, also, there is another race condition which happens
>> sometimes,
>>> here:
>>> 	      public void run() {
>>>     	  try {
>>> 	here===>    socket.setSoTimeout(getSocketTimeout());
>>> Really I guess it can happen everywhere 'socket' is used in a thread
>>> different than the closing thread...
>>>
>>> Regards
>>>
>>>> -----Original Message-----
>>>> From: snmp4j-bounces at agentpp.org [mailto:snmp4j-
>>>> bounces at agentpp.org]On
>>>> Behalf Of Paul Marquis
>>>> Sent: Friday, September 24, 2010 1:32 AM
>>>> To: Frank Fock
>>>> Cc: snmp4j at agentpp.org
>>>> Subject: Re: [SNMP4J] Another race condition fix
>>>> forDefaultUdpTransportMapping
>>>>
>>>>
>>>> Thanks for fixing this!
>>>>
>>>> Any idea when there will be an official release with this fix
>>>> in it?
>>>> I just joined the list, so if you've already discussed
>> when the next
>>>> release will be available, I apologize.
>>>>
>>>> On Sep 23, 2010, at 7:24 PM, Frank Fock wrote:
>>>>
>>>>> Hi Paul,
>>>>>
>>>>> Thank you for the bug report. If fixed it a bit different:
>>>>>
>>>>>         try {
>>>>>           DatagramSocket socketCopy = socket;
>>>>>           if (socketCopy == null) {
>>>>>             stop = true;
>>>>>             continue;
>>>>>           }
>>>>>           socketCopy.receive(packet);
>>>>>         }
>>>>>
>>>>> Best regards,
>>>>> Frank
>>>>>
>>>>> On 23.09.2010 23:44, Paul Marquis wrote:
>>>>>> Version 1.11.1 of SNMP4J included a fix for a race condition in
>>>>>> DefautlUdpTrasnportMapping which caused a NullPointerException.
>>>>>> However, there is still one that remains while
>> attempting to read a
>>>>>> packet and I see this from time to time.  Below is a patch
>>>> that fixes
>>>>>> the problem for me that I'd like to submit for review.
>>>>>>
>>>>>> Index: src/org/snmp4j/transport/DefaultUdpTransportMapping.java
>>>>>>
>> ===================================================================
>>>>>> ---
>>>> src/org/snmp4j/transport/DefaultUdpTransportMapping.java
>>>>>> (revision
>>>>>> 215)
>>>>>> +++
>>>> src/org/snmp4j/transport/DefaultUdpTransportMapping.java	(working
>>>>>> copy)
>>>>>> @@ -337,7 +337,11 @@
>>>>>>
>>>>>> udpAddress.getPort());
>>>>>>         try {
>>>>>>           try {
>>>>>> -            socket.receive(packet);
>>>>>> +            DatagramSocket readingSocket = socket;
>>>>>> +            if (readingSocket == null) {
>>>>>> +              continue;
>>>>>> +            }
>>>>>> +            readingSocket.receive(packet);
>>>>>>           }
>>>>>>           catch (InterruptedIOException iiox) {
>>>>>>             if (iiox.bytesTransferred<= 0) {
>>>>>>
>>>>>> _______________________________________________
>>>>>> SNMP4J mailing list
>>>>>> SNMP4J at agentpp.org
>>>>>> http://lists.agentpp.org/mailman/listinfo/snmp4j
>>>>>
>>>>> --
>>>>> AGENT++
>>>>> http://www.agentpp.com
>>>>> http://www.snmp4j.com
>>>>> http://www.mibexplorer.com
>>>>> http://www.mibdesigner.com
>>>>>
>>>>> _______________________________________________
>>>>> SNMP4J mailing list
>>>>> SNMP4J at agentpp.org
>>>>> http://lists.agentpp.org/mailman/listinfo/snmp4j
>>>>
>>>> _______________________________________________
>>>> SNMP4J mailing list
>>>> SNMP4J at agentpp.org
>>>> http://lists.agentpp.org/mailman/listinfo/snmp4j
>>>>
>>>
>>
>>