Reject a None loop in Server's constructor instead of crashing later - #762
Open
afonsojanu wants to merge 1 commit into
Open
Reject a None loop in Server's constructor instead of crashing later#762afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
uvloop.loop.Server is constructed internally with Loop(self) in a couple of places, but nothing stopped it from being called directly with None. That's not a realistic usage pattern, but Cython's typed attribute access on self._loop skips the usual None check once it's stored, so calling close() on a Server built this way segfaults deep inside _unref() instead of raising anything. Marking the loop parameter not None makes the constructor itself reject it right away with a clean TypeError, matching how the type is already declared everywhere it's used internally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #760
uvloop.loop.Serveris only ever constructed internally withServer(self), but the constructor accepted anything sinceLoop loopdoesn't saynot None. As reported,Server(None)builds fine, and callingclose()on it segfaults instead of raising, becauseself._loopis a typed cdef attribute and Cython's attribute access on it skips the usual None check that a plain Python object reference would get. The crash happens deep inside_unref(), well past where the bad input was actually supplied.Traced the actual path with the ASan trace from the issue:
close()calls_unref()in itsfinallyblock, which doesself._loop._servers.discard(self). Withself._loopset toNone, that ends up dereferencing garbage instead of raisingAttributeErrorthe way ordinary Python code would, which lines up with thePyType_IsSubtype/__Pyx_PySet_Discardframes in the report.Marking the parameter
Loop loop not Nonemakes the constructor reject bad input immediately with a cleanTypeError, which is what the reporter expected in the first place. Checked both call sites inloop.pyx(create_serverand the Unix socket path) and they already passself, a realLoop, so this doesn't affect any real usage.Reproduced the crash locally first (built from source with the constructor unchanged, confirmed
Server(None); server.close()segfaults with exit code 139) before writing the fix, then confirmed the same reproducer raises a cleanTypeErrorafterward. Addedtest_server_with_none_loop_raises_instead_of_crashingintests/test_regr1.py, run in a subprocess since without the fix it takes down the whole interpreter rather than failing normally. Reverting just the constructor change makes that test fail with the SIGSEGV assertion, confirming it actually exercises the bug.Ran the full
tests/test_tcp.pysuite (112 tests) afterward with no regressions, andflake8on the touched test file is clean.