[SNMP4J] Another race condition fix forDefaultUdpTransportMapping

[Paul Marquis]

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
>>
>