-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
gh-143249: Fix buffer leak when overlapped operation fails to start (OS-Windows) #143250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need a NEWS entry for that and some tests as well. Please add them so that we can run them under the refleak bot as well.
cc @zooba
| Py_RETURN_NONE; | ||
| default: | ||
| self->type = TYPE_NOT_STARTED; | ||
| Overlapped_clear(self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was very confused with this, I thought it was tp_clear but it's explicitly not tp_clear so this looks like the correct fix.
This reverts commit 2508cfb.
Thanks for the review. I will add a suitable test case. |
| # If the exported buffer is still held, this will raise BufferError. | ||
| buf.append(1) | ||
|
|
||
| @support.refcount_test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this? I think you can just run -R : from the CLI to check if three is a refleak. It's more robust as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the guidance — that makes sense now. I’ve updated the test case, and here are the results from my local run.
Without the patch
d:\MyCode\cpython>PCbuild\amd64\python_d.exe -m test -R 3:3 test_asyncio.test_windows_utils
Using random seed: 514712329
0:00:00 Run 1 test sequentially in a single process
0:00:00 [1/1] test_asyncio.test_windows_utils
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
XX5 555
test_asyncio.test_windows_utils leaked [5, 5, 5] references, sum=15
0:00:01 [1/1/1] test_asyncio.test_windows_utils failed (reference leak)
== Tests result: FAILURE ==
1 test failed:
test_asyncio.test_windows_utils
Total duration: 1.2 sec
Total tests: run=6
Total test files: run=1/1 failed=1
Result: FAILUREwith the patch
d:\MyCode\cpython>PCbuild\amd64\python_d.exe -m test -R 3:3 test_asyncio.test_windows_utils
Using random seed: 3834115587
0:00:00 Run 1 test sequentially in a single process
0:00:00 [1/1] test_asyncio.test_windows_utils
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
XX. ...
0:00:01 [1/1] test_asyncio.test_windows_utils passed
== Tests result: SUCCESS ==
1 test OK.
Total duration: 1.2 sec
Total tests: run=6
Total test files: run=1/1
Result: SUCCESS
| ov.WSARecvFromInto(0x1234, buf, len(buf), 0) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ov.WSARecvFromInto(0x1234, buf, len(buf), 0) | |
| ov.WSARecvFromInto(0x1234, buf, len(buf), 0) | |
On Windows, some overlapped I/O operations leak buffers when they fail to start, notably in
WSASendTo(),WSARecvFrom(), andWSARecvFromInto().This change ensures that buffers are released on failure by clearing the overlapped state, following the same pattern used in other overlapped error paths. This issue is similar in nature to commit 5485085, which fixed related overlapped cleanup problems.
Python script for tracking leaks
Result without the patch
Result with the patch