Re: [Libguestfs] [libnbd PATCH v3 2/5] python: Alter lock for persistent buffer

As long as we were wrapping a C buffer and not leaking any Python objects, we had to lock the nbd.Buffer object, to ensure that the PyCapsule was not released prematurely, as there was nothing else to hold on to. But now that the previous commit (4f15d0e9) swapped to wrapping a Python bytearray object, we are better off locking the PyMemoryView created from that object, for two reasons. First, if we are going to allow aio_{pread,pwrite} to take an arbitrary buffer-like object instead of requiring an nbd.Buffer object, then that's all we have available to lock. Second, hanging on to a PyMemoryView provides some nice guarantees that the underlying python buffer-like object won't be reallocated behind our backs. The following snippets from nbdsh use bad style (you should never access a ._* member outside of its class), but demonstrate the same issue we would be faced with once nbd.Buffer starts sharing rather than copying in the public .{to,from}_bytearray: Pre-patch: nbd> b = nbd.Buffer(10) nbd> c = h.aio_pread(b, 0) nbd> b._o.pop() 0 nbd> b.size() 9 nbd> h.poll(-1) Oops - we changed the size of the underlying bytearray prior to receiving the server's reply. If that caused python to reallocate the buffer underlying the bytearray, we've just clobbered random memory. Post-patch: nbd> b = nbd.Buffer(10) nbd> c = h.aio_pread(b, 0) nbd> b._o.pop() Traceback (most recent call last): File "/usr/lib64/python3.10/code.py", line 90, in runcode exec(code, self.locals) File "<console>", line 1, in <module> BufferError: Existing exports of data: object cannot be re-sized

nbd> h.poll(-1) nbd> h.aio_command_completed(c) True nbd> b._o.pop() 0 nbd> b.size() 9 There - now we are sure that as long as our read is pending, the underlying bytearray cannot be reallocated; but as soon as it completes, we have regained full use of the bytearray. --- generator/Python.ml | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 6980034..20d7e78 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -424,16 +424,12 @@ let pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n; pr " if (!%s_view) goto out;\n" n; pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n; - pr " /* Increment refcount since buffer may be saved by libnbd. */\n"; - pr " Py_INCREF (%s);\n" n; - pr " completion_user_data->buf = %s;\n" n + pr " completion_user_data->view = %s_view;\n" n | BytesPersistOut (n, _) -> pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n; pr " if (!%s_view) goto out;\n" n; pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n; - pr " /* Increment refcount since buffer may be saved by libnbd. */\n"; - pr " Py_INCREF (%s);\n" n; - pr " completion_user_data->buf = %s;\n" n + pr " completion_user_data->view = %s_view;\n" n | Closure { cbname } -> pr " %s.user_data = %s_user_data = alloc_user_data ();\n" cbname cbname; pr " if (%s_user_data == NULL) goto out;\n" cbname; @@ -538,8 +534,7 @@ let pr " if (%s.obj)\n" n; pr " PyBuffer_Release (&%s);\n" n | BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n - | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> - pr " Py_XDECREF (%s_view);\n" n + | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> () | Closure { cbname } -> pr " free_user_data (%s_user_data);\n" cbname | Enum _ -> () @@ -588,7 +583,7 @@ let pr " */\n"; pr "struct user_data {\n"; pr " PyObject *fn; /* Optional pointer to Python function. */\n"; - pr " PyObject *buf; /* Optional pointer to persistent buffer. */\n"; + pr " PyObject *view; /* Optional PyMemoryView of persistent buffer. */\n"; pr "};\n"; pr "\n"; pr "static struct user_data *\n"; @@ -609,7 +604,7 @@ let pr "\n"; pr " if (data) {\n"; pr " Py_XDECREF (data->fn);\n"; - pr " Py_XDECREF (data->buf);\n"; + pr " Py_XDECREF (data->view);\n"; pr " free (data);\n"; pr " }\n"; pr "}\n";