Skip to content
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

gh-120754: Add a strace helper and test set of syscalls for open().read(), Take 2 #123413

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

cmaloney
Copy link
Contributor

@cmaloney cmaloney commented Aug 28, 2024

Reapply GH-121143 with additional changes after it was reverted in GH-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 from GH-121143:

  1. Added statx to set of allowed syscall forms (Should make Raspian bot pass).
  2. Check that the fd returned from the open call is passed to all future calls. This helps ensure that 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 inisatty/ioctl + seek calls from GH-120754: Disable buffering in Path.read_bytes #122111.
  4. Better diagnostic assertion messages from @gpshead, so when the test fails have first information immediately available. Makes remote CI debugging much simpler.

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 from python#122111
4. Better diagnostic assertion messagess from @gpshead, so when the test fails have first information immediately available. Makes remote CI debugging much simpler.
@hauntsaninja hauntsaninja added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 28, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @hauntsaninja for commit e196d3d 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 28, 2024
@cmaloney
Copy link
Contributor Author

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:

  1. AMD64 Arch Linux Usan:
    - C compilation issue, seems unrelated to these changes
  2. PPC64LE RHEL8 Refleaks
    - test.test_concurrent_futures.test_shutdown timed out after 3 hour 49 min
    - seems to be: test_concurrent_futures.test_shutdown: AssertionError: b'' != b'apple' #112542
  3. test.test_asyncio.test_server.TestServer2.test_abort_clients failures
    • Also occur in main on same buildbots (ex. https://buildbot.python.org/#/builders/29/builds/6842)
    • AMD64 Fedora Stable Clang Installed
    • AMD64 Fedora Stable Clang PR
    • AMD64 Fedora Stable LTO + PGO
    • AMD64 Fedora Stable LTO PR
    • AMD64 Fedora Stable PR
    • note: I can get this same failure on my Arch Linux locally on main, may try investigating a bit more. Can't seem to find a issue for it currently.
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) ... ok test_start_unix_server_1 (test.test_asyncio.test_server.SelectorStartServerTests.test_start_unix_server_1) ... ok test_abort_clients (test.test_asyncio.test_server.TestServer2.test_abort_clients) ... FAIL test_close_clients (test.test_asyncio.test_server.TestServer2.test_close_clients) ... ok test_wait_closed_basic (test.test_asyncio.test_server.TestServer2.test_wait_closed_basic) ... ok test_wait_closed_race (test.test_asyncio.test_server.TestServer2.test_wait_closed_race) ... ok test_unix_server_addr_cleanup (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_addr_cleanup) ... ok test_unix_server_cleanup_gone (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_cleanup_gone) ... ok test_unix_server_cleanup_prevented (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_cleanup_prevented) ... ok test_unix_server_cleanup_replaced (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_cleanup_replaced) ... ok test_unix_server_sock_cleanup (test.test_asyncio.test_server.UnixServerCleanupTests.test_unix_server_sock_cleanup) ... ok

======================================================================
FAIL: test_abort_clients (test.test_asyncio.test_server.TestServer2.test_abort_clients)

Traceback (most recent call last):
File "/home/buildbot/buildarea/pull_request.cstratak-fedora-stable-x86_64.clang-installed/build/target/lib/python3.14/unittest/async_case.py", line 93, in _callTestMethod
result = self._callMaybeAsync(method)
File "/home/buildbot/buildarea/pull_request.cstratak-fedora-stable-x86_64.clang-installed/build/target/lib/python3.14/unittest/async_case.py", line 119, in _callMaybeAsync
return self._asyncioRunner.run(
~~~~~~~~~~~~~~~~~~~~~~~^
func(*args, **kwargs),
^^^^^^^^^^^^^^^^^^^^^^
context=self._asyncioTestContext,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
)
^
File "/home/buildbot/buildarea/pull_request.cstratak-fedora-stable-x86_64.clang-installed/build/target/lib/python3.14/asyncio/runners.py", line 118, in run
return self._loop.run_until_complete(task)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
File "/home/buildbot/buildarea/pull_request.cstratak-fedora-stable-x86_64.clang-installed/build/target/lib/python3.14/asyncio/base_events.py", line 721, in run_until_complete
return future.result()
~~~~~~~~~~~~~^^
File "/home/buildbot/buildarea/pull_request.cstratak-fedora-stable-x86_64.clang-installed/build/target/lib/python3.14/test/test_asyncio/test_server.py", line 249, in test_abort_clients
self.assertNotEqual(s_wr.transport.get_write_buffer_size(), 0)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0 == 0


Ran 12 tests in 0.250s

FAILED (failures=1, skipped=1)
test test.test_asyncio.test_server failed

@cmaloney
Copy link
Contributor Author

Bots which failed around test_asyncio should pass now with #123423

@cmaloney
Copy link
Contributor Author

cmaloney commented Oct 3, 2024

The Tests / Address sanitizer failure seems to be a case that needs to be handled, investigating it (an mmap shows up in the middle of the I/O calls). Could this get the no news tag (just changes python test. module pieces)

@cmaloney
Copy link
Contributor Author

cmaloney commented Oct 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:

  1. AddressSanitizer + LeakSanitizer can't be used under strace / ptrace (And new versions print a very helpful hint about this), added a skip for that so "Tests / Address sanitizer" should pass now.
  2. Updated for Avoid calling isatty() for most open() calls #90102 to validate the no isatty call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants