Fix illegal early call to PyBuffer_Release in swig typemaps

[lawrence_danna]

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.

[lawrence_danna]

thanks @vadimcn for pointing this out.

[lawrence_danna]

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

[labath]

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.

[lawrence_danna]

In D77480#1963315, @labath wrote:

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]

review fixes

[lawrence_danna]

fixed

[lawrence_danna]

delete operator= too

[JDevlieghere]

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

[labath]

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 = {};).

[lawrence_danna]

fix initializer

[lawrence_danna]

fixed

[aadsm]

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?

[lawrence_danna]

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.

[lawrence_danna]

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]

lldb/bindings/python/python-typemaps.swig

500

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

lldb/bindings/python/python-typemaps.swig

%typemap(in) (const char* string, int len) {
     }
     else
     {
         PyErr_SetString(PyExc_TypeError,"not a string-like object");
         return NULL;
     }
 }
 
+%inline %{
+
+struct Py_buffer_RAII {
+  Py_buffer buffer = {};
+  Py_buffer_RAII() {};
+  Py_buffer &operator=(const Py_buffer_RAII &) = delete;
+  Py_buffer_RAII(const Py_buffer_RAII &) = delete;
+  ~Py_buffer_RAII() {
+    if (buffer.obj)
+      PyBuffer_Release(&buffer);
+  }
+};
+
+%}
+
 // These two pybuffer macros are copied out of swig/Lib/python/pybuffer.i,
 // and fixed so they will not crash if PyObject_GetBuffer fails.
 // https://github.com/swig/swig/issues/1640
+//
+// I've also moved the call to PyBuffer_Release to the end of the SWIG wrapper,
+// doing it right away is not legal according to the python buffer protocol.
 
 %define %pybuffer_mutable_binary(TYPEMAP, SIZE)
-%typemap(in) (TYPEMAP, SIZE) {
+%typemap(in) (TYPEMAP, SIZE) (Py_buffer_RAII view) {
   int res; Py_ssize_t size = 0; void *buf = 0;
-  Py_buffer view;
-  res = PyObject_GetBuffer($input, &view, PyBUF_WRITABLE);
+  res = PyObject_GetBuffer($input, &view.buffer, PyBUF_WRITABLE);
   if (res < 0) {
     PyErr_Clear();
     %argument_fail(res, "(TYPEMAP, SIZE)", $symname, $argnum);
   }
-  size = view.len;
-  buf = view.buf;
-  PyBuffer_Release(&view);
+  size = view.buffer.len;
+  buf = view.buffer.buf;
   $1 = ($1_ltype) buf;
   $2 = ($2_ltype) (size/sizeof($*1_type));
 }
 %enddef
 
 %define %pybuffer_binary(TYPEMAP, SIZE)
-%typemap(in) (TYPEMAP, SIZE) {
+%typemap(in) (TYPEMAP, SIZE) (Py_buffer_RAII view) {
   int res; Py_ssize_t size = 0; const void *buf = 0;
-  Py_buffer view;
-  res = PyObject_GetBuffer($input, &view, PyBUF_CONTIG_RO);
+  res = PyObject_GetBuffer($input, &view.buffer, PyBUF_CONTIG_RO);
   if (res < 0) {
     PyErr_Clear();
     %argument_fail(res, "(TYPEMAP, SIZE)", $symname, $argnum);
   }
-  size = view.len;
-  buf = view.buf;
-  PyBuffer_Release(&view);
+  size = view.buffer.len;
+  buf = view.buffer.buf;
   $1 = ($1_ltype) buf;
   $2 = ($2_ltype) (size / sizeof($*1_type));
 }
 %enddef
 
 %pybuffer_binary(const uint8_t *buf, size_t num_bytes);
 %pybuffer_mutable_binary(uint8_t *buf, size_t num_bytes);