8000 BUG: Incrementing the wrong reference on return by madphysicist · Pull Request #7308 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: Incrementing the wrong reference on return #7308

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
merged 1 commit into from
Feb 23, 2016
Merged

BUG: Incrementing the wrong reference on return #7308

merged 1 commit into from
Feb 23, 2016

Conversation

madphysicist
Copy link
Contributor

How did this work before? If i2 is NULL, Py_INCREF will generally crash. That pretty much means that this branch was never excercised in a test or elsewhere?

@seberg
Copy link
Member
seberg commented Feb 22, 2016

It almost never happens that you have NULL, NULL only happens for newly initialized arrays and then is interpreted as if it was the None object. Not sure if this is even easy to create such a NULLed array from python or any sane method at all to be honest.

@madphysicist
Copy link
Contributor Author

That makes sense, otherwise I imaging this would have been caught a very long time ago :)

@seberg
Copy link
Member
seberg commented Feb 22, 2016

I thought np.empty might do the trick, but no segfault on first attempt.

@seberg
Copy link
Member
seberg commented Feb 22, 2016

You could try np.zeros as well. If neither works, maybe we can just merge without a test

@madphysicist
Copy link
Contributor Author

Sorry, I am just now beginning to figure out where things are in the C code. Could you please point me to the file that you want me to mess with?

@seberg
Copy link
Member
seberg commented Feb 22, 2016

Uh, I was thinking of the simple python calls, but np.empty does not do the trick:

In [10]: arr = np.empty(5, dtype=object)

In [11]: np.ndarray(buffer=arr, shape=3, dtype=np.intp)
Out[11]: array([9405248, 9405248, 9405248])

But you can abuse ndarray:

np.ndarray(buffer=np.zeros(3, dtype=np.intp), shape=3, dtype=np.object_)

note that while np.intp is probably in practice always correct, maybe we should check np.dtype(np.object_).itemsize to get the correctly sized (unsigned) integer.

@seberg
Copy link
Member
seberg commented Feb 22, 2016

And yeah, that np.ndarray array call is totally unsafe if you do not fill it with 0s :))

@seberg
Copy link
Member
seberg commented Feb 22, 2016

Just to be clear, all I would like is a short test for these two NULL cases, since it seems pretty easy to do (and if it ever breaks because we change mp.ndarray, so be it).

@madphysicist
Copy link
Contributor Author

I understand. I just need to dig around a little more. All the tests I tried to construct in Python ended up returning None instead of NULL.

@seberg
Copy link
Member
seberg commented Feb 23, 2016

@madphysicist as I said: np.ndarray(buffer=np.zeros(3, dtype=np.intp), shape=3, dtype=np.object_) (just not np.intp maybe, but the correct int with size np.dtype(np.object_).itemsize. If that does not work, to trigger this bug. I am fine with just giving up.

@njsmith
Copy link
Member
njsmith commented Feb 23, 2016

np.intp is fine, it is extremely unlikely that numpy will ever support an architecture where PyObject* and void* are different sizes :-)

@madphysicist
Copy link
Contributor Author 
>>> np.ndarray(buffer=np.zeros(3, dtype=np.intp), shape=3, dtype=np.object_)
array([None, None, None], dtype=object)

This is already suspicious. I put in a bunch of print statements to make sure I knew which branch is being taken. I tried three cases:

>>> x = np.ndarray(buffer=np.zeros(3, dtype=np.intp), shape=3, dtype=np.object_)
>>> np.logical_or(x, x)
array([None, None, None], dtype=object)
>>> np.logical_or([1, 1, 1], x)
array([1, 1, 1], dtype=object)
>>> np.logical_or(x, [1, 1, 1])
array([1, 1, 1], dtype=object)

The case we want is never triggered. This is pretty much to be expected given the initial printout.

As I mentioned before, I am just starting out with the C API. I will keep this PR in mind as I keep going and will add tests if I see a good way to do so.

@njsmith
Copy link
Member
njsmith commented Feb 23, 2016 
>>> np.ndarray(buffer=np.zeros(3, dtype=np.intp), shape=3, dtype=np.object_)
array([None, None, None], dtype=object)

This is creating an array of nulls -- it's just that we map null pointers to None when going from C to Python. To convince yourself:

In [1]: a = np.zeros(3, dtype=np.intp)

In [2]: a_obj = np.ndarray(buffer=a, shape=(3,), dtype=object)

In [3]: a_obj
Out[3]: array([None, None, None], dtype=object)

In [4]: a_obj[0]

In [5]: a_obj[0] is None
Out[5]: True

In [6]: a
Out[6]: array([0, 0, 0])

In [7]: a[0] = 1

In [8]: a
Out[8]: array([1, 0, 0])

In [9]: a_obj[0]
zsh: segmentation fault  ipython

@seberg
Copy link
Member
seberg commented Feb 23, 2016

I have two theories why it does not work:

  1. The function is not actually used for the ufunc loop.
  2. Buffering possibly replace NULL with None, and this is buffered.
    Your examples are, because the other array will be an int one. But I
    cannot reproduce the error if I make both arrays object. So maybe
    object arrays are always buffered. If you like to dig into weird numpy
    stuff, try putting on the debug tracing stuff in nditer_imp.h (or
    similar nditer* files in core/src/multiarray).

@madphysicist
Copy link
Contributor Author

I am sure it's not #1 since I put a bunch of printf statements before the four different returns. Dirty and unprofessional, I know, but also fast and effective. The last two were always triggered, so the function is called as expected but the input is always sanitized. I will look into the debugging stuff you mentioned.

@madphysicist
Copy link
Contributor Author

I the meantime, since the NULL test needs to be there for robustness, it may as well be correct.

@seberg
Copy link
Member
seberg commented Feb 23, 2016

Yes, fine. If you still find something, we can put it in later, nothing to dabble about now I guess, in the end maybe that whole NULL == None logic is maybe just some weird historic stuff. Thanks.

seberg added a commit that referenced this pull request Feb 23, 2016
@seberg
Merge pull request #7308 from madphysicist/madphysicist-pyref-bug
4b78b90 
BUG: Incrementing the wrong reference on return
@seberg seberg merged commit 4b78b90 into numpy:master Feb 23, 2016
@madphysicist madphysicist deleted the madphysicist-pyref-bug branch February 23, 2016 20:33
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
00 - Bug component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@madphysicist @seberg @njsmith @charris
0