BLD: fix py37 build warnings #26769
BLD: fix py37 build warnings #26769
Conversation
pandas/io/msgpack/_unpacker.pyx
Outdated
| PyObject_GetBuffer(packed, &view, PyBUF_SIMPLE) | ||
| buf = <char*>view.buf | ||
| buf_len = view.len | ||
| PyBuffer_Release(&view) |
There was a problem hiding this comment.
Shouldn't this release call come later in the function, after buf and buf_len are used?
There was a problem hiding this comment.
Yes. There is no guarantee in the buffer protocol that the buffer stays alive beyond a call to PyBuffer_Release().
Also, (Cython does the exception handling here.)PyObject_GetBuffer() can fail, in which case it will return -1.
Simply casting packed to a <char*> might actually also work in Cython, but that won't give you the buffer length. Casting it to a memory view would, but that's also a bit more heavy than calling GetBuffer directly.
There was a problem hiding this comment.
@jbrockmendel can you move the release to the end of the function? (for both the success and unsuccessful paths)
|
FYI here is the bpo patch which might be helpful to compare |
Codecov Report
@@ Coverage Diff @@
## master #26769 +/- ##
==========================================
- Coverage 91.71% 91.7% -0.01%
==========================================
Files 178 178
Lines 50755 50755
==========================================
- Hits 46552 46547 -5
- Misses 4203 4208 +5
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26769 +/- ##
==========================================
- Coverage 92.05% 91.97% -0.09%
==========================================
Files 180 180
Lines 50726 50756 +30
==========================================
- Hits 46696 46681 -15
- Misses 4030 4075 +45
Continue to review full report at Codecov.
|
|
lgtm. is @WillAyd comment relevant? |
|
Will's comment is definitely relevant; the BPO conversation specifically describes this usage as unsafe. Do we have anyone conversant in CPython internals we can hand this off to? Do we have decent test coverage for msgpack? |
|
I think this is fine as is but it's probably better to move the release of the object until after buf and buf_len have actually been used. Seems like as is could be at risk for segfault |
|
@zooba any idea about the usage here ? |
|
can you merge master, ping on green. prob ok to merge this. |
|
Updated, but since segfaults are at risk and this isn't actively broken ATM I'd rather leave this open until we get someone more conversant in the C internals to sign off on it. Any thoughts @scoder? |
|
sure |
|
lgtm. merge on green. |
|
thanks @jbrockmendel |
Not at all sure this is the optimal way to do this, cc @WillAyd for suggestions