UDP crashes python.exe when using IOCPReactor

[JoshAlbrecht]

The program below causes python.exe to crash. I've tested on Windows XP 32Bit, Windows 7 64bit, and I've tested with Python 2.5.2 and 2.6.4, and I've tested using both Twisted 9.0.0 and 8.2.0. It has crashed with every combination.

One interesting thing to note is that it crashes with no output if you comment out the for loop (even with the for loop, it crashes with no output on my windows XP system). Usually I get this output though:

C:\Python25\lib\site-packages\twisted\internet\iocpreactor\reactor.py:29: UserWarning: pyOpenSSL 0.10 or newer is required for SSL support in iocpreactor. It is missing, so the reactor will not support SSL APIs.

"pyOpenSSL 0.10 or newer is required for SSL support in iocpreactor. "

(('127.0.0.1', 6951), 'HELLO') (('127.0.0.1', 33351), 'HELLO')

Sometimes more or fewer HELLO lines are printed.

from twisted.internet.iocpreactor.reactor import IOCPReactor
from twisted.internet.main import installReactor
from twisted.internet.protocol import DatagramProtocol


reactor = IOCPReactor()
installReactor(reactor)


DATA = 'HELLO'
REMOTE_IP = "127.0.0.1"
REMOTE_PORT = 33351


class EchoDatagram(DatagramProtocol):       
  def datagramReceived(self, datagram, addr):
    print((addr, datagram))
    self.transport.write(datagram, addr)
    

p1 = EchoDatagram()
listening_port1 = reactor.listenUDP(6951, p1, interface='')
p2 = EchoDatagram()
listening_port2 = reactor.listenUDP(REMOTE_PORT, p2, interface=REMOTE_IP)
    

def pointless_write():
  p1.transport.write(DATA, (REMOTE_IP, REMOTE_PORT))
  
  for i in xrange(10):
    f = open("temp1", "wb")
    f.write("Hi!")
    f.close()
    f = open("temp1", "rb")
    data = f.read()
    f.close()
    
  reactor.callLater(1, pointless_write)


reactor.callLater(1, pointless_write)
reactor.run()

[PenguinOfDoom]

Argh :( Confirmed on my machine.

[itamar]

Any idea why the test suite doesn't catch this?

[pahan]

(In [27940]) Branching to 'recvfrom-segfault-4172'

[exarkun]

I love Windows and Windows development so much that I couldn't possibly horde this ticket all for myself.

[itamar]

Was the test suite reproducing this crash? If not, you need to add an appropriate test. With that, and a short explanation of the what the issue was and how you fixed it, I could do a real review.

[exarkun]

Complete test coverage is awesome and we should always have it.

However... the problem here (AIUI) was that an uninitialized pointer was passed to the Windows kernel to hold the address the datagram was received from. This "worked" most of the time because addresses are small and there's lots of random memory you can overwrite without immediate disastrous consequences. The test suite does exercise this code path, but usually it doesn't crash because it writes to a semi-harmless location.

Making this crash deterministically with the buggy implementation sounds really, really hard to me. Really, this is the kind of bug you want a memory debugger to be pointing out to you.

Alternatively, we could say this is a bug in Pyrex(/Cython) for allowing you to have uninitialized pointers at all. Actually, I wonder what would happen if someone were to make a much more limited change to the current trunk version: initialize fromaddr in recvfrom to NULL. That might be more likely to deterministically segfault.

[PenguinOfDoom]

Replying to itamar:

Was the test suite reproducing this crash? If not, you need to add an appropriate test. With that, and a short explanation of the what the issue was and how you fixed it, I could do a real review.

From  http://msdn.microsoft.com/en-us/library/ms741686(VS.85).aspx:

Also, the values indicated by lpFrom and lpFromlen are not updated until completion is itself indicated. Applications must not use or disturb these values until they have been updated, therefore the application must not use automatic (that is, stack-based) variables for these parameters.

Right now, IOCP's recvfrom wrapper uses a stack buffer for lpFromlen. Some time after the wrapper returns, Windows code from otherwhere clobbers the stack location. Undefined behavior ensues. What sort of unit test would you recommend to cover this situation?

[therve]

Life is terrible, please merge.

[pahan]

(In [28414]) Merge recvfrom-segfault-4172

Author: PenguinOfDoom Reviewer: therve Fixes #4172

Allocate lpFromlen for WSARecvFrom on the heap instead of the stack. Change iocpsupport from Pyrex to Cython and fix a minor related issue.