Opened 5 years ago

Closed 4 years ago

#4172 defect closed fixed (fixed)

UDP crashes python.exe when using IOCPReactor

Reported by: JoshAlbrecht Owned by:
Priority: highest Milestone:
Component: core Keywords: windows
Cc: itamar Branch: branches/recvfrom-segfault-4172
(diff, github, buildbot, log)
Author: Launchpad Bug:

Description

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

Change History (14)

comment:1 Changed 5 years ago by exarkun

  • Keywords windows added
  • Owner changed from glyph to PenguinOfDoom
  • Priority changed from highest to normal

comment:2 Changed 5 years ago by PenguinOfDoom

Argh :( Confirmed on my machine.

comment:3 Changed 5 years ago by itamar

  • Cc itamar added

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

comment:4 Changed 5 years ago by pahan

  • Author set to pahan
  • Branch set to branches/recvfrom-segfault-4172

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

comment:5 Changed 5 years ago by PenguinOfDoom

  • Author pahan deleted
  • Branch branches/recvfrom-segfault-4172 deleted
  • Keywords review added
  • Owner changed from PenguinOfDoom to exarkun
  • Priority changed from normal to highest

comment:6 Changed 5 years ago by exarkun

  • Owner exarkun deleted

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

comment:7 follow-up: Changed 5 years ago by itamar

  • Keywords review removed
  • Owner set to PenguinOfDoom

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.

comment:8 Changed 5 years ago by 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.

comment:9 in reply to: ↑ 7 Changed 5 years ago by 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?

comment:10 Changed 4 years ago by PenguinOfDoom

  • Keywords review added
  • Owner PenguinOfDoom deleted

comment:11 Changed 4 years ago by PenguinOfDoom

  • Branch set to branches/recvfrom-segfault-4172

comment:12 Changed 4 years ago by therve

  • Keywords review removed
  • Owner set to PenguinOfDoom

Life is terrible, please merge.

comment:13 Changed 4 years ago by pahan

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:14 Changed 3 years ago by <automation>

  • Owner PenguinOfDoom deleted
Note: See TracTickets for help on using tickets.