GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@vstinner

@vstinner vstinner commented Mar 18, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 _zoneinfo load_data and ts_to_local #146092

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
pythongh-146092: Fix error handling in _BINARY_OP_ADD_FLOAT opcode
3bd1e9d
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.
@bedevere-app bedevere-app Bot mentioned this pull request Mar 18, 2026
Missing PyErr_NoMemory in _zoneinfo load_data and ts_to_local #146092
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Mar 18, 2026
@vstinner vstinner mentioned this pull request Mar 18, 2026
gh-146092: Fix error handling in _BINARY_OP_ADD_UNICODE opcode #146117
Merged
@vstinner

vstinner commented Mar 18, 2026

Copy link
Copy Markdown
Member Author

cc @Fidget-Spinner

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner approved these changes Mar 18, 2026
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Mar 18, 2026
@vstinner
Fix also _BINARY_OP_INPLACE_ADD_UNICODE opcode
659c4ed
@vstinner
vstinner requested a review from markshannon as a code owner March 18, 2026 16:11
@vstinner

vstinner commented Mar 18, 2026

Copy link
Copy Markdown
Member Author

@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);
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner requested a review from Fidget-Spinner March 18, 2026 16:21
@Fidget-Spinner

Fidget-Spinner commented Mar 18, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
Fix reference counting
3517db6
@vstinner

vstinner commented Mar 18, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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;
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
Replace ERROR_IF(1) with ERROR_IF(true)
83d5ef3
Fidget-Spinner
Fidget-Spinner reviewed Mar 19, 2026
View reviewed changes
Comment thread Python/bytecodes.c
Py_DECREF(right_o);
ERROR_IF(PyStackRef_IsNull(*target_local));
if (temp == NULL) {
*target_local = PyStackRef_NULL;

@Fidget-Spinner Fidget-Spinner Mar 19, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner Mar 19, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fidget-Spinner Fidget-Spinner Mar 19, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Fidget-Spinner
Fidget-Spinner approved these changes Mar 19, 2026
View reviewed changes
@vstinner
vstinner merged commit 7f29c1d into python:3.14 Mar 19, 2026
70 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner deleted the eval_err14 branch March 19, 2026 11:14
@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 19, 2026
@vstinner

vstinner commented Mar 19, 2026

Copy link
Copy Markdown
Member Author

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

Fidget-Spinner reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@vstinner @Fidget-Spinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.