Skip to content

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Jun 14, 2018

PyObject_Type increases the reference count, which was not taken into account.

Split off from #11320, since that one was getting too complex (don't understand the failure in USE_DEBUG...), and this part should probably be back-ported.

PyObject_Type increases the reference count, which was not taken
into account.
@mhvk
Copy link
Contributor Author

mhvk commented Jun 14, 2018

@eric-wieser - if you have a chance, could you have a quick look at this one? I'm a bit worried about stacking too many PRs on top of each other...

for (i = 0; i < num_override_args; i++) {
obj = with_override[i];
if (obj == NULL) {
override_obj = with_override[i];
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the declaration of override_obj within this loop too?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, that's not possible

PyObject *other_obj = with_override[j];
if (other_obj != NULL &&
PyObject_Type(other_obj) != PyObject_Type(obj) &&
Py_TYPE(other_obj) != Py_TYPE(override_obj) &&
Copy link
Member

Choose a reason for hiding this comment

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

A nit: There's no point doing this extra check - it's just the first line of PyObject_IsInstance anyway.

Not going to make you change that in this PR though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surely, the first line is for object == other in isinstance, while here we do not want to defer to other if the types are the same. But I think you are right that the check can be removed, since we now explicitly remove instances of the same type...

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, what I said above was wrong - it's not an optimization, it's a check for a strict subclass

@eric-wieser eric-wieser merged commit a362f45 into numpy:master Jun 14, 2018
@mhvk mhvk deleted the ufunc-override-ref-counting-bugs branch June 15, 2018 01:14
@mhvk mhvk restored the ufunc-override-ref-counting-bugs branch June 15, 2018 01:15
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jun 20, 2018
@charris charris removed this from the 1.15.0 release milestone Jun 20, 2018
@charris charris changed the title BUG: decref in failure path; replace PyObject_Type by Py_TYPE BUG: decref in failure path; replace PyObject_Type by Py_TYPE Nov 10, 2018
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