From bfb576ee23c133bec0ce7c26a8ecea76926b9d8e Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Fri, 1 Dec 2023 12:57:31 +0000 Subject: [PATCH 1/4] gh-111058: Change coro.cr_frame/gen.gi_frame to be None for a closed coroutine/generator. (#112428) --- Lib/test/test_coroutines.py | 8 ++++++++ Lib/test/test_inspect/test_inspect.py | 8 ++++++++ .../2023-11-26-21-30-11.gh-issue-111058.q4DqDY.rst | 3 +++ Objects/genobject.c | 2 +- 4 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-11-26-21-30-11.gh-issue-111058.q4DqDY.rst diff --git a/Lib/test/test_coroutines.py b/Lib/test/test_coroutines.py index 47145782c0f04f..25c981d1511bc1 100644 --- a/Lib/test/test_coroutines.py +++ b/Lib/test/test_coroutines.py @@ -2216,6 +2216,14 @@ async def f(): gen.cr_frame.clear() gen.close() + def test_cr_frame_after_close(self): + async def f(): + pass + gen = f() + self.assertIsNotNone(gen.cr_frame) + gen.close() + self.assertIsNone(gen.cr_frame) + def test_stack_in_coroutine_throw(self): # Regression test for https://github.com/python/cpython/issues/93592 async def a(): diff --git a/Lib/test/test_inspect/test_inspect.py b/Lib/test/test_inspect/test_inspect.py index becbb0498bbb3f..e75682f881ab34 100644 --- a/Lib/test/test_inspect/test_inspect.py +++ b/Lib/test/test_inspect/test_inspect.py @@ -2384,6 +2384,10 @@ def test_closed_after_immediate_exception(self): self.generator.throw(RuntimeError) self.assertEqual(self._generatorstate(), inspect.GEN_CLOSED) + def test_closed_after_close(self): + self.generator.close() + self.assertEqual(self._generatorstate(), inspect.GEN_CLOSED) + def test_running(self): # As mentioned on issue #10220, checking for the RUNNING state only # makes sense inside the generator itself. @@ -2493,6 +2497,10 @@ def test_closed_after_immediate_exception(self): self.coroutine.throw(RuntimeError) self.assertEqual(self._coroutinestate(), inspect.CORO_CLOSED) + def test_closed_after_close(self): + self.coroutine.close() + self.assertEqual(self._coroutinestate(), inspect.CORO_CLOSED) + def test_easy_debugging(self): # repr() and str() of a coroutine state should contain the state name names = 'CORO_CREATED CORO_RUNNING CORO_SUSPENDED CORO_CLOSED'.split() diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-11-26-21-30-11.gh-issue-111058.q4DqDY.rst b/Misc/NEWS.d/next/Core and Builtins/2023-11-26-21-30-11.gh-issue-111058.q4DqDY.rst new file mode 100644 index 00000000000000..de5661f911aa82 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-11-26-21-30-11.gh-issue-111058.q4DqDY.rst @@ -0,0 +1,3 @@ +Change coro.cr_frame/gen.gi_frame to return ``None`` after the coroutine/generator has been closed. +This fixes a bug where :func:`~inspect.getcoroutinestate` and :func:`~inspect.getgeneratorstate` +return the wrong state for a closed coroutine/generator. diff --git a/Objects/genobject.c b/Objects/genobject.c index f98aa357cd2ce1..9614713883741c 100644 --- a/Objects/genobject.c +++ b/Objects/genobject.c @@ -732,7 +732,7 @@ _gen_getframe(PyGenObject *gen, const char *const name) if (PySys_Audit("object.__getattr__", "Os", gen, name) < 0) { return NULL; } - if (gen->gi_frame_state == FRAME_CLEARED) { + if (FRAME_STATE_FINISHED(gen->gi_frame_state)) { Py_RETURN_NONE; } return _Py_XNewRef((PyObject *)_PyFrame_GetFrameObject((_PyInterpreterFrame *)gen->gi_iframe)); From a73aa48e6bec900be7edd3431deaa5fc1d809e6f Mon Sep 17 00:00:00 2001 From: Pablo Galindo Salgado Date: Fri, 1 Dec 2023 13:20:51 +0000 Subject: [PATCH 2/4] gh-112367: Only free perf trampoline arenas at shutdown (#112368) Signed-off-by: Pablo Galindo --- Include/internal/pycore_ceval.h | 1 + ...-11-24-14-10-57.gh-issue-112367.9z1IDp.rst | 2 + Python/perf_trampoline.c | 40 ++++++++++++++++--- Python/pylifecycle.c | 2 +- 4 files changed, 39 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-11-24-14-10-57.gh-issue-112367.9z1IDp.rst diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index c372b7224fb047..3f7ac922bdf451 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -101,6 +101,7 @@ extern int _PyPerfTrampoline_SetCallbacks(_PyPerf_Callbacks *); extern void _PyPerfTrampoline_GetCallbacks(_PyPerf_Callbacks *); extern int _PyPerfTrampoline_Init(int activate); extern int _PyPerfTrampoline_Fini(void); +extern void _PyPerfTrampoline_FreeArenas(void); extern int _PyIsPerfTrampolineActive(void); extern PyStatus _PyPerfTrampoline_AfterFork_Child(void); #ifdef PY_HAVE_PERF_TRAMPOLINE diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-11-24-14-10-57.gh-issue-112367.9z1IDp.rst b/Misc/NEWS.d/next/Core and Builtins/2023-11-24-14-10-57.gh-issue-112367.9z1IDp.rst new file mode 100644 index 00000000000000..991e45ad47fabe --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-11-24-14-10-57.gh-issue-112367.9z1IDp.rst @@ -0,0 +1,2 @@ +Avoid undefined behaviour when using the perf trampolines by not freeing the +code arenas until shutdown. Patch by Pablo Galindo diff --git a/Python/perf_trampoline.c b/Python/perf_trampoline.c index 208ced6c101dce..540b650192ed34 100644 --- a/Python/perf_trampoline.c +++ b/Python/perf_trampoline.c @@ -216,10 +216,24 @@ perf_map_write_entry(void *state, const void *code_addr, PyMem_RawFree(perf_map_entry); } +static void* +perf_map_init_state(void) +{ + PyUnstable_PerfMapState_Init(); + return NULL; +} + +static int +perf_map_free_state(void *state) +{ + PyUnstable_PerfMapState_Fini(); + return 0; +} + _PyPerf_Callbacks _Py_perfmap_callbacks = { - NULL, + &perf_map_init_state, &perf_map_write_entry, - NULL, + &perf_map_free_state, }; static int @@ -415,7 +429,6 @@ _PyPerfTrampoline_SetCallbacks(_PyPerf_Callbacks *callbacks) trampoline_api.write_state = callbacks->write_state; trampoline_api.free_state = callbacks->free_state; trampoline_api.state = NULL; - perf_status = PERF_STATUS_OK; #endif return 0; } @@ -434,6 +447,7 @@ _PyPerfTrampoline_Init(int activate) } if (!activate) { tstate->interp->eval_frame = NULL; + perf_status = PERF_STATUS_NO_INIT; } else { tstate->interp->eval_frame = py_trampoline_evaluator; @@ -444,6 +458,9 @@ _PyPerfTrampoline_Init(int activate) if (extra_code_index == -1) { return -1; } + if (trampoline_api.state == NULL && trampoline_api.init_state != NULL) { + trampoline_api.state = trampoline_api.init_state(); + } perf_status = PERF_STATUS_OK; } #endif @@ -454,16 +471,29 @@ int _PyPerfTrampoline_Fini(void) { #ifdef PY_HAVE_PERF_TRAMPOLINE + if (perf_status != PERF_STATUS_OK) { + return 0; + } PyThreadState *tstate = _PyThreadState_GET(); if (tstate->interp->eval_frame == py_trampoline_evaluator) { tstate->interp->eval_frame = NULL; } - free_code_arenas(); + if (perf_status == PERF_STATUS_OK) { + trampoline_api.free_state(trampoline_api.state); + } extra_code_index = -1; + perf_status = PERF_STATUS_NO_INIT; #endif return 0; } +void _PyPerfTrampoline_FreeArenas(void) { +#ifdef PY_HAVE_PERF_TRAMPOLINE + free_code_arenas(); +#endif + return; +} + int PyUnstable_PerfTrampoline_SetPersistAfterFork(int enable){ #ifdef PY_HAVE_PERF_TRAMPOLINE @@ -477,8 +507,8 @@ PyStatus _PyPerfTrampoline_AfterFork_Child(void) { #ifdef PY_HAVE_PERF_TRAMPOLINE - PyUnstable_PerfMapState_Fini(); if (persist_after_fork) { + _PyPerfTrampoline_Fini(); char filename[256]; pid_t parent_pid = getppid(); snprintf(filename, sizeof(filename), "/tmp/perf-%d.map", parent_pid); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index ac8d5208322882..aff67d7a835e89 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1797,6 +1797,7 @@ finalize_interp_clear(PyThreadState *tstate) _PyArg_Fini(); _Py_ClearFileSystemEncoding(); _PyPerfTrampoline_Fini(); + _PyPerfTrampoline_FreeArenas(); } finalize_interp_types(tstate->interp); @@ -1854,7 +1855,6 @@ Py_FinalizeEx(void) */ _PyAtExit_Call(tstate->interp); - PyUnstable_PerfMapState_Fini(); /* Copy the core config, PyInterpreterState_Delete() free the core config memory */ From 058444308abee79bb1b3358883adfa8c97bd043a Mon Sep 17 00:00:00 2001 From: Zackery Spytz Date: Fri, 1 Dec 2023 05:36:37 -0800 Subject: [PATCH 3/4] gh-82565: Add tests for pickle and unpickle with bad files (GH-16606) --- Lib/test/pickletester.py | 78 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index ddb180ef5ef825..fd446c8145850c 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3514,6 +3514,84 @@ def __init__(self): pass self.assertRaises(pickle.PicklingError, BadPickler().dump, 0) self.assertRaises(pickle.UnpicklingError, BadUnpickler().load) + def test_unpickler_bad_file(self): + # bpo-38384: Crash in _pickle if the read attribute raises an error. + def raises_oserror(self, *args, **kwargs): + raise OSError + @property + def bad_property(self): + 1/0 + + # File without read and readline + class F: + pass + self.assertRaises((AttributeError, TypeError), self.Unpickler, F()) + + # File without read + class F: + readline = raises_oserror + self.assertRaises((AttributeError, TypeError), self.Unpickler, F()) + + # File without readline + class F: + read = raises_oserror + self.assertRaises((AttributeError, TypeError), self.Unpickler, F()) + + # File with bad read + class F: + read = bad_property + readline = raises_oserror + self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + + # File with bad readline + class F: + readline = bad_property + read = raises_oserror + self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + + # File with bad readline, no read + class F: + readline = bad_property + self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + + # File with bad read, no readline + class F: + read = bad_property + self.assertRaises((AttributeError, ZeroDivisionError), self.Unpickler, F()) + + # File with bad peek + class F: + peek = bad_property + read = raises_oserror + readline = raises_oserror + try: + self.Unpickler(F()) + except ZeroDivisionError: + pass + + # File with bad readinto + class F: + readinto = bad_property + read = raises_oserror + readline = raises_oserror + try: + self.Unpickler(F()) + except ZeroDivisionError: + pass + + def test_pickler_bad_file(self): + # File without write + class F: + pass + self.assertRaises(TypeError, self.Pickler, F()) + + # File with bad write + class F: + @property + def write(self): + 1/0 + self.assertRaises(ZeroDivisionError, self.Pickler, F()) + def check_dumps_loads_oob_buffers(self, dumps, loads): # No need to do the full gamut of tests here, just enough to # check that dumps() and loads() redirect their arguments From f8ff80f63536e96b004d29112452a8f1738fde37 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 1 Dec 2023 14:46:50 +0100 Subject: [PATCH 4/4] gh-109413: regrtest: add WorkerRunTests class (#112588) --- Lib/test/libregrtest/main.py | 1 - Lib/test/libregrtest/run_workers.py | 12 +++++------ Lib/test/libregrtest/runtests.py | 31 +++++++++++++++++++---------- Lib/test/libregrtest/worker.py | 6 +++--- 4 files changed, 28 insertions(+), 22 deletions(-) diff --git a/Lib/test/libregrtest/main.py b/Lib/test/libregrtest/main.py index 55fc3a820d3451..16f6974ae32465 100644 --- a/Lib/test/libregrtest/main.py +++ b/Lib/test/libregrtest/main.py @@ -423,7 +423,6 @@ def create_run_tests(self, tests: TestTuple): python_cmd=self.python_cmd, randomize=self.randomize, random_seed=self.random_seed, - json_file=None, ) def _run_tests(self, selected: TestTuple, tests: TestList | None) -> int: diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index 35aaf90ffc4299..18a0342f0611cf 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -18,7 +18,7 @@ from .logger import Logger from .result import TestResult, State from .results import TestResults -from .runtests import RunTests, JsonFile, JsonFileType +from .runtests import RunTests, WorkerRunTests, JsonFile, JsonFileType from .single import PROGRESS_MIN_TIME from .utils import ( StrPath, TestName, @@ -162,7 +162,7 @@ def stop(self) -> None: self._stopped = True self._kill() - def _run_process(self, runtests: RunTests, output_fd: int, + def _run_process(self, runtests: WorkerRunTests, output_fd: int, tmp_dir: StrPath | None = None) -> int | None: popen = create_worker_process(runtests, output_fd, tmp_dir) self._popen = popen @@ -252,9 +252,7 @@ def create_json_file(self, stack: contextlib.ExitStack) -> tuple[JsonFile, TextI json_file = JsonFile(json_fd, JsonFileType.UNIX_FD) return (json_file, json_tmpfile) - def create_worker_runtests(self, test_name: TestName, json_file: JsonFile) -> RunTests: - """Create the worker RunTests.""" - + def create_worker_runtests(self, test_name: TestName, json_file: JsonFile) -> WorkerRunTests: tests = (test_name,) if self.runtests.rerun: match_tests = self.runtests.get_match_tests(test_name) @@ -267,12 +265,12 @@ def create_worker_runtests(self, test_name: TestName, json_file: JsonFile) -> Ru if self.runtests.output_on_failure: kwargs['verbose'] = True kwargs['output_on_failure'] = False - return self.runtests.copy( + return self.runtests.create_worker_runtests( tests=tests, json_file=json_file, **kwargs) - def run_tmp_files(self, worker_runtests: RunTests, + def run_tmp_files(self, worker_runtests: WorkerRunTests, stdout_fd: int) -> tuple[int | None, list[StrPath]]: # gh-93353: Check for leaked temporary files in the parent process, # since the deletion of temporary files can happen late during diff --git a/Lib/test/libregrtest/runtests.py b/Lib/test/libregrtest/runtests.py index b765ba5b41d236..edd72276320e41 100644 --- a/Lib/test/libregrtest/runtests.py +++ b/Lib/test/libregrtest/runtests.py @@ -93,13 +93,17 @@ class RunTests: python_cmd: tuple[str, ...] | None randomize: bool random_seed: int | str - json_file: JsonFile | None - def copy(self, **override): + def copy(self, **override) -> 'RunTests': state = dataclasses.asdict(self) state.update(override) return RunTests(**state) + def create_worker_runtests(self, **override): + state = dataclasses.asdict(self) + state.update(override) + return WorkerRunTests(**state) + def get_match_tests(self, test_name) -> FilterTuple | None: if self.match_tests_dict is not None: return self.match_tests_dict.get(test_name, None) @@ -120,13 +124,6 @@ def iter_tests(self): else: yield from self.tests - def as_json(self) -> StrJSON: - return json.dumps(self, cls=_EncodeRunTests) - - @staticmethod - def from_json(worker_json: StrJSON) -> 'RunTests': - return json.loads(worker_json, object_hook=_decode_runtests) - def json_file_use_stdout(self) -> bool: # Use STDOUT in two cases: # @@ -141,9 +138,21 @@ def json_file_use_stdout(self) -> bool: ) +@dataclasses.dataclass(slots=True, frozen=True) +class WorkerRunTests(RunTests): + json_file: JsonFile + + def as_json(self) -> StrJSON: + return json.dumps(self, cls=_EncodeRunTests) + + @staticmethod + def from_json(worker_json: StrJSON) -> 'WorkerRunTests': + return json.loads(worker_json, object_hook=_decode_runtests) + + class _EncodeRunTests(json.JSONEncoder): def default(self, o: Any) -> dict[str, Any]: - if isinstance(o, RunTests): + if isinstance(o, WorkerRunTests): result = dataclasses.asdict(o) result["__runtests__"] = True return result @@ -158,6 +167,6 @@ def _decode_runtests(data: dict[str, Any]) -> RunTests | dict[str, Any]: data['hunt_refleak'] = HuntRefleak(**data['hunt_refleak']) if data['json_file']: data['json_file'] = JsonFile(**data['json_file']) - return RunTests(**data) + return WorkerRunTests(**data) else: return data diff --git a/Lib/test/libregrtest/worker.py b/Lib/test/libregrtest/worker.py index b3bb0b7f34a060..7a6d33d4499943 100644 --- a/Lib/test/libregrtest/worker.py +++ b/Lib/test/libregrtest/worker.py @@ -7,7 +7,7 @@ from test.support import os_helper, Py_DEBUG from .setup import setup_process, setup_test_dir -from .runtests import RunTests, JsonFile, JsonFileType +from .runtests import WorkerRunTests, JsonFile, JsonFileType from .single import run_single_test from .utils import ( StrPath, StrJSON, TestFilter, @@ -17,7 +17,7 @@ USE_PROCESS_GROUP = (hasattr(os, "setsid") and hasattr(os, "killpg")) -def create_worker_process(runtests: RunTests, output_fd: int, +def create_worker_process(runtests: WorkerRunTests, output_fd: int, tmp_dir: StrPath | None = None) -> subprocess.Popen: python_cmd = runtests.python_cmd worker_json = runtests.as_json() @@ -73,7 +73,7 @@ def create_worker_process(runtests: RunTests, output_fd: int, def worker_process(worker_json: StrJSON) -> NoReturn: - runtests = RunTests.from_json(worker_json) + runtests = WorkerRunTests.from_json(worker_json) test_name = runtests.tests[0] match_tests: TestFilter = runtests.match_tests json_file: JsonFile = runtests.json_file