Skip to content

Adds missing Py_INCREF in array_boolean_subscript #4494

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.

Sign up for GitHub

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

Jump to bottom
Merged
juliantaylor merged 2 commits into from
Mar 17, 2014
Merged

Adds missing Py_INCREF in array_boolean_subscript #4494

juliantaylor merged 2 commits into from
Mar 17, 2014

Conversation

@embray
Copy link
Contributor

@embray embray commented Mar 14, 2014

This bug was introduced, as far as I can tell, in 52b4a94. It came up in the context of astropy/astropy#1987 where we were getting semi-irregular segfaults. The problem is that after this line a new array is created with PyArray_NewFromDescr using an existing dtype object, but it does not incref that dtype object.

A reliable way that I found to demonstrate this bug was something like:

>>> import numpy as np
>>> c = np.rec.array([(1, 2, 3), (4, 5, 6)])
>>> masked = c[np.array([True, False])]
>>> base = masked.base
>>> del masked, c
>>> base.dtype

at this point various different things could happen, but most often it just results in a segfault.

charris
charris reviewed Mar 14, 2014
View reviewed changes
numpy/core/src/multiarray/mapping.c Outdated
Copy link
Member

@charris charris Mar 14, 2014

Choose a reason for hiding this comment

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

Can omit the blank line, Py_INCREF(dtype); naturally goes with the next line of code.

The function is a bit overspaced as is ;)

Copy link
Contributor Author

@embray embray Mar 14, 2014

Choose a reason for hiding this comment

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

Yes, probably best to group them together.

@charris
Copy link
Member

charris commented Mar 14, 2014

LGTM. Could you squash the two commits into one and rewrite the commit message. Something like

BUG: Fix refcounting bug introduced in 52b4a94.

Extended explanation.

@juliantaylor
Copy link
Contributor

juliantaylor commented Mar 14, 2014

thanks for debugging that for us :)
Adding the triggering case from your comment to a testcase would be great, I guess ./numpy/core/tests/test_regression.py with a reference to the PR is the right spot

@embray
Copy link
Contributor Author

embray commented Mar 14, 2014

@juliantaylor I thought of doing that, but given that the failure result is unpredictable and possibly a segfault was troubling. Should I just go with it anyways?

@juliantaylor
Copy link
Contributor

juliantaylor commented Mar 14, 2014

yes, a test that finds an issue sometimes is better than none, that its segfaulting is no problem

@charris
Copy link
Member

charris commented Mar 17, 2014

@embray Could you finish this up? I would like to backport it for the 1.8.1 release.

@juliantaylor
Copy link
Contributor

juliantaylor commented Mar 17, 2014

this doesn't affect 1.8.x, it got introduced in the indexing rewrite
I guess we can still merge now, I can add the tests in a separate PR

juliantaylor added a commit that referenced this pull request Mar 17, 2014
@juliantaylor
Merge pull request #4494 from embray/astropy-1987
e3794e7 
BUG: Adds missing Py_INCREF in array_boolean_subscript
@juliantaylor juliantaylor merged commit e3794e7 into numpy:master Mar 17, 2014
juliantaylor added a commit to juliantaylor/numpy that referenced this pull request Mar 17, 2014
@juliantaylor
TST: add some more tests
5df0158 
test for numpygh-4494
test median returns array scalars and works with object arrays
@juliantaylor juliantaylor mentioned this pull request Mar 17, 2014
Merged
@embray
Copy link
Contributor Author

embray commented Mar 18, 2014

Sorry I didn't get a chance to finish this up--got sidetracked by other tasks for the last few days. Thanks!

@embray embray deleted the astropy-1987 branch March 20, 2014 19:40
@astrofrog astrofrog mentioned this pull request Jun 18, 2014
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@embray @charris @juliantaylor