Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
gh-120754: Add a strace helper and test set of syscalls for open().read(), Take 2#123413
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…s for o… (python#123303)This reverts commit52caaef.
NOTE: This needs a full buildbot test pass before merge, see:python#121143 (comment).1. Added `statx` to set of allowed syscall forms (Should make Raspian bot pass).2. Check that the `fd` returned from `open` is passed to all future calls. This helps ensure things like the `stat` call uses the file descriptor rather than the `filename` to avoid TOCTOU isuses.3. Update the `Path().read_bytes()` test case to additionally validate the reduction in`isatty`/`ioctl` + `seek` calls frompython#1221114. Better diagnostic assertion messagess from@gpshead, so when the test fails have first information immediately available. Makes remote CI debugging much simpler.
bedevere-bot commentedAug 28, 2024
🤖 New build scheduled with the buildbot fleet by@hauntsaninja for commite196d3d 🤖 If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
cmaloney commentedAug 29, 2024
I looked into the buildbot failures and they don't seem related to the changes I make here (full details below). Raspbian bot which failed post-merge on last PR, does pass here. Investigating buildbot failures:
test.test_asyncio.test_server.TestServer2.test_abort_clients```0:01:43 load avg: 8.22 [353/479/1] test.test_asyncio.test_server failed (1 failure)test_start_server_1 (test.test_asyncio.test_server.ProactorStartServerTests.test_start_server_1) ... skipped 'Windows only'test_start_server_1 (test.test_asyncio.test_server.SelectorStartServerTests.test_start_server_1) ... oktest_start_unix_server_1 (test.test_asyncio.test_server.SelectorStartServerTests.test_start_unix_server_1) ... oktest_abort_clients (test.test_asyncio.test_server.TestServer2.test_abort_clients) ... FAILtest_close_clients (test.test_asyncio.test_server.TestServer2.test_close_clients) ... oktest_wait_closed_basic (test.test_asyncio.test_server.TestServer2.test_wait_closed_basic) ... oktest_wait_closed_race (test.test_asyncio.test_server.TestServer2.test_wait_closed_race) ... oktest_unix_server_addr_cleanup (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_addr_cleanup) ... oktest_unix_server_cleanup_gone (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_cleanup_gone) ... oktest_unix_server_cleanup_prevented (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_cleanup_prevented) ... oktest_unix_server_cleanup_replaced (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_cleanup_replaced) ... oktest_unix_server_sock_cleanup (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_sock_cleanup) ... ok====================================================================== |
cmaloney commentedAug 29, 2024
Bots which failed around |
cmaloney commentedOct 3, 2024
The |
cmaloney commentedOct 8, 2024
Could this get the "no news" label? (Internal changes). I think this is ready for a more full review again + run bots (merged current main in recently) Recent changes:
|
Uh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedNov 2, 2024
🤖 New build scheduled with the buildbot fleet by@hauntsaninja for commit9e8b23d 🤖 If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
cmaloney commentedNov 2, 2024
Buildbots look fairly clean. Three failed (AMD64 Windows10 PR, ARM64 Windows Non-Debug PR, ARM64 Windows PR), all with issues in |
hauntsaninja left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 sticking with this change!
It looks like if unknown syscalls are introduced the current test is fine with that. If you wanted to make a follow-up PR tightening that up, I'd be in favour of it. (I acknowledge that that maybe contradicts the advice in#121143 (comment) , but I think the similar name logic helps address the maintainability concern)
9ad57cf intopython:mainUh oh!
There was an error while loading.Please reload this page.
cmaloney commentedNov 3, 2024
This does prevent additional |
mgorny commentedNov 20, 2024
All the newly added tests fail on Gentoo Linux with sandbox enabled. From a quick check, the test wrongly assumes that the first syscall reported by If I add some prints to get the list of syscalls, I get e.g. the following: i.e. there's a bunch of extra "preparatory" calls before the |
mgorny commentedNov 20, 2024
Besides, given how fragile this thing is by design, there should be an easy way to disable it (e.g. via |
mgorny commentedNov 20, 2024
It's broken on musl (without any special environment) too: |
cmaloney commentedNov 20, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Happy to work on making the state better on Gentoo. There are tradeoffs in the fragility, and I'm happy to iterate to get everything working well. Unfortunately not having a check like this has resulted in syscalls being re-added unintentionally (ex.bpo-21679 removed a double stat andgh-120754 removed a double stat again). In the short term I think the There looks like three cases to investigate/work on from this:
For 3, did anything in For all three of those I think the standard python process is either open a new issue or reopen thegh-120754 one. Happy to open one so can track these changes and get a fix landed. |
mgorny commentedNov 20, 2024
On a glibc system, only Could you open the bug, please? I still have lots to do today. |
Uh oh!
There was an error while loading.Please reload this page.
ReapplyGH-121143 with additional changes after it was reverted inGH-123303. Investigation why the bot broke + suggestions for improvements in#121143 (comment)
Note: This needs a buildbot trigger + pass before merge, see:#121143 (comment).
Changes fromGH-121143:
statxto set of allowed syscall forms (Should make Raspian bot pass).fdreturned from theopencall is passed to all future calls. This helps ensure that thestatcall uses the file descriptor rather than thefilenameto avoid TOCTOU isuses.Path().read_bytes()test case to additionally validate the reduction inisatty/ioctl+seekcalls fromGH-120754: Disable buffering in Path.read_bytes #122111.