GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||
GeezerVison is showing github.com — [3.14] gh-146092: Fix error handling in _BINARY_OP_ADD_FLOAT opcode by vstinner · Pull Request #146119 · python/cpython Conversation
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@Fidget-Spinner: Please review again, I found another bug in PyUnicode_Append(&temp, right_o); Py_DECREF(right_o); ERROR_IF(temp == NULL); *target_local = PyStackRef_FromPyObjectSteal(temp); I don't know if 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); |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
I think setting it to PyStackRef_NULL might be wrong? Not too sure. Basically at that point, 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
I wrote such test and main code is correct, whereas my 3.14 PR was wrong. I fixed my 3.14 PR by adding Here is my patch on top of this PR to test the error path using 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; |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| Py_DECREF(right_o); | ||
| ERROR_IF(PyStackRef_IsNull(*target_local)); | ||
| if (temp == NULL) { | ||
| *target_local = PyStackRef_NULL; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why not follow main and set PyStackRef_NULL unconditionally?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I forgot we changed the signature slightly to make the JIT happy. My bad.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Merged. Thanks for your careful review @Fidget-Spinner! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.