Skip to content

Adds missing Py_INCREF in array_boolean_subscript - #4494

Merged
juliantaylor merged 2 commits into
numpy:masterfrom
embray:astropy-1987
Mar 17, 2014
Merged

Adds missing Py_INCREF in array_boolean_subscript#4494
juliantaylor merged 2 commits into
numpy:masterfrom
embray:astropy-1987

Conversation

@embray

@embray embray commented Mar 14, 2014

Copy link
Copy Markdown
Contributor

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
Comment thread numpy/core/src/multiarray/mapping.c Outdated

@charris charris Mar 14, 2014

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.

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

The function is a bit overspaced as is ;)

@embray embray Mar 14, 2014

Copy link
Copy Markdown
Contributor Author

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

charris commented Mar 14, 2014

Copy link
Copy Markdown
Member

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

juliantaylor commented Mar 14, 2014

Copy link
Copy Markdown
Contributor

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

embray commented Mar 14, 2014

Copy link
Copy Markdown
Contributor Author

@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

juliantaylor commented Mar 14, 2014

Copy link
Copy Markdown
Contributor

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

@charris

charris commented Mar 17, 2014

Copy link
Copy Markdown
Member

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

@juliantaylor

juliantaylor commented Mar 17, 2014

Copy link
Copy Markdown
Contributor

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

embray commented Mar 18, 2014

Copy link
Copy Markdown
Contributor Author

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