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

pytest node crashes on windows in cirq_google serialization tests #5067

Open
maffoo opened this issue Mar 10, 2022 · 3 comments
Open

pytest node crashes on windows in cirq_google serialization tests #5067

maffoo opened this issue Mar 10, 2022 · 3 comments
Labels
area/testing kind/health For CI/testing/release process/refactoring/technical debt items

Comments

@maffoo
Copy link
Contributor

maffoo commented Mar 10, 2022

Description of the issue

I've seen a few pytest failures on windows in cirq-google serialization tests:

[gw1] win32 -- Python 3.7.9 c:\hostedtoolcache\windows\python\3.7.9\x64\python.exe
worker 'gw1' crashed while running 'cirq-google/cirq_google/serialization/serializable_gate_set_test.py::test_deserialize_infinite_recursion_fails'

I've seen this happen both in 3.7 tests and in 3.9 tests. Seems to be intermittent.

@maffoo maffoo added the kind/health For CI/testing/release process/refactoring/technical debt items label Mar 10, 2022
@95-martin-orion
Copy link
Collaborator

I've also seen this error, but at the time chalked it up to Github Actions runner instability. Seeing that it's still coming up - and that it's in the infinite recursion test - suggests I should revisit my code here.

@95-martin-orion 95-martin-orion self-assigned this Mar 10, 2022
@maffoo
Copy link
Contributor Author

maffoo commented Mar 10, 2022

It does say right in the test it's doing something sketchy :-)

# Maliciously modify the CircuitOperation to be self-referencing.

@95-martin-orion
Copy link
Collaborator

#4315 for context on what this test is about. The deserialization half of this test is fine - it will try to locate the recursively-defined operation in the constants map, fail, and error out.

The serialization half intentionally forces a RecursionError, but it's the safer variety: Python notices that the a method was called with the exact same arguments and system state and it fails after a single recursive call. It doesn't seem like this could cause a crash unless the Windows runners (a) disabled this recursion safeguard in their Python config, and (b) encountered a hardware issue (OOM, maybe worse?) before the "too many recursive calls" error triggered.

Given all of that, I feel pretty safe labeling this particular bit of malicious code as de-fanged 😝 Good to keep an eye on this, but at the moment it seems like only a minor nuisance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing kind/health For CI/testing/release process/refactoring/technical debt items
Projects
Status: No status
Development

No branches or pull requests

4 participants