Skip to content

BUG: fix out array leak in reduceat and accumulate when dtype resolution fails - #31699

Merged
seberg merged 2 commits into
numpy:mainfrom
ikrommyd:fix-leak-reduceat-accumulate
Jun 22, 2026
Merged

BUG: fix out array leak in reduceat and accumulate when dtype resolution fails#31699
seberg merged 2 commits into
numpy:mainfrom
ikrommyd:fix-leak-reduceat-accumulate

Conversation

@ikrommyd

@ikrommyd ikrommyd commented Jun 20, 2026

Copy link
Copy Markdown
Member

Closes #31691

PR summary

In both of those cases out reference count is incremented before reducelike_promote_and_resolve is called but if the dtype resolution fails, it's not decremented. One can see it with a snippet like the following:

>>> import sys, numpy as np
... arr = np.arange(3)
... out = np.empty(3, dtype="U5")
...
... before = sys.getrefcount(out)
... for _ in range(10000):
...     try:
...         np.add.accumulate(arr, out=out)
...     except Exception:
...         pass
... print(sys.getrefcount(out) - before)
...
10000 # with the fix, we get 0 here

Let me know if you think the above should be added as a test. I found it a trivial fix and not worthy of a specialized test personally.

First time committer introduction

N/A

AI Disclosure

No AI

@ikrommyd
BUG: fix out array leak in reduceat and accumulate when dtype resolut…
3accecf 
…ion fails

Signed-off-by: Iason Krommydas <iason.krom@gmail.com>
MaanasArora
MaanasArora approved these changes Jun 20, 2026
View reviewed changes

@MaanasArora MaanasArora left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM! (All other returns goto fail or finish.)

Pretty sure we don't need a test for this as well!

@ikrommyd

ikrommyd commented Jun 20, 2026

Copy link
Copy Markdown
Member Author

Yeah what one can do is also PyArray_Descr *descrs[3] = {NULL, NULL, NULL}; and goto fail; but that breaks the pattern that everywhere else in this file the descriptor array is left uninitialized before passed to the dtype resolver so I liked the current pattern more.

@MaanasArora

MaanasArora commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

The descriptor array is set up for the ufunc branch, so I agree, this seems more natural!

@seberg

seberg commented Jun 22, 2026

Copy link
Copy Markdown
Member

I agree we shouldn't be too strict about tests here, but it should be pretty easy to write one that hits this, right?
(I wouldn't worry about actually testing for the refcount leak, I guess for strings it may even get tested by the leak sanitizer/etc. Mainly ensure we have coverage.)

But if not agreed, we can also just put it in (and I will in a bit). This bug leak looks very theoretical (i.e. not going to happen in production code).

@ikrommyd

ikrommyd commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

Sorry but I didn't understand what you meant with your comment here. The first sentence feels like is being contradicted by "I wouldn't worry about actually testing for the refcount leak" to me. What would you want to test here? The ref count is the leak no? Are you talking about a test that would just be caught by a sanitizer instead of asserting on the actual ref count number? I feel like the snippet in the PR description would be the test.

@seberg

seberg commented Jun 22, 2026

Copy link
Copy Markdown
Member

I guess I would prefer to be sure to cover the error path at least. Testing for the leak is nice, but I am more willing to ignore it (but if you test with strings failing here, then we should already be testing for leaks implicitly).

(But yes, I am only saying that because I don't want us to slip too casually into the "hard to test, let's skip it" territory.)

@MaanasArora

MaanasArora commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

There are a few different bugs related to this, but this one seems only about the leak right @seberg? There doesn't seem to be any other failure to detect...

@seberg

seberg commented Jun 22, 2026

Copy link
Copy Markdown
Member

Sorry, yeah, I should have looked closer. Any test that covers out= and fails would work (it doesn't have to check refcounts explicitly).
But also yeah, we do have few but real tests without out= passed in.

Anyway, if you add one I am happy, if not I'll put it in tomorrow.

@ikrommyd

ikrommyd commented Jun 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

LMK if what I added is what you had in mind. Indeed I wouldn't assert on the ref count either cause I think I've had cases in the past where a change in python version causes ref count assertions to fail in tests.

Edit: the linting failure is not in code I'm touching here? What?

seberg
seberg approved these changes Jun 22, 2026
View reviewed changes

@seberg seberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sorry for being obnoxious about adding tests :).

The lint failure, I'll ignore it: Not quite sure but the cython lint may be running process wide and maybe they had a release or so? (We'll have to fix it if it shows up generally, though.)

@ikrommyd

ikrommyd commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

#31711 I think Joren's on it.

@seberg
seberg merged commit 8469124 into numpy:main Jun 22, 2026
84 of 87 checks passed
@ikrommyd
ikrommyd deleted the fix-leak-reduceat-accumulate branch June 22, 2026 18:36
@ikrommyd ikrommyd mentioned this pull request Jun 23, 2026
Merged
@rgommers rgommers mentioned this pull request Jun 24, 2026
Closed
@ikrommyd

ikrommyd commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

@seberg since you added the backport candidate label in #31731, I guess it can go here too no?

@seberg seberg added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported labels Jun 24, 2026
@charris charris mentioned this pull request Jul 2, 2026
Merged
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jul 2, 2026
@charris charris mentioned this pull request Jul 2, 2026
Merged
charris pushed a commit that referenced this pull request Jul 2, 2026
@ikrommyd @charris
BUG: fix out array leak in reduceat and accumulate when dtype resolut…
ce3a906 
…ion fails (#31699)

Signed-off-by: Iason Krommydas <iason.krom@gmail.com>
charris added a commit that referenced this pull request Jul 2, 2026
@charris
Merge pull request #31833 from charris/backport-31699
ab538b8 
BUG: fix out array leak in reduceat and accumulate when dtype resolution fails (#31699)
@charris charris added 09 - Backport-Candidate PRs tagged should be backported and removed 09 - Backport-Candidate PRs tagged should be backported labels Jul 10, 2026
@ikrommyd ikrommyd mentioned this pull request Jul 12, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg approved these changes
@MaanasArora MaanasArora MaanasArora approved these changes

Assignees

No one assigned

Labels

00 - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BUG: out reference leak in PyUFunc_Accumulate and PyUFunc_Reduceat on type resolution failure

4 participants

@ikrommyd @MaanasArora @seberg @charris