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

Don't steal reference from existing Python object #553

Merged
merged 4 commits into from
Sep 3, 2018

Conversation

tkf
Copy link
Member

@tkf tkf commented Sep 1, 2018

closes #551

I'm not very familiar with Python C API but I think this fixes the bug.

@stevengj
Copy link
Member

stevengj commented Sep 1, 2018

Thanks, good catch!

@stevengj
Copy link
Member

stevengj commented Sep 1, 2018

Can you audit the other usages of pystealref! in case of similar problems?

It seems like it would be cleaner to define:

pyptr(x) = pystealref!(PyObject(x))
pyptr(x::PyObject) = pyincref_(x.o)

and then call pyptr(x) in all the cases where we would have previously called pystealref!(PyObject(x))

@tkf
Copy link
Member Author

tkf commented Sep 1, 2018

I grepped pystealref! and found two more similar bugs which are fixed in the same way.

wrapping/converted from `x` is created.
"""
pyreturn(x::Any) = pystealref!(PyObject(x))
pyreturn(x::PyObject) = pyincref_(x.o)
Copy link
Member Author

Choose a reason for hiding this comment

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

I named it pyreturn since I thought that's more intuitive. Of course, it is super easy to replace it to pyptr.

@stevengj stevengj merged commit 49029e0 into JuliaPy:master Sep 3, 2018
@stevengj
Copy link
Member

stevengj commented Sep 3, 2018

Thanks!

@tkf tkf deleted the nosteal branch November 4, 2018 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passing PyObject through include/eval turns it into PyNULL (which causes segfaults)
2 participants