10:49 a.m.
This patch fixes the corner-case regression introduced by the previous
patch where the pread_structured callback buffer lifetime ends as soon
as the callback (that is, where accessing a stashed callback parameter
can result in ValueError instead of modifying a harmless copy). With
careful effort, we can create a memoryview of the Python object that
we read into, then slice that memoryview as part of the callback; now
the slice will be valid as long as the original object is also valid.
Note that this does not tackle the case of aio_pread_structured; for
that, we either need to make nbd.Buffer honor the Python buffer
protocol, or else change our handling of BytesPersistOut to utilize
the buffer protocol directly instead of requiring the user to
round-trip through nbd.Buffer.
The runtime of this patch is still about the same 3.0s as in the test
of the previous patch, but now you can do:
$ nbdsh
nbd>
h.connect_command(["nbdkit","-s","memory","10"])
nbd> copy=None
nbd> def f(b,o,s,e):
... global copy
... copy = b
... print(b[0])
...
nbd> print(copy)
None
nbd> buf = h.pread_structured(10,0,f)
0
nbd> copy
<memory at 0x7fecef4e07c0>
nbd> copy[0]=0x31
nbd> print(buf)
bytearray(b'1\x00\x00\x00\x00\x00\x00\x00\x00\x00')
That is, because we passed in a memoryview slice of the original
buffer instead of a temporary slice of C memory, we can now stash that
slice into a global, modify it, and see those modifications reflected
back to the overall bytearray object returned by pread_structured.
The generated diff is a bit more verbose, including some no-op lines
added into pread, aio_pread, and aio_pread_structured. In practice,
only pread_structured is actually impacted.
| --- python/methods.c.bak 2022-05-31 09:51:30.406757934 -0500
| +++ python/methods.c 2022-05-31 10:26:01.797636894 -0500
| @@ -27,6 +27,7 @@
| #include <stdlib.h>
| #include <stdint.h>
| #include <stdbool.h>
| +#include <stddef.h>
|
| #include <libnbd.h>
|
| @@ -76,8 +77,24 @@ chunk_wrapper (void *user_data, const vo
| PyObject *py_subbuf = NULL;
| PyObject *py_error = NULL;
|
| - /* Cast away const */
| - py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ);
| + if (data->buf) {
| + PyObject *start, *end, *slice;
| + assert (PyMemoryView_Check (data->buf));
| + ptrdiff_t offs = subbuf - PyMemoryView_GET_BUFFER (data->buf)->buf;
| + start = PyLong_FromLong (offs);
| + if (!start) { PyErr_PrintEx (0); goto out; }
| + end = PyLong_FromLong (offs + count);
| + if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; }
| + slice = PySlice_New (start, end, NULL);
| + Py_DECREF (start);
| + Py_DECREF (end);
| + if (!slice) { PyErr_PrintEx (0); goto out; }
| + py_subbuf = PyObject_GetItem (data->buf, slice);
| + }
| + else {
| + /* Cast away const */
| + py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ);
| + }
| if (!py_subbuf) { PyErr_PrintEx (0); goto out; }
| PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
| if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
| @@ -115,11 +132,11 @@ chunk_wrapper (void *user_data, const vo
| };
|
| out:
| - if (py_subbuf) {
| + if (py_subbuf && !data->buf) {
| PyObject *tmp = PyObject_CallMethod(py_subbuf, "release", NULL);
| Py_XDECREF (tmp);
| - Py_DECREF (py_subbuf);
| }
| + Py_XDECREF (py_subbuf);
| if (py_error) {
| PyObject *py_error_ret = PyObject_GetAttrString (py_error, "value");
| *error = PyLong_AsLong (py_error_ret);
| @@ -2304,7 +2321,7 @@ nbd_internal_py_pread (PyObject *self, P
| struct nbd_handle *h;
| int ret;
| PyObject *py_ret = NULL;
| - PyObject *buf = NULL;
| + PyObject *py_buf = NULL;
| Py_ssize_t count;
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| @@ -2318,20 +2335,20 @@ nbd_internal_py_pread (PyObject *self, P
| h = get_handle (py_h);
| if (!h) goto out;
| flags_u32 = flags;
| - buf = PyByteArray_FromStringAndSize (NULL, count);
| - if (buf == NULL) goto out;
| + py_buf = PyByteArray_FromStringAndSize (NULL, count);
| + if (py_buf == NULL) goto out;
| offset_u64 = offset;
|
| - ret = nbd_pread (h, PyByteArray_AS_STRING (buf), count, offset_u64, flags_u32);
| + ret = nbd_pread (h, PyByteArray_AS_STRING (py_buf), count, offset_u64, flags_u32);
| if (ret == -1) {
| raise_exception ();
| goto out;
| }
| - py_ret = buf;
| - buf = NULL;
| + py_ret = py_buf;
| + py_buf = NULL;
|
| out:
| - Py_XDECREF (buf);
| + Py_XDECREF (py_buf);
| return py_ret;
| }
|
| @@ -2342,7 +2359,7 @@ nbd_internal_py_pread_structured (PyObje
| struct nbd_handle *h;
| int ret;
| PyObject *py_ret = NULL;
| - PyObject *buf = NULL;
| + PyObject *py_buf = NULL;
| Py_ssize_t count;
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| @@ -2360,8 +2377,8 @@ nbd_internal_py_pread_structured (PyObje
| h = get_handle (py_h);
| if (!h) goto out;
| flags_u32 = flags;
| - buf = PyByteArray_FromStringAndSize (NULL, count);
| - if (buf == NULL) goto out;
| + py_buf = PyByteArray_FromStringAndSize (NULL, count);
| + if (py_buf == NULL) goto out;
| offset_u64 = offset;
| chunk.user_data = chunk_user_data = alloc_user_data ();
| if (chunk_user_data == NULL) goto out;
| @@ -2373,18 +2390,20 @@ nbd_internal_py_pread_structured (PyObje
| /* Increment refcount since pointer may be saved by libnbd. */
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
| + if (py_buf)
| + chunk_user_data->buf = PyMemoryView_FromObject (py_buf);
|
| - ret = nbd_pread_structured (h, PyByteArray_AS_STRING (buf), count, offset_u64, chunk,
flags_u32);
| + ret = nbd_pread_structured (h, PyByteArray_AS_STRING (py_buf), count, offset_u64,
chunk, flags_u32);
| chunk_user_data = NULL;
| if (ret == -1) {
| raise_exception ();
| goto out;
| }
| - py_ret = buf;
| - buf = NULL;
| + py_ret = py_buf;
| + py_buf = NULL;
|
| out:
| - Py_XDECREF (buf);
| + Py_XDECREF (py_buf);
| free_user_data (chunk_user_data);
| return py_ret;
| }
| @@ -3171,6 +3190,7 @@ nbd_internal_py_aio_pread (PyObject *sel
| struct nbd_handle *h;
| int64_t ret;
| PyObject *py_ret = NULL;
| + PyObject *py_buf = NULL;
| PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| struct py_aio_buffer *buf_buf;
| uint64_t offset_u64;
| @@ -3230,6 +3250,7 @@ nbd_internal_py_aio_pread_structured (Py
| struct nbd_handle *h;
| int64_t ret;
| PyObject *py_ret = NULL;
| + PyObject *py_buf = NULL;
| PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */
| struct py_aio_buffer *buf_buf;
| uint64_t offset_u64;
| @@ -3282,6 +3303,8 @@ nbd_internal_py_aio_pread_structured (Py
| /* Increment refcount since pointer may be saved by libnbd. */
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
| + if (py_buf)
| + chunk_user_data->buf = PyMemoryView_FromObject (py_buf);
|
| ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len, offset_u64,
chunk, completion, flags_u32);
| chunk_user_data = NULL;
---
generator/Python.ml | 60 +++++++++++++++++++++++++++++++++------------
1 file changed, 44 insertions(+), 16 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 0191f79..4160759 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -187,8 +187,24 @@ let
pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n;
pr " }\n"
| CBBytesIn (n, len) ->
- pr " /* Cast away const */\n";
- pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n"
n n len;
+ pr " if (data->buf) {\n";
+ pr " PyObject *start, *end, *slice;\n";
+ pr " assert (PyMemoryView_Check (data->buf));\n";
+ pr " ptrdiff_t offs = %s - PyMemoryView_GET_BUFFER
(data->buf)->buf;\n" n;
+ pr " start = PyLong_FromLong (offs);\n";
+ pr " if (!start) { PyErr_PrintEx (0); goto out; }\n";
+ pr " end = PyLong_FromLong (offs + %s);\n" len;
+ pr " if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out;
}\n";
+ pr " slice = PySlice_New (start, end, NULL);\n";
+ pr " Py_DECREF (start);\n";
+ pr " Py_DECREF (end);\n";
+ pr " if (!slice) { PyErr_PrintEx (0); goto out; }\n";
+ pr " py_%s = PyObject_GetItem (data->buf, slice);\n" n;
+ pr " }\n";
+ pr " else {\n";
+ pr " /* Cast away const */\n";
+ pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s,
PyBUF_READ);\n" n n len;
+ pr " }\n";
pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n
| CBInt _
| CBInt64 _ -> ()
@@ -272,11 +288,11 @@ let
pr " }\n"
| CBBytesIn (n, _) ->
(*
https://stackoverflow.com/questions/58870010/how-to-call-release-on-a-mem...
*)
- pr " if (py_%s) {\n" n;
+ pr " if (py_%s && !data->buf) {\n" n;
pr " PyObject *tmp = PyObject_CallMethod(py_%s, \"release\",
NULL);\n" n;
pr " Py_XDECREF (tmp);\n";
- pr " Py_DECREF (py_%s);\n" n;
- pr " }\n"
+ pr " }\n";
+ pr " Py_XDECREF (py_%s);\n" n
| CBInt _ | CBInt64 _
| CBString _
| CBUInt _ | CBUInt64 _ -> ()
@@ -288,6 +304,7 @@ let
(* Generate the Python binding. *)
let print_python_binding name { args; optargs; ret; may_set_error } =
+ let store_mem = ref false in
pr "PyObject *\n";
pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name;
pr "{\n";
@@ -301,13 +318,19 @@ let
| BytesIn (n, _) ->
pr " Py_buffer %s = { .obj = NULL };\n" n
| BytesOut (n, count) ->
- pr " PyObject *%s = NULL;\n" n;
- pr " Py_ssize_t %s;\n" count
- | BytesPersistIn (n, _)
- | BytesPersistOut (n, _) ->
+ pr " PyObject *py_%s = NULL;\n" n;
+ pr " Py_ssize_t %s;\n" count;
+ store_mem := true
+ | BytesPersistIn (n, _) ->
pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n"
n;
pr " struct py_aio_buffer *%s_buf;\n" n
+ | BytesPersistOut (n, _) ->
+ pr " PyObject *py_%s = NULL;\n" n;
+ pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n"
+ n;
+ pr " struct py_aio_buffer *%s_buf;\n" n;
+ store_mem := true
| Closure { cbname } ->
pr " struct user_data *%s_user_data = NULL;\n" cbname;
pr " PyObject *py_%s_fn;\n" cbname;
@@ -440,8 +463,8 @@ let
| Bool _ -> ()
| BytesIn _ -> ()
| BytesOut (n, count) ->
- pr " %s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count;
- pr " if (%s == NULL) goto out;\n" n
+ pr " py_%s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count;
+ pr " if (py_%s == NULL) goto out;\n" n
| BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n;
pr " if (!%s_buf) goto out;\n" n;
@@ -458,7 +481,11 @@ let
pr " }\n";
pr " /* Increment refcount since pointer may be saved by libnbd.
*/\n";
pr " Py_INCREF (py_%s_fn);\n" cbname;
- pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname
+ pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname;
+ if !store_mem then (
+ pr " if (py_buf)\n";
+ pr " %s_user_data->buf = PyMemoryView_FromObject (py_buf);\n"
cbname
+ )
| Enum _ -> ()
| Flags (n, _) -> pr " %s_u32 = %s;\n" n n
| Fd _ | Int _ -> ()
@@ -487,7 +514,7 @@ let
function
| Bool n -> pr ", %s" n
| BytesIn (n, _) -> pr ", %s.buf, %s.len" n n
- | BytesOut (n, count) -> pr ", PyByteArray_AS_STRING (%s), %s" n count
+ | BytesOut (n, count) -> pr ", PyByteArray_AS_STRING (py_%s), %s" n
count
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) -> pr ", %s_buf->data, %s_buf->len" n n
| Closure { cbname } -> pr ", %s" cbname
@@ -533,8 +560,8 @@ let
List.iter (
function
| BytesOut (n, _) ->
- pr " py_ret = %s;\n" n;
- pr " %s = NULL;\n" n;
+ pr " py_ret = py_%s;\n" n;
+ pr " py_%s = NULL;\n" n;
use_ret := false
| Bool _
| BytesIn _
@@ -581,7 +608,7 @@ let
| BytesIn (n, _) ->
pr " if (%s.obj)\n" n;
pr " PyBuffer_Release (&%s);\n" n
- | BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n
+ | BytesOut (n, _) -> pr " Py_XDECREF (py_%s);\n" n
| BytesPersistIn _ | BytesPersistOut _ -> ()
| Closure { cbname } ->
pr " free_user_data (%s_user_data);\n" cbname
@@ -620,6 +647,7 @@ let
pr "#include <stdlib.h>\n";
pr "#include <stdint.h>\n";
pr "#include <stdbool.h>\n";
+ pr "#include <stddef.h>\n";
pr "\n";
pr "#include <libnbd.h>\n";
pr "\n";
--
2.36.1