From cdc41f1731e7ebe6df96e90b5004bc0093c2cc14 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Fri, 25 Aug 2023 17:04:35 -0700 Subject: [PATCH 1/3] Remove assert that should've been DEOPT_IF The assert(method != NULL) in CALL_NO_KW_LIST_APPEND is wrong -- this condition should lead to a deoptimization, and indeed there is a DEOPT_IF two lines later that will trigger if method == NULL. This would crash in a devious repro scenario (first seen live in boto3 tests) when compiled with assertions enabled. In a production version there is no crash, so impact is limited. (The crash also appears in main; I will prepare a separate PR.) --- Python/bytecodes.c | 2 +- Python/generated_cases.c.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index dc2ae221f0bdb1..d1c79371c4525a 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2992,7 +2992,7 @@ dummy_func( inst(CALL_NO_KW_LIST_APPEND, (unused/1, unused/2, method, self, args[oparg] -- unused)) { assert(kwnames == NULL); assert(oparg == 1); - assert(method != NULL); + PyInterpreterState *interp = _PyInterpreterState_GET(); DEOPT_IF(method != interp->callable_cache.list_append, CALL); DEOPT_IF(!PyList_Check(self), CALL); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index b0a363ce9aa111..e697352b8d9d08 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -4248,7 +4248,7 @@ #line 2993 "Python/bytecodes.c" assert(kwnames == NULL); assert(oparg == 1); - assert(method != NULL); + PyInterpreterState *interp = _PyInterpreterState_GET(); DEOPT_IF(method != interp->callable_cache.list_append, CALL); DEOPT_IF(!PyList_Check(self), CALL); From 298cf2ff2f56f346f8fbaf4da3f09ae0d36ceb1a Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Fri, 25 Aug 2023 21:26:58 -0700 Subject: [PATCH 2/3] Add back a different assert(self != NULL) --- Python/bytecodes.c | 2 +- Python/generated_cases.c.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index d1c79371c4525a..5e80e06205ae4e 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2992,9 +2992,9 @@ dummy_func( inst(CALL_NO_KW_LIST_APPEND, (unused/1, unused/2, method, self, args[oparg] -- unused)) { assert(kwnames == NULL); assert(oparg == 1); - PyInterpreterState *interp = _PyInterpreterState_GET(); DEOPT_IF(method != interp->callable_cache.list_append, CALL); + assert(self != NULL); DEOPT_IF(!PyList_Check(self), CALL); STAT_INC(CALL, hit); if (_PyList_AppendTakeRef((PyListObject *)self, args[0]) < 0) { diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index e697352b8d9d08..a3c049569268c1 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -4248,9 +4248,9 @@ #line 2993 "Python/bytecodes.c" assert(kwnames == NULL); assert(oparg == 1); - PyInterpreterState *interp = _PyInterpreterState_GET(); DEOPT_IF(method != interp->callable_cache.list_append, CALL); + assert(self != NULL); DEOPT_IF(!PyList_Check(self), CALL); STAT_INC(CALL, hit); if (_PyList_AppendTakeRef((PyListObject *)self, args[0]) < 0) { From d39847d8b6585745e6af12b4ace1b292342bfee0 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sat, 26 Aug 2023 04:33:21 +0000 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2023-08-26-04-33-18.gh-issue-108487.aUFxqf.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-08-26-04-33-18.gh-issue-108487.aUFxqf.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-08-26-04-33-18.gh-issue-108487.aUFxqf.rst b/Misc/NEWS.d/next/Core and Builtins/2023-08-26-04-33-18.gh-issue-108487.aUFxqf.rst new file mode 100644 index 00000000000000..1117bcd7e9852b --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-08-26-04-33-18.gh-issue-108487.aUFxqf.rst @@ -0,0 +1 @@ +Change an assert that would cause a spurious crash in a devious case that should only trigger deoptimization.