Skip to content

[3.11] gh-111942: Remove an extra incref in textiowrapper_change_encoding #126542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

encukou
Copy link
Member

@encukou encukou commented Nov 7, 2024

A recent test fix uncovered a refleak that went in 3.11 on November due to a Git mis-merge.

So, @pablogsal, here's a RM decision for you. (Not time-sensitive, of course.)
This is not a security issue, but it should fix failing buildbots. (The other way to fix them would be to revert the change that uncovered the issue, which would mean we probably won't notice future refleaks in test_io.)


There's an issue with the 3.11 backport commit e2421a3 The change for the main branch was:

+    Py_INCREF(errors);
 ...
     Py_SETREF(self->encoding, encoding);
-    Py_SETREF(self->errors, Py_NewRef(errors));
+    Py_SETREF(self->errors, errors);

but in 3.11 this became:

+    Py_INCREF(errors);
...
     Py_INCREF(errors);
     Py_SETREF(self->encoding, encoding);
     Py_SETREF(self->errors, errors);

i.e. there's an extra incref, but it looks -- at least to Git -- like the change that removes Py_NewRef was already applied.

This was not caught because test_io refleaks tests were ineffective on 3.11 (see #126478 (comment)).

Remove the extraneous incref.

…e_encoding

There's an issue with the 3.11 backport commit e2421a3
The chane for the main branch was:

```diff
+    Py_INCREF(errors);
 ...
     Py_SETREF(self->encoding, encoding);
-    Py_SETREF(self->errors, Py_NewRef(errors));
+    Py_SETREF(self->errors, errors);
```

but in 3.11 this became:

```diff
+    Py_INCREF(errors);
...
     Py_INCREF(errors);
     Py_SETREF(self->encoding, encoding);
     Py_SETREF(self->errors, errors);
```
i.e. there's an extra incref, but it looks -- at least to Git -- like the change
that removes `Py_NewRef` was already applied.

This was not caught because `test_io` refleaks tests were ineffective on 3.11
(see python#126478 (comment)).

Remove the extraneous incref.
@encukou
Copy link
Member Author

encukou commented Nov 7, 2024

(stepping out for a bit, then I'll start the buildbots on this after GHA runs.)

@encukou encukou added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section and removed awaiting core review labels Nov 7, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 8e08bf8 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 7, 2024
@encukou
Copy link
Member Author

encukou commented Nov 7, 2024

!buildbot AMD64 Windows11 Refleaks

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 8e08bf8 🤖

The command will test the builders whose names match following regular expression: AMD64 Windows11 Refleaks

The builders matched are:

  • AMD64 Windows11 Refleaks PR

@encukou
Copy link
Member Author

encukou commented Nov 8, 2024

The failing builds are unstable (Alpine, Usan, Valgrind, FIPS, AIX), not meant for 3.10 (Android, iOS, WASI, NoGIL, bigmem/extralargefile), flaky (s390x RHEL8 Refleaks, PPC64LE Fedora), or show an unrelated problem (test_socket or Fedora).

@pablogsal pablogsal merged commit e33b6fc into python:3.11 Nov 8, 2024
105 of 147 checks passed
@pablogsal
Copy link
Member

Thanks @encukou! I think it makes sense to land it so I went ahead

@encukou encukou deleted the extra-incref-3.11 branch November 11, 2024 14:27
@encukou
Copy link
Member Author

encukou commented Nov 11, 2024

Thank you!

The buildbots have recovered :)

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

Successfully merging this pull request may close these issues.

3 participants