Skip to content
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

BUG, MAINT: Ufunc reduce reference leak #10194

Merged
merged 2 commits into from Dec 11, 2017

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Dec 10, 2017

A missing DECREF stanza in one of the failure paths, introduced as part of the check_and_adjust_axis PR. In the maintenance commit, I collected all the similar failure paths in a single fail.

I kept the two commits separate, though probably it is easiest to just backport both.

Would lead to a reference leak for the case that an invalid axis
is passed in.
&axes_in,
PyArray_DescrConverter2, &otype,
PyArray_OutputConverter, &out)) {
Py_XDECREF(otype);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

otype cannot be defined if this fails.

Copy link
Member

Choose a reason for hiding this comment

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

That's not true - this was correct before - it could fail on the OutputConverter.

Copy link
Member

Choose a reason for hiding this comment

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

In fact, this should maybe xdecref out too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked for other usages, and they always DECREF only stuff before OutputConverter - so, I guess one does not have to worry about the one that does fail, just what comes before. Anyway, I'll change it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, rather, change it to goto fail as it is for the other options.

Copy link
Member

Choose a reason for hiding this comment

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

I guess one does not have to worry about the one that does fail, just what comes before.

Looking at the implementation of PyArg_ParseTupleAndKeywords in 3.6, I think I can break this by providing an argument by keyword and position, which happens after the other arguments are parsed. I certainly don't think that cpython guarantees the last object will never need freeing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may well be right, but since this is separate from the fix here (in that it is done everywhere OutputConverter is used), I think is should be a separate issue: #10197

Copy link
Member

Choose a reason for hiding this comment

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

I'm wrong, PyArray_OutputConverter borrows a reference. Would be good to distinguish the Converters that lend/create references in the name somehow, but we're way too late to make that decision.

}
/* Special case letting axis={0 or -1} slip through for scalars */
if (ndim == 0 && (axis == 0 || axis == -1)) {
axis = 0;
}
else if (check_and_adjust_axis(&axis, ndim) < 0) {
return NULL;
goto fail;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the DECREF was missing.

Copy link
Member

Choose a reason for hiding this comment

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

Careless of me, good catch

@@ -3941,6 +3919,11 @@ PyUFunc_GenericReduction(PyUFuncObject *ufunc, PyObject *args,
}
}
return PyArray_Return(ret);

fail:
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we normally indent like this.

@mhvk mhvk force-pushed the ufunc-reduce-reference-leak branch 2 times, most recently from 2af1692 to 0a2c9a9 Compare December 10, 2017 20:35
return NULL;
if (error_converting(axis) ||
(check_and_adjust_axis(&axis, ndim) < 0)) {
goto fail;
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be clearer as two separate ifs, like it was before - it makes it look more like the other uses of check_and_adjust_axis.

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

One style nit, but generally looks correct. Thanks for correcting my mistake!

This should help avoid reference leaks in future additions.
@mhvk mhvk force-pushed the ufunc-reduce-reference-leak branch from 0a2c9a9 to 2fadede Compare December 11, 2017 13:39
@mhvk
Copy link
Contributor Author

mhvk commented Dec 11, 2017

OK, nitpick addressed.

@eric-wieser eric-wieser merged commit 06b972f into numpy:master Dec 11, 2017
@eric-wieser
Copy link
Member

Thanks!

@mhvk mhvk deleted the ufunc-reduce-reference-leak branch December 11, 2017 17:29
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Dec 11, 2017
@eric-wieser
Copy link
Member

How'd you find this one?

@mhvk
Copy link
Contributor Author

mhvk commented Dec 11, 2017

I started trying to implement where for ufunc.reduce, as it seemed a good way to get rid of some of the overrides in MaskedArray. But I found that the iterators really are not set up for this; the masks for normal ufuncs are applied when writing output buffers back into the output, which doesn't help if one is doing a reduce (which essentially boils down to np.add(out, in, out=out) with zero strides for the axes reduced over).

@eric-wieser
Copy link
Member

While you're looking at reduce, mind reviewing #8952?

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.

None yet

3 participants