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

Fix crash when calling buffer.request multiple times #45

Closed
wants to merge 1 commit into from

Conversation

adler-j
Copy link
Contributor

@adler-j adler-j commented Dec 22, 2015

buffer.request now creates a temporary Py_buffer instead of a pointer.
This reduces the probability of misstakes, and stops a segfault when
buffer.request was called twice.

I think the new design has no major drawbacks over the previous, it is possibly faster since the buffer is now stack-allocated. Also we got rid of the destructor which should not be needed in this case.

Also add build folder to gitignore.

buffer.request now creates a temporary Py_buffer instead of a pointer.
This reduces the probability of misstakes, and stops a segfault when
buffer.request was called twice.

Also add build folder to gitignore.
This was referenced Dec 22, 2015
@wjakob
Copy link
Member

wjakob commented Dec 22, 2015

Hi -- the issue with your change is that the buffer_info will return a pointer that may not be valid anymore after calling PyBuffer_Release(). This is the case when the requested buffer has a format that is different from the internal storage representation.

I think it will be better to use the previous approach and detect if view != nullptr, and not re-call PyObject_GetBuffer() in this case.

@adler-j
Copy link
Contributor Author

adler-j commented Dec 22, 2015

What if the user calls with writable=false then writable=true?

How about letting the buffer_info have ownership of the Py_buffer instead the the buffer? That would clarify buffer ownership issue.

@wjakob
Copy link
Member

wjakob commented Dec 22, 2015

That would be reasonable -- can you make it a private field in this case?

buffer_info needs a new constructor in this case which requests a buffer view and fills all the necessary fields. And the current constructor should just set the buffer view pointer to nullptr.

@adler-j
Copy link
Contributor Author

adler-j commented Dec 28, 2015

I've been putting some thought into this and I wonder, where do we need buffer_info to have the current constructor? Wouldnt a single constructor that stores a Py_buffer by value be the simplest?

The only other place we currently create a buffer_info is inside vectorize_helper.run, but in this case, wouldnt it be more simple if array had a constructor that creates its own data (using PyArray_NewFromDescr) and then calling array.request(true) on it and then writing the values in a loop (as currently done using a std::vector). That would also remove the performance adverse copy that is currently done.

@wjakob
Copy link
Member

wjakob commented Dec 28, 2015

Aren't you forgetting about def_buffer(), which can be used to provide a buffer view for custom types?

@adler-j
Copy link
Contributor Author

adler-j commented Dec 28, 2015

You are indeed correct, will need to think more about this. The ownership issue really should be settled properly however.

@wjakob
Copy link
Member

wjakob commented Jan 17, 2016

This is now fixed, a buffer can only be requested once (second time is a NOP).

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.

2 participants