[3.14] gh-146092: Fix error handling in _BINARY_OP_ADD_FLOAT opcode

[vstinner]

Fix error handling in _PyFloat_FromDouble_ConsumeInputs() used by _BINARY_OP_ADD_FLOAT, _BINARY_OP_SUBTRACT_FLOAT and _BINARY_OP_MULTIPLY_FLOAT opcodes. PyStackRef_FromPyObjectSteal() must not be called with a NULL pointer.

Fix also _BINARY_OP_INPLACE_ADD_UNICODE opcode.

• Issue: Missing PyErr_NoMemory in _zoneinfoload_data and ts_to_local #146092

[vstinner]

cc @Fidget-Spinner

[vstinner]

@Fidget-Spinner: Please review again, I found another bug in _BINARY_OP_INPLACE_ADD_UNICODE opcode. I updated the PR to fix this opcode as well.

PyUnicode_Append(&temp, right_o);
Py_DECREF(right_o);
ERROR_IF(temp==NULL);
*target_local=PyStackRef_FromPyObjectSteal(temp);

I don't know if *target_local should be set to PyStackRef_NULL on error.

The code in the main branch is different:

DEAD(left);
PyUnicode_Append(&temp, right_o);
_Py_DECREF_SPECIALIZED(right_o, _PyUnicode_ExactDealloc);
*target_local=PyStackRef_NULL;
ERROR_IF(temp==NULL);
res=PyStackRef_FromPyObjectSteal(temp);

[Fidget-Spinner]

I don't know if *target_local should be set to PyStackRef_NULL on error.

I think setting it to PyStackRef_NULL might be wrong? Not too sure. Basically at that point, *target_local refcnt == 1. If we leave it in locals, it must be PyStackref_CLOSE(frame->localsplus[i]) when the frame is cleared during exception raising.

If you want to be sure. Write a test where the append fails (you can trigger an artificial failure somehow), and check if there's reference leaks. If there's refleaks, then we should not be setting it to PyStackRef_NULL and main is wrong.

[vstinner]

If you want to be sure. Write a test where the append fails (you can trigger an artificial failure somehow), and check if there's reference leaks. If there's refleaks, then we should not be setting it to PyStackRef_NULL and main is wrong.

I wrote such test and main code is correct, whereas my 3.14 PR was wrong.

I fixed my 3.14 PR by adding *target_local = PyStackRef_NULL; to the error path.

Here is my patch on top of this PR to test the error path using ./python -m test test_str -m test_leak -R 3:3:

diff --git a/Lib/test/test_str.py b/Lib/test/test_str.py
index 2584fbf72d3..9fd24782a5c 100644
--- a/Lib/test/test_str.py+++ b/Lib/test/test_str.py@@ -2784,6 +2784,20 @@ class Bag:
o.name2 = 4
self.assertEqual(list(o.__dict__), [name, name2])
+ def test_leak(self):+ def f():+ left = b"opazkeopakeoakezaijzoe".decode()+ right = b"xyz".decode()+ try:+ left += right+ except MemoryError:+ left = "MemoryError"+ return left++ f()+ strings = [f() for _ in range(1024)]+ self.assertEqual(strings[-1], "MemoryError")+
if __name__ == "__main__":
unittest.main()
diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index 7a33f63a051..18eda799074 100644
--- a/Python/bytecodes.c+++ b/Python/bytecodes.c@@ -792,7 +792,13 @@ dummy_func(
DEAD(left);
PyObject *temp = PyStackRef_AsPyObjectSteal(*target_local);
PyObject *right_o = PyStackRef_AsPyObjectSteal(right);
- PyUnicode_Append(&temp, right_o);+ if (PyUnicode_EqualToUTF8(temp, "opazkeopakeoakezaijzoe") == 0) {+ PyUnicode_Append(&temp, right_o);+ }+ else {+ Py_CLEAR(temp);+ PyErr_NoMemory();+ }
Py_DECREF(right_o);
if (temp == NULL) {
*target_local = PyStackRef_NULL;

[Fidget-Spinner]

Why not follow main and set PyStackRef_NULL unconditionally?

[vstinner]

The main branch uses op(_BINARY_OP_INPLACE_ADD_UNICODE, (left, right -- res)) signature, but I failed to use the same signature (add res) in Python 3.14.

[Fidget-Spinner]

Oh yeah, I forgot we changed the signature slightly to make the JIT happy. My bad.

[vstinner]

Merged. Thanks for your careful review @Fidget-Spinner!