Opened 12 years ago

Closed 10 years ago

#1798 defect closed duplicate (duplicate)

iocpreactor should use GetAcceptExSockaddrs instead of getpeername for incoming connections

Reported by: ghazel Owned by:
Priority: highest Milestone:
Component: core Keywords: win32
Cc: teratorn, PenguinOfDoom, TimothyFitz Branch:


socket.getpeername() (despite SO_UPDATE_ACCEPT_CONTEXT) does not always work on incoming connections. The result in this failure case is:

print acc_sock.getpeername() #=> (0, '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00')

(you'll notice not only is the IP an integer 0, but the port is a string of 0s) The answer to this problem is to use GetAcceptExSockaddrs instead. I double-checked this, and it works:

print acc_sock.getpeername(), self.reactor.GetAcceptExSockaddrs(buffer)[1]
old:(0, '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00') new:('', 1619)

I propose this: t.i.iocp\ AcceptOpEx.ovDone:

localaddr, remoteaddr = self.reactor.GetAcceptExSockaddrs(buffer)
self.transport.acceptDone(acc_sock, remoteaddr)

And then the ServerSocket should take the client tuple the same way t.i.tcp Server does. You'll also notice that t.i.tcp also uses the return remote addr from accept().

Change History (16)

comment:1 Changed 12 years ago by ghazel

Keywords: review added
Priority: normalhighest

There is a branch with this change called 'acceptex-1798'

comment:2 Changed 12 years ago by Glyph

Owner: changed from Glyph to teratorn

I don't know how to review this. Tag, Mr. Windows!

comment:3 Changed 12 years ago by Jean-Paul Calderone

Cc: teratorn PenguinOfDoom added

This really needs to get reviewed. Two weeks is too long for a branch to hang around without even being commented on. Can someone with a Win32 dev environment and some familiarity with this code please take a look?

comment:4 Changed 11 years ago by Jean-Paul Calderone

Cc: TimothyFitz added

Another week gone by....

comment:5 Changed 11 years ago by teratorn

I'm putting this on my TODO shortlist. Not exactly sure when I will get to it.

comment:6 Changed 11 years ago by teratorn

Keywords: win32 added
Status: newassigned

comment:7 Changed 11 years ago by TimothyFitz

Note that this doesn't build on any buildbots, because they can't find addrinfo.h (which exists as python-src\Modules\addrinfo.h). py-src\Modules isn't on the path, should it be?

comment:8 Changed 11 years ago by teratorn

ghazel, I've had a look through the win32 Platform SDK regarding these functions, and I'm curious to know under what conditions getpeername() fails.

Do you think you could come up with a failing test case and post it here?

comment:9 Changed 11 years ago by ghazel

The failing test case probably goes something like: Install Win2k SP4 Run a test which checks the local endpoint

I've seen it happen in WinXP as well, but it's much more intermittent.

comment:10 Changed 11 years ago by ghazel

I meant remote endpoint, of course.

comment:11 Changed 11 years ago by teratorn

ghazel, GetAcceptExSockaddrs() doesn't strike me as an *obviously* correct API to use. The MS docs say it's only to be used in conjunction with AcceptEx(). Perhaps it works by accident, or perhaps iocpreactor is actually using AcceptEx() at some lower Windows layer.

GetAcceptExSockaddrs doc:

I'm not a WinSock programmer, but I have a *little* bit of a hard time believing that an old, mainstream, documented, function like getpeername() is basically broken on various version of Windows up to and including XP. I'm much more inclined to think that we're misusing the API somehow.

PenguinOfDoom, do you have any thoughts on this?

comment:12 Changed 11 years ago by PenguinOfDoom

No, that's how it works. Sockets spat out by AcceptEx are not your normal sockets. They are crazy winsock extension sockets that only support certain operations, getpeername not included. See the AcceptEx page for details. iocpreactor2 handles this case.

comment:13 Changed 11 years ago by radix


what's the current status of this ticket? It's got the "review" tag but nobody seems to be reviewing it.

comment:14 Changed 11 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from teratorn to PenguinOfDoom
Status: assignednew
+    // shouldn't we be increfing these?

Find out, Make a decision, remove the comment, then merge.

comment:15 Changed 10 years ago by PenguinOfDoom

Resolution: duplicate
Status: newclosed

This is fixed in the new iocpreactor. See #1760

comment:16 Changed 7 years ago by <automation>

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