Opened 16 years ago
Closed 14 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: | |
Author: |
Description
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:('127.0.0.1', 1619)
I propose this: t.i.iocp\ops.py 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 16 years ago by
Keywords: | review added |
---|---|
Priority: | normal → highest |
comment:2 Changed 16 years ago by
Owner: | changed from Glyph to teratorn |
---|
I don't know how to review this. Tag, Mr. Windows!
comment:3 Changed 16 years ago by
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:5 Changed 16 years ago by
I'm putting this on my TODO shortlist. Not exactly sure when I will get to it.
comment:6 Changed 16 years ago by
Keywords: | win32 added |
---|---|
Status: | new → assigned |
comment:7 Changed 16 years ago by
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 16 years ago by
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 16 years ago by
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:11 Changed 16 years ago by
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: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winsock/winsock/getacceptexsockaddrs_2.asp
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 16 years ago by
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 16 years ago by
ping!
what's the current status of this ticket? It's got the "review" tag but nobody seems to be reviewing it.
comment:14 Changed 16 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from teratorn to PenguinOfDoom |
Status: | assigned → new |
+ // shouldn't we be increfing these?
Find out, Make a decision, remove the comment, then merge.
comment:15 Changed 14 years ago by
Resolution: | → duplicate |
---|---|
Status: | new → closed |
This is fixed in the new iocpreactor. See #1760
comment:16 Changed 11 years ago by
Owner: | PenguinOfDoom deleted |
---|
There is a branch with this change called 'acceptex-1798'