8:34 a.m.
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";
--
2.36.1