From 65755d048f84a0067b670321df731bf76056f30a Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Tue, 24 Sep 2024 11:15:16 -0700 Subject: [PATCH 1/2] Change tests in PyStackRef operations for NULL-ness to asserts --- Include/internal/pycore_stackref.h | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Include/internal/pycore_stackref.h b/Include/internal/pycore_stackref.h index b5b6993812057d..cf6dd22cfb18d1 100644 --- a/Include/internal/pycore_stackref.h +++ b/Include/internal/pycore_stackref.h @@ -111,7 +111,8 @@ PyStackRef_AsPyObjectBorrow(_PyStackRef stackref) static inline PyObject * PyStackRef_AsPyObjectSteal(_PyStackRef stackref) { - if (!PyStackRef_IsNull(stackref) && PyStackRef_IsDeferred(stackref)) { + assert(!PyStackRef_IsNull(stackref)); + if (PyStackRef_IsDeferred(stackref)) { return Py_NewRef(PyStackRef_AsPyObjectBorrow(stackref)); } return PyStackRef_AsPyObjectBorrow(stackref); @@ -131,9 +132,10 @@ PyStackRef_AsPyObjectSteal(_PyStackRef stackref) static inline _PyStackRef _PyStackRef_FromPyObjectSteal(PyObject *obj) { + assert(obj != NULL); // Make sure we don't take an already tagged value. assert(((uintptr_t)obj & Py_TAG_BITS) == 0); - unsigned int tag = (obj == NULL || _Py_IsImmortal(obj)) ? (Py_TAG_DEFERRED) : Py_TAG_PTR; + unsigned int tag = _Py_IsImmortal(obj) ? (Py_TAG_DEFERRED) : Py_TAG_PTR; return ((_PyStackRef){.bits = ((uintptr_t)(obj)) | tag}); } # define PyStackRef_FromPyObjectSteal(obj) _PyStackRef_FromPyObjectSteal(_PyObject_CAST(obj)) @@ -193,6 +195,7 @@ PyStackRef_FromPyObjectImmortal(PyObject *obj) # define PyStackRef_CLOSE(REF) \ do { \ _PyStackRef _close_tmp = (REF); \ + assert(!PyStackRef_IsNull(_close_tmp)); \ if (!PyStackRef_IsDeferred(_close_tmp)) { \ Py_DECREF(PyStackRef_AsPyObjectBorrow(_close_tmp)); \ } \ @@ -214,10 +217,11 @@ PyStackRef_FromPyObjectImmortal(PyObject *obj) static inline _PyStackRef PyStackRef_DUP(_PyStackRef stackref) { + assert(!PyStackRef_IsNull(stackref)); if (PyStackRef_IsDeferred(stackref)) { - assert(PyStackRef_IsNull(stackref) || - _Py_IsImmortal(PyStackRef_AsPyObjectBorrow(stackref)) || - _PyObject_HasDeferredRefcount(PyStackRef_AsPyObjectBorrow(stackref))); + assert(_Py_IsImmortal(PyStackRef_AsPyObjectBorrow(stackref)) || + _PyObject_HasDeferredRefcount(PyStackRef_AsPyObjectBorrow(stackref)) + ); return stackref; } Py_INCREF(PyStackRef_AsPyObjectBorrow(stackref)); From 183dc9e75d984b33e5bdc27ea0aac623be189ef4 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Wed, 2 Oct 2024 04:16:58 -0700 Subject: [PATCH 2/2] Fix bytecodes.c to handle NULLs before passing to stack refs --- Python/bytecodes.c | 44 ++++++++++++++++++------------ Python/executor_cases.c.h | 27 ++++++++++-------- Python/generated_cases.c.h | 56 ++++++++++++++++++++++---------------- 3 files changed, 75 insertions(+), 52 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 846404e28bb18f..5928cab8dcbab0 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1894,9 +1894,10 @@ dummy_func( DECREF_INPUTS(); ERROR_IF(super == NULL, error); PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 2); - attr = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(super, name)); + PyObject *attr_o = PyObject_GetAttr(super, name); Py_DECREF(super); - ERROR_IF(PyStackRef_IsNull(attr), error); + ERROR_IF(attr_o == NULL, error); + attr = PyStackRef_FromPyObjectSteal(attr_o); null = PyStackRef_NULL; } @@ -2692,9 +2693,10 @@ dummy_func( inst(GET_ITER, (iterable -- iter)) { /* before: [obj]; after [getiter(obj)] */ - iter = PyStackRef_FromPyObjectSteal(PyObject_GetIter(PyStackRef_AsPyObjectBorrow(iterable))); + PyObject *iter_o = PyObject_GetIter(PyStackRef_AsPyObjectBorrow(iterable)); DECREF_INPUTS(); - ERROR_IF(PyStackRef_IsNull(iter), error); + ERROR_IF(iter_o == NULL, error); + iter = PyStackRef_FromPyObjectSteal(iter_o); } inst(GET_YIELD_FROM_ITER, (iterable -- iter)) { @@ -3002,16 +3004,18 @@ dummy_func( PyObject *owner_o = PyStackRef_AsPyObjectSteal(owner); PyObject *name = _Py_SpecialMethods[oparg].name; PyObject *self_or_null_o; - attr = PyStackRef_FromPyObjectSteal(_PyObject_LookupSpecialMethod(owner_o, name, &self_or_null_o)); - if (PyStackRef_IsNull(attr)) { + PyObject *attr_o = _PyObject_LookupSpecialMethod(owner_o, name, &self_or_null_o); + if (attr_o == NULL) { if (!_PyErr_Occurred(tstate)) { _PyErr_Format(tstate, PyExc_TypeError, _Py_SpecialMethods[oparg].error, Py_TYPE(owner_o)->tp_name); } + ERROR_IF(true, error); } - ERROR_IF(PyStackRef_IsNull(attr), error); - self_or_null = PyStackRef_FromPyObjectSteal(self_or_null_o); + attr = PyStackRef_FromPyObjectSteal(attr_o); + self_or_null = self_or_null_o == NULL ? + PyStackRef_NULL : PyStackRef_FromPyObjectSteal(self_or_null_o); } inst(WITH_EXCEPT_START, (exit_func, exit_self, lasti, unused, val -- exit_func, exit_self, lasti, unused, val, res)) { @@ -3042,9 +3046,10 @@ dummy_func( (void)lasti; // Shut up compiler warning if asserts are off PyObject *stack[5] = {NULL, PyStackRef_AsPyObjectBorrow(exit_self), exc, val_o, tb}; int has_self = !PyStackRef_IsNull(exit_self); - res = PyStackRef_FromPyObjectSteal(PyObject_Vectorcall(exit_func_o, stack + 2 - has_self, - (3 + has_self) | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL)); - ERROR_IF(PyStackRef_IsNull(res), error); + PyObject *res_o = PyObject_Vectorcall(exit_func_o, stack + 2 - has_self, + (3 + has_self) | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL); + ERROR_IF(res_o == NULL, error); + res = PyStackRef_FromPyObjectSteal(res_o); } pseudo(SETUP_FINALLY, (-- unused), (HAS_ARG)) = { @@ -4298,6 +4303,7 @@ dummy_func( } assert(PyTuple_CheckExact(callargs)); EVAL_CALL_STAT_INC_IF_FUNCTION(EVAL_CALL_FUNCTION_EX, func); + PyObject *result_o; if (opcode == INSTRUMENTED_CALL_FUNCTION_EX) { PyObject *arg = PyTuple_GET_SIZE(callargs) > 0 ? PyTuple_GET_ITEM(callargs, 0) : &_PyInstrumentation_MISSING; @@ -4305,10 +4311,10 @@ dummy_func( tstate, PY_MONITORING_EVENT_CALL, frame, this_instr, func, arg); if (err) ERROR_NO_POP(); - result = PyStackRef_FromPyObjectSteal(PyObject_Call(func, callargs, kwargs)); + result_o = PyObject_Call(func, callargs, kwargs); if (!PyFunction_Check(func) && !PyMethod_Check(func)) { - if (PyStackRef_IsNull(result)) { + if (result_o == NULL) { _Py_call_instrumentation_exc2( tstate, PY_MONITORING_EVENT_C_RAISE, frame, this_instr, func, arg); @@ -4318,7 +4324,7 @@ dummy_func( tstate, PY_MONITORING_EVENT_C_RETURN, frame, this_instr, func, arg); if (err < 0) { - PyStackRef_CLEAR(result); + Py_CLEAR(result_o); } } } @@ -4344,11 +4350,12 @@ dummy_func( frame->return_offset = 1; DISPATCH_INLINED(new_frame); } - result = PyStackRef_FromPyObjectSteal(PyObject_Call(func, callargs, kwargs)); + result_o = PyObject_Call(func, callargs, kwargs); } DECREF_INPUTS(); assert(PyStackRef_AsPyObjectBorrow(PEEK(2 + (oparg & 1))) == NULL); - ERROR_IF(PyStackRef_IsNull(result), error); + ERROR_IF(result_o == NULL, error); + result = PyStackRef_FromPyObjectSteal(result_o); } macro(CALL_FUNCTION_EX) = @@ -4458,9 +4465,10 @@ dummy_func( /* If value is a unicode object, then we know the result * of format(value) is value itself. */ if (!PyUnicode_CheckExact(value_o)) { - res = PyStackRef_FromPyObjectSteal(PyObject_Format(value_o, NULL)); + PyObject *res_o = PyObject_Format(value_o, NULL); PyStackRef_CLOSE(value); - ERROR_IF(PyStackRef_IsNull(res), error); + ERROR_IF(res_o == NULL, error); + res = PyStackRef_FromPyObjectSteal(res_o); } else { res = value; diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 93ab068f9de949..0796ba9a21c9b0 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -3085,9 +3085,10 @@ _PyStackRef iter; iterable = stack_pointer[-1]; /* before: [obj]; after [getiter(obj)] */ - iter = PyStackRef_FromPyObjectSteal(PyObject_GetIter(PyStackRef_AsPyObjectBorrow(iterable))); + PyObject *iter_o = PyObject_GetIter(PyStackRef_AsPyObjectBorrow(iterable)); PyStackRef_CLOSE(iterable); - if (PyStackRef_IsNull(iter)) JUMP_TO_ERROR(); + if (iter_o == NULL) JUMP_TO_ERROR(); + iter = PyStackRef_FromPyObjectSteal(iter_o); stack_pointer[-1] = iter; break; } @@ -3340,16 +3341,18 @@ PyObject *owner_o = PyStackRef_AsPyObjectSteal(owner); PyObject *name = _Py_SpecialMethods[oparg].name; PyObject *self_or_null_o; - attr = PyStackRef_FromPyObjectSteal(_PyObject_LookupSpecialMethod(owner_o, name, &self_or_null_o)); - if (PyStackRef_IsNull(attr)) { + PyObject *attr_o = _PyObject_LookupSpecialMethod(owner_o, name, &self_or_null_o); + if (attr_o == NULL) { if (!_PyErr_Occurred(tstate)) { _PyErr_Format(tstate, PyExc_TypeError, _Py_SpecialMethods[oparg].error, Py_TYPE(owner_o)->tp_name); } + if (true) JUMP_TO_ERROR(); } - if (PyStackRef_IsNull(attr)) JUMP_TO_ERROR(); - self_or_null = PyStackRef_FromPyObjectSteal(self_or_null_o); + attr = PyStackRef_FromPyObjectSteal(attr_o); + self_or_null = self_or_null_o == NULL ? + PyStackRef_NULL : PyStackRef_FromPyObjectSteal(self_or_null_o); stack_pointer[-1] = attr; stack_pointer[0] = self_or_null; stack_pointer += 1; @@ -3392,9 +3395,10 @@ (void)lasti; // Shut up compiler warning if asserts are off PyObject *stack[5] = {NULL, PyStackRef_AsPyObjectBorrow(exit_self), exc, val_o, tb}; int has_self = !PyStackRef_IsNull(exit_self); - res = PyStackRef_FromPyObjectSteal(PyObject_Vectorcall(exit_func_o, stack + 2 - has_self, - (3 + has_self) | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL)); - if (PyStackRef_IsNull(res)) JUMP_TO_ERROR(); + PyObject *res_o = PyObject_Vectorcall(exit_func_o, stack + 2 - has_self, + (3 + has_self) | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL); + if (res_o == NULL) JUMP_TO_ERROR(); + res = PyStackRef_FromPyObjectSteal(res_o); stack_pointer[0] = res; stack_pointer += 1; assert(WITHIN_STACK_BOUNDS()); @@ -5077,9 +5081,10 @@ /* If value is a unicode object, then we know the result * of format(value) is value itself. */ if (!PyUnicode_CheckExact(value_o)) { - res = PyStackRef_FromPyObjectSteal(PyObject_Format(value_o, NULL)); + PyObject *res_o = PyObject_Format(value_o, NULL); PyStackRef_CLOSE(value); - if (PyStackRef_IsNull(res)) JUMP_TO_ERROR(); + if (res_o == NULL) JUMP_TO_ERROR(); + res = PyStackRef_FromPyObjectSteal(res_o); } else { res = value; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 6d902e2c1d9ba8..a67d06c692b065 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -1584,6 +1584,7 @@ } assert(PyTuple_CheckExact(callargs)); EVAL_CALL_STAT_INC_IF_FUNCTION(EVAL_CALL_FUNCTION_EX, func); + PyObject *result_o; if (opcode == INSTRUMENTED_CALL_FUNCTION_EX) { PyObject *arg = PyTuple_GET_SIZE(callargs) > 0 ? PyTuple_GET_ITEM(callargs, 0) : &_PyInstrumentation_MISSING; @@ -1591,9 +1592,9 @@ tstate, PY_MONITORING_EVENT_CALL, frame, this_instr, func, arg); if (err) goto error; - result = PyStackRef_FromPyObjectSteal(PyObject_Call(func, callargs, kwargs)); + result_o = PyObject_Call(func, callargs, kwargs); if (!PyFunction_Check(func) && !PyMethod_Check(func)) { - if (PyStackRef_IsNull(result)) { + if (result_o == NULL) { _Py_call_instrumentation_exc2( tstate, PY_MONITORING_EVENT_C_RAISE, frame, this_instr, func, arg); @@ -1603,7 +1604,7 @@ tstate, PY_MONITORING_EVENT_C_RETURN, frame, this_instr, func, arg); if (err < 0) { - PyStackRef_CLEAR(result); + Py_CLEAR(result_o); } } } @@ -1628,17 +1629,18 @@ frame->return_offset = 1; DISPATCH_INLINED(new_frame); } - result = PyStackRef_FromPyObjectSteal(PyObject_Call(func, callargs, kwargs)); + result_o = PyObject_Call(func, callargs, kwargs); } PyStackRef_CLOSE(func_st); PyStackRef_CLOSE(callargs_st); PyStackRef_XCLOSE(kwargs_st); assert(PyStackRef_AsPyObjectBorrow(PEEK(2 + (oparg & 1))) == NULL); - if (PyStackRef_IsNull(result)) { + if (result_o == NULL) { stack_pointer += -3 - (oparg & 1); assert(WITHIN_STACK_BOUNDS()); goto error; } + result = PyStackRef_FromPyObjectSteal(result_o); } // _CHECK_PERIODIC { @@ -3571,9 +3573,10 @@ /* If value is a unicode object, then we know the result * of format(value) is value itself. */ if (!PyUnicode_CheckExact(value_o)) { - res = PyStackRef_FromPyObjectSteal(PyObject_Format(value_o, NULL)); + PyObject *res_o = PyObject_Format(value_o, NULL); PyStackRef_CLOSE(value); - if (PyStackRef_IsNull(res)) goto pop_1_error; + if (res_o == NULL) goto pop_1_error; + res = PyStackRef_FromPyObjectSteal(res_o); } else { res = value; @@ -3930,9 +3933,10 @@ _PyStackRef iter; iterable = stack_pointer[-1]; /* before: [obj]; after [getiter(obj)] */ - iter = PyStackRef_FromPyObjectSteal(PyObject_GetIter(PyStackRef_AsPyObjectBorrow(iterable))); + PyObject *iter_o = PyObject_GetIter(PyStackRef_AsPyObjectBorrow(iterable)); PyStackRef_CLOSE(iterable); - if (PyStackRef_IsNull(iter)) goto pop_1_error; + if (iter_o == NULL) goto pop_1_error; + iter = PyStackRef_FromPyObjectSteal(iter_o); stack_pointer[-1] = iter; DISPATCH(); } @@ -5832,16 +5836,18 @@ PyObject *owner_o = PyStackRef_AsPyObjectSteal(owner); PyObject *name = _Py_SpecialMethods[oparg].name; PyObject *self_or_null_o; - attr = PyStackRef_FromPyObjectSteal(_PyObject_LookupSpecialMethod(owner_o, name, &self_or_null_o)); - if (PyStackRef_IsNull(attr)) { + PyObject *attr_o = _PyObject_LookupSpecialMethod(owner_o, name, &self_or_null_o); + if (attr_o == NULL) { if (!_PyErr_Occurred(tstate)) { _PyErr_Format(tstate, PyExc_TypeError, _Py_SpecialMethods[oparg].error, Py_TYPE(owner_o)->tp_name); } + if (true) goto pop_1_error; } - if (PyStackRef_IsNull(attr)) goto pop_1_error; - self_or_null = PyStackRef_FromPyObjectSteal(self_or_null_o); + attr = PyStackRef_FromPyObjectSteal(attr_o); + self_or_null = self_or_null_o == NULL ? + PyStackRef_NULL : PyStackRef_FromPyObjectSteal(self_or_null_o); stack_pointer[-1] = attr; stack_pointer[0] = self_or_null; stack_pointer += 1; @@ -5916,9 +5922,10 @@ PyStackRef_CLOSE(self_st); if (super == NULL) goto pop_3_error; PyObject *name = GETITEM(FRAME_CO_NAMES, oparg >> 2); - attr = PyStackRef_FromPyObjectSteal(PyObject_GetAttr(super, name)); + PyObject *attr_o = PyObject_GetAttr(super, name); Py_DECREF(super); - if (PyStackRef_IsNull(attr)) goto pop_3_error; + if (attr_o == NULL) goto pop_3_error; + attr = PyStackRef_FromPyObjectSteal(attr_o); null = PyStackRef_NULL; } stack_pointer[-3] = attr; @@ -7597,9 +7604,10 @@ (void)lasti; // Shut up compiler warning if asserts are off PyObject *stack[5] = {NULL, PyStackRef_AsPyObjectBorrow(exit_self), exc, val_o, tb}; int has_self = !PyStackRef_IsNull(exit_self); - res = PyStackRef_FromPyObjectSteal(PyObject_Vectorcall(exit_func_o, stack + 2 - has_self, - (3 + has_self) | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL)); - if (PyStackRef_IsNull(res)) goto error; + PyObject *res_o = PyObject_Vectorcall(exit_func_o, stack + 2 - has_self, + (3 + has_self) | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL); + if (res_o == NULL) goto error; + res = PyStackRef_FromPyObjectSteal(res_o); stack_pointer[0] = res; stack_pointer += 1; assert(WITHIN_STACK_BOUNDS()); @@ -7686,6 +7694,7 @@ } assert(PyTuple_CheckExact(callargs)); EVAL_CALL_STAT_INC_IF_FUNCTION(EVAL_CALL_FUNCTION_EX, func); + PyObject *result_o; if (opcode == INSTRUMENTED_CALL_FUNCTION_EX) { PyObject *arg = PyTuple_GET_SIZE(callargs) > 0 ? PyTuple_GET_ITEM(callargs, 0) : &_PyInstrumentation_MISSING; @@ -7693,9 +7702,9 @@ tstate, PY_MONITORING_EVENT_CALL, frame, this_instr, func, arg); if (err) goto error; - result = PyStackRef_FromPyObjectSteal(PyObject_Call(func, callargs, kwargs)); + result_o = PyObject_Call(func, callargs, kwargs); if (!PyFunction_Check(func) && !PyMethod_Check(func)) { - if (PyStackRef_IsNull(result)) { + if (result_o == NULL) { _Py_call_instrumentation_exc2( tstate, PY_MONITORING_EVENT_C_RAISE, frame, this_instr, func, arg); @@ -7705,7 +7714,7 @@ tstate, PY_MONITORING_EVENT_C_RETURN, frame, this_instr, func, arg); if (err < 0) { - PyStackRef_CLEAR(result); + Py_CLEAR(result_o); } } } @@ -7730,17 +7739,18 @@ frame->return_offset = 1; DISPATCH_INLINED(new_frame); } - result = PyStackRef_FromPyObjectSteal(PyObject_Call(func, callargs, kwargs)); + result_o = PyObject_Call(func, callargs, kwargs); } PyStackRef_CLOSE(func_st); PyStackRef_CLOSE(callargs_st); PyStackRef_XCLOSE(kwargs_st); assert(PyStackRef_AsPyObjectBorrow(PEEK(2 + (oparg & 1))) == NULL); - if (PyStackRef_IsNull(result)) { + if (result_o == NULL) { stack_pointer += -3 - (oparg & 1); assert(WITHIN_STACK_BOUNDS()); goto error; } + result = PyStackRef_FromPyObjectSteal(result_o); stack_pointer[-3 - (oparg & 1)] = result; stack_pointer += -2 - (oparg & 1); assert(WITHIN_STACK_BOUNDS());