Page MenuHomePhabricator

Fix illegal early call to PyBuffer_Release in swig typemaps
ClosedPublic

Authored by lawrence_danna on Apr 4 2020, 7:58 PM.

Details

Summary

The buffer protocol does not allow us to just call PyBuffer_Release
and assume the buffer will still be there. Most things that implement the
buffer protocol will let us get away with that, but not all. We need
to release it at the end of the SWIG wrapper.

Diff Detail

Event Timeline

lawrence_danna created this revision.Apr 4 2020, 7:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2020, 7:58 PM

thanks @vadimcn for pointing this out.

Those errors don't look like they could possibly be related to these changes.

labath added a comment.Apr 6 2020, 2:48 AM

Thanks for getting back to this. Is there any reasonable way of testing this (e.g. an existing python class with temporary buffer storage), or would we have to implement our own PyBuffer object?

lldb/bindings/python/python-typemaps.swig
493–495

please delete public, s/class/struct and delete m_ from m_buffer. (| llvm guidelines say we should use struct for classes with public members, and lldb does not (typically) use the m_ prefix in such classes.)

500

Could you also = delete the copy operations to make sure nothing funny happens with those.

Thanks for getting back to this. Is there any reasonable way of testing this (e.g. an existing python class with temporary buffer storage), or would we have to implement our own PyBuffer object?

I don't how of any existing class that does that, I think we'd need to implement our own.

lawrence_danna marked 2 inline comments as done.Apr 6 2020, 12:45 PM

fixed

delete operator= too

LGTM, but let's wait for Pavel to sign off on this.

labath accepted this revision.Apr 7 2020, 3:08 AM

I'm not exactly happy about the lack of a test case, but I guess we can let this slide on the account of it requiring implementing a non-standard PyBuffer in c++ and then loading that from a python test suite and passing it to lldb, which would require a completely new kind of a test case.

lldb/bindings/python/python-typemaps.swig
495

Move this into the initializer list, or maybe even into the default initializer (Py_buffer buffer = {};).

This revision is now accepted and ready to land.Apr 7 2020, 3:08 AM

fix initializer

lawrence_danna marked an inline comment as done.Apr 7 2020, 12:34 PM

fixed

This revision was automatically updated to reflect the committed changes.
aadsm added a subscriber: aadsm.Aug 21 2020, 10:40 AM
aadsm added inline comments.
lldb/bindings/python/python-typemaps.swig
500

The = delete is unsupported in SWIG 2, only in 3: http://www.swig.org/Doc3.0/CPlusPlus11.html#CPlusPlus11_defaulted_deleted
Do we really need it, or is there a workaround it, or should we just bump the minimum requirements to SWIG 3?

lldb/bindings/python/python-typemaps.swig
500

It shouldn't be strictly necessary. I put it in so if for some reason one of these values gets copied, it would result in a compiler error instead of a crash.

lldb/bindings/python/python-typemaps.swig
500

This type could also just be moved into a header so swig doesn't need to parse it.

aadsm added inline comments.Aug 21 2020, 1:46 PM
lldb/bindings/python/python-typemaps.swig
500

ah nice, of course, I can do this. thanks!