gh-102336: remove windows 7 special handling#102337
gh-102336: remove windows 7 special handling#102337zooba merged 4 commits intopython:mainfrom maxbachmann:patch-8
Conversation
maxbachmann
changed the title
|
Does something like this need / should have a news entry? |
|
|
||
| if (!SetHandleInformation((HANDLE)newfd, HANDLE_FLAG_INHERIT, 0)) { | ||
| closesocket(newfd); | ||
| PyErr_SetFromWindowsErr(0); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
@eryksun are you sure this is the correct change? This seems to break the tests.
There was a problem hiding this comment.
Sorry. I forgot that the WSASocketW() call in this case isn't responsible for duplicating the handle into the current process. It's implemented the other way around. The API can't assume that the process that calls WSASocketW() is allowed to duplicate a handle from the process that calls WSADuplicateSocketW(). Instead the value of the already duplicated handle is passed in the WSAPROTOCOL_INFOW record. This handle is inheritable, so calling SetHandleInformation() is required in this case.
There was a problem hiding this comment.
Calling SetHandleInformation() is thus also required in sock_initobj_impl() for the case where a socket is created with FROM_PROTOCOL_INFO. I'd do the SetHandleInformation() call locally right after calling WSASocketW(), and add a comment that explains why WSA_FLAG_NO_HANDLE_INHERIT doesn't work.
The other WSASocketW() call in sock_initobj_impl() doesn't need SetHandleInformation(). Keeping it separate and adding a comment makes it less likely that we'll mistakenly go down this path again.
Yeah, I think it should. But it's got one now. The change looks good to me overall, and passes CI. If you think it's ready, I'll go ahead and merge, or let me know if you want to hold off for a bit. |
|
Yes from my side this can be merged. |
|
Thanks for the quick response! |
4 participants
This pulls out the part of #102256 cleaning up old Windows 7 specific code to simplify the review of this PR.