12:51 p.m.
In the case that building the parameters to the Python event callback
fails, args was returned as NULL. We immediately tried to call
Py_INCREF on this which crashed. Returning NULL means the Python
function threw an exception, so print the exception and return (there
is no way to return an error here - the event is lost).
Reported-by: Yonatan Shtarkman
See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
---
python/handle.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/python/handle.c b/python/handle.c
index c8752baf47..f37e939e03 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -134,18 +134,21 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
args = Py_BuildValue ("(Kis#O)",
(unsigned PY_LONG_LONG) event, event_handle,
buf, buf_len, py_array);
+ if (args == NULL) {
+ PyErr_PrintEx (0);
+ goto out;
+ }
+
Py_INCREF (args);
-
py_r = PyObject_CallObject (py_callback, args);
-
Py_DECREF (args);
-
if (py_r != NULL)
Py_DECREF (py_r);
else
/* Callback threw an exception: print it. */
PyErr_PrintEx (0);
+ out:
PyGILState_Release (py_save);
}
--
2.39.0
12:51 p.m.
New subject: [PATCH 2/2] python: Use bytes instead of str for event callback buffer
The event callback gets a buffer parameter which is usually something
like a log message. However as it comes from C it is not necessarily
well-formed (eg) UTF-8 but could contain any old byte sequence.
In the test case provided by the reported, we failed to encode the
buffer as str with this error:
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position 137:
unexpected end of data
Use bytes instead.
Reported-by: Yonatan Shtarkman
See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
---
python/handle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/python/handle.c b/python/handle.c
index f37e939e03..bf639b5789 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -131,7 +131,7 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
}
/* XXX As with Perl we don't pass the guestfs_h handle here. */
- args = Py_BuildValue ("(Kis#O)",
+ args = Py_BuildValue ("(Kiy#O)",
(unsigned PY_LONG_LONG) event, event_handle,
buf, buf_len, py_array);
if (args == NULL) {
--
2.39.0
7:27 a.m.
New subject: [PATCH 2/2] python: Use bytes instead of str for event callback buffer
The series was pushed as:
https://github.com/libguestfs/libguestfs/commit/6ef5837e2d8c5d4d83eff51c0...
https://github.com/libguestfs/libguestfs/commit/bbf396fc5562b4f0e27132f6b...
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
7:34 a.m.
New subject: [PATCH 2/2] python: Use bytes instead of str for event callback buffer
Thanks for the update!
On Wed, Feb 15, 2023 at 3:27 PM Richard W.M. Jones <rjones(a)redhat.com>
wrote:
The series was pushed as:
https://github.com/libguestfs/libguestfs/commit/6ef5837e2d8c5d4d83eff51c0...
https://github.com/libguestfs/libguestfs/commit/bbf396fc5562b4f0e27132f6b...
--
Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
8:22 a.m.
New subject: [PATCH 2/2] python: Use bytes instead of str for event callback buffer
On 2/14/23 19:51, Richard W.M. Jones wrote:
The event callback gets a buffer parameter which is usually
something
like a log message. However as it comes from C it is not necessarily
well-formed (eg) UTF-8 but could contain any old byte sequence.
In the test case provided by the reported, we failed to encode the
buffer as str with this error:
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position 137:
unexpected end of data
Use bytes instead.
Reported-by: Yonatan Shtarkman
See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
---
python/handle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/python/handle.c b/python/handle.c
index f37e939e03..bf639b5789 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -131,7 +131,7 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
}
/* XXX As with Perl we don't pass the guestfs_h handle here. */
- args = Py_BuildValue ("(Kis#O)",
+ args = Py_BuildValue ("(Kiy#O)",
(unsigned PY_LONG_LONG) event, event_handle,
buf, buf_len, py_array);
if (args == NULL) {
Looks reasonable, per Py_BuildValue() spec.
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
8:09 a.m.
On 2/14/23 19:51, Richard W.M. Jones wrote:
In the case that building the parameters to the Python event
callback
fails, args was returned as NULL. We immediately tried to call
Py_INCREF on this which crashed. Returning NULL means the Python
function threw an exception, so print the exception and return (there
is no way to return an error here - the event is lost).
Reported-by: Yonatan Shtarkman
See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
---
python/handle.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/python/handle.c b/python/handle.c
index c8752baf47..f37e939e03 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -134,18 +134,21 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
args = Py_BuildValue ("(Kis#O)",
(unsigned PY_LONG_LONG) event, event_handle,
buf, buf_len, py_array);
+ if (args == NULL) {
+ PyErr_PrintEx (0);
+ goto out;
+ }
+
Py_INCREF (args);
-
py_r = PyObject_CallObject (py_callback, args);
-
Py_DECREF (args);
-
if (py_r != NULL)
Py_DECREF (py_r);
else
/* Callback threw an exception: print it. */
PyErr_PrintEx (0);
+ out:
PyGILState_Release (py_save);
}
My understanding of object references in this function is the following:
- PyList_New creates a new object / new reference "py_array"
- Py_BuildValue with the "O" format specifier transfers the new list's
*sole* reference (= ownership) to the just-built higher-level object "args"
- when "args" is killed (decref'd), it takes care of "py_array".
Consequently, if Py_BuildValue fails, "py_array" continues owning the
new list -- and I believe that, if we take the new error branch, we leak
the object pointed-to by "py_array". Is that the case?
Laszlo
9:24 a.m.
On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
On 2/14/23 19:51, Richard W.M. Jones wrote:
> In the case that building the parameters to the Python event callback
> fails, args was returned as NULL. We immediately tried to call
> Py_INCREF on this which crashed. Returning NULL means the Python
> function threw an exception, so print the exception and return (there
> is no way to return an error here - the event is lost).
>
> Reported-by: Yonatan Shtarkman
> See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
> ---
> python/handle.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/python/handle.c b/python/handle.c
> index c8752baf47..f37e939e03 100644
> --- a/python/handle.c
> +++ b/python/handle.c
> @@ -134,18 +134,21 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
> args = Py_BuildValue ("(Kis#O)",
> (unsigned PY_LONG_LONG) event, event_handle,
> buf, buf_len, py_array);
> + if (args == NULL) {
> + PyErr_PrintEx (0);
> + goto out;
> + }
> +
> Py_INCREF (args);
> -
> py_r = PyObject_CallObject (py_callback, args);
> -
> Py_DECREF (args);
> -
> if (py_r != NULL)
> Py_DECREF (py_r);
> else
> /* Callback threw an exception: print it. */
> PyErr_PrintEx (0);
>
> + out:
> PyGILState_Release (py_save);
> }
>
My understanding of object references in this function is the following:
- PyList_New creates a new object / new reference "py_array"
- Py_BuildValue with the "O" format specifier transfers the new list's
*sole* reference (= ownership) to the just-built higher-level object "args"
- when "args" is killed (decref'd), it takes care of "py_array".
Consequently, if Py_BuildValue fails, "py_array" continues owning the
new list -- and I believe that, if we take the new error branch, we leak
the object pointed-to by "py_array". Is that the case?
Yes I think it would leak. Sent the fix as another patch.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
10:52 a.m.
On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
On 2/14/23 19:51, Richard W.M. Jones wrote:
> In the case that building the parameters to the Python event callback
> fails, args was returned as NULL. We immediately tried to call
> Py_INCREF on this which crashed. Returning NULL means the Python
> function threw an exception, so print the exception and return (there
> is no way to return an error here - the event is lost).
>
> Reported-by: Yonatan Shtarkman
> See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
> ---
> python/handle.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/python/handle.c b/python/handle.c
> index c8752baf47..f37e939e03 100644
> --- a/python/handle.c
> +++ b/python/handle.c
> @@ -134,18 +134,21 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
> args = Py_BuildValue ("(Kis#O)",
> (unsigned PY_LONG_LONG) event, event_handle,
> buf, buf_len, py_array);
According to https://docs.python.org/3/c-api/arg.html#building-values,
"O" increments the count of py_array. So before the Py_BuildValue
call, py_array has a refcount of 1. I think (although the docs are
unclear) that if Py_BuildValue fails, it rolls back any refcount
changes it makes before returning (since there is no new object
created, nothing exists that needs a second reference) - but you are
indeed left with py_array needing to be cleaned up to avoid a memory
leak.
> + if (args == NULL) {
> + PyErr_PrintEx (0);
> + goto out;
> + }
So reducing the reference count here on the error path makes sense.
However, on success, it means py_array now has a refcount of 2, and
args has a refcount of 1 (where cleaning up args will reduce only 1 of
the refs to py_array)...
> +
> Py_INCREF (args);
> -
> py_r = PyObject_CallObject (py_callback, args);
> -
> Py_DECREF (args);
This temporary increment/decrement of args around PyObject_CallObject
is a safety factor to ensure that the call itself doesn't try to
reclaim args while still in use. I'm not sure if it is strictly
required, but it doesn't hurt. At any rate, at this point, args is
back to refcount 1.
> -
> if (py_r != NULL)
> Py_DECREF (py_r);
> else
> /* Callback threw an exception: print it. */
> PyErr_PrintEx (0);
>
> + out:
> PyGILState_Release (py_save);
> }
...and we are STILL not releasing our count, of either py_array, or of args.
If I understand Python bindings in C correctly, you want to
unconditionally decrement the refcount of py_array after Py_BuildValue
(either reducing it to 0 because args is NULL and you don't need py_array
anymore, or reducing it to 1 because cleaning up args will take care
of it), and you also need to add a Py_DECREF(args) under out.
>
My understanding of object references in this function is the following:
- PyList_New creates a new object / new reference "py_array"
- Py_BuildValue with the "O" format specifier transfers the new list's
*sole* reference (= ownership) to the just-built higher-level object "args"
Reference transfer is done with "N", not "O". That would be an
alternative to decreasing the refcount of py_array on success, but not
eliminate the need to decrease the refcount on Py_BuildValue failure.
- when "args" is killed (decref'd), it takes care of "py_array".
Consequently, if Py_BuildValue fails, "py_array" continues owning the
new list -- and I believe that, if we take the new error branch, we leak
the object pointed-to by "py_array". Is that the case?
Not quite. "O" is different than "N".
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
2:45 a.m.
On 2/17/23 17:52, Eric Blake wrote:
On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
> - Py_BuildValue with the "O" format specifier transfers
the new list's
> *sole* reference (= ownership) to the just-built higher-level object
"args"
Reference transfer is done with "N", not "O". That would be an
alternative to decreasing the refcount of py_array on success, but not
eliminate the need to decrease the refcount on Py_BuildValue failure.
>
> - when "args" is killed (decref'd), it takes care of
"py_array".
>
> Consequently, if Py_BuildValue fails, "py_array" continues owning the
> new list -- and I believe that, if we take the new error branch, we leak
> the object pointed-to by "py_array". Is that the case?
Not quite. "O" is different than "N".
I agree with you *now*, looking up the "O" specification at
<https://docs.python.org/3/c-api/arg.html#building-values>;.
However, when I was writing my email, I looked up Py_BuildValue at that
time as well, just elsewhere. I don't know where. Really. And then that
documentation said that the reference count would *not* be increased. I
distinctly remember that, because it surprised me -- I actually recalled
an *even earlier* experience reading the documentation, which had again
stated that "O" would increase the reference count.
So right now, I have three (inconsistent) memories:
- original (old) memory: "O" increments the refcount
- recent memory: "O" does not increment the refcount
- your reminder: "O" does increment the refcount
I guess I must have misread something (I can't find the document now!).
Sorry about that; I agree we need to drop the original py_array
reference unconditionally.
Laszlo
3:10 a.m.
On Mon, Feb 20, 2023 at 09:45:33AM +0100, Laszlo Ersek wrote:
On 2/17/23 17:52, Eric Blake wrote:
> On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
>> - Py_BuildValue with the "O" format specifier transfers the new
list's
>> *sole* reference (= ownership) to the just-built higher-level object
"args"
>
> Reference transfer is done with "N", not "O". That would be an
> alternative to decreasing the refcount of py_array on success, but not
> eliminate the need to decrease the refcount on Py_BuildValue failure.
>
>>
>> - when "args" is killed (decref'd), it takes care of
"py_array".
>>
>> Consequently, if Py_BuildValue fails, "py_array" continues owning the
>> new list -- and I believe that, if we take the new error branch, we leak
>> the object pointed-to by "py_array". Is that the case?
>
> Not quite. "O" is different than "N".
I agree with you *now*, looking up the "O" specification at
<https://docs.python.org/3/c-api/arg.html#building-values>;.
However, when I was writing my email, I looked up Py_BuildValue at that
time as well, just elsewhere. I don't know where. Really. And then that
documentation said that the reference count would *not* be increased. I
distinctly remember that, because it surprised me -- I actually recalled
an *even earlier* experience reading the documentation, which had again
stated that "O" would increase the reference count.
So right now, I have three (inconsistent) memories:
- original (old) memory: "O" increments the refcount
- recent memory: "O" does not increment the refcount
- your reminder: "O" does increment the refcount
I looked at the source, and 'O' definitely increments the refcount
whereas 'N' does not. See:
https://github.com/python/cpython/blob/b1b375e2670a58fc37cb4c2629ed73b045...
and:
https://github.com/python/cpython/blob/b1b375e2670a58fc37cb4c2629ed73b045...
Rich.
I guess I must have misread something (I can't find the document
now!).
Sorry about that; I agree we need to drop the original py_array
reference unconditionally.
Laszlo
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
1:08 p.m.
On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek <lersek(a)redhat.com> wrote:
On 2/17/23 17:52, Eric Blake wrote:
> On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
>> - Py_BuildValue with the "O" format specifier transfers the new
list's
>> *sole* reference (= ownership) to the just-built higher-level object
"args"
>
> Reference transfer is done with "N", not "O". That would be an
> alternative to decreasing the refcount of py_array on success, but not
> eliminate the need to decrease the refcount on Py_BuildValue failure.
>
>>
>> - when "args" is killed (decref'd), it takes care of
"py_array".
>>
>> Consequently, if Py_BuildValue fails, "py_array" continues owning the
>> new list -- and I believe that, if we take the new error branch, we leak
>> the object pointed-to by "py_array". Is that the case?
>
> Not quite. "O" is different than "N".
I agree with you *now*, looking up the "O" specification at
<https://docs.python.org/3/c-api/arg.html#building-values>;.
However, when I was writing my email, I looked up Py_BuildValue at that
time as well, just elsewhere. I don't know where. Really. And then that
documentation said that the reference count would *not* be increased. I
distinctly remember that, because it surprised me -- I actually recalled
an *even earlier* experience reading the documentation, which had again
stated that "O" would increase the reference count.
Maybe here:
https://docs.python.org/2/c-api/arg.html#building-values
Looks like another incompatibility between python 2 and 3.
Nir
2:52 p.m.
On Mon, Feb 20, 2023 at 09:08:08PM +0200, Nir Soffer wrote:
On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek
<lersek(a)redhat.com> wrote:
>
> On 2/17/23 17:52, Eric Blake wrote:
> > On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
>
> >> - Py_BuildValue with the "O" format specifier transfers the new
list's
> >> *sole* reference (= ownership) to the just-built higher-level object
"args"
> >
> > Reference transfer is done with "N", not "O". That would
be an
> > alternative to decreasing the refcount of py_array on success, but not
> > eliminate the need to decrease the refcount on Py_BuildValue failure.
> >
> >>
> >> - when "args" is killed (decref'd), it takes care of
"py_array".
> >>
> >> Consequently, if Py_BuildValue fails, "py_array" continues owning
the
> >> new list -- and I believe that, if we take the new error branch, we leak
> >> the object pointed-to by "py_array". Is that the case?
> >
> > Not quite. "O" is different than "N".
>
> I agree with you *now*, looking up the "O" specification at
> <https://docs.python.org/3/c-api/arg.html#building-values>;.
>
> However, when I was writing my email, I looked up Py_BuildValue at that
> time as well, just elsewhere. I don't know where. Really. And then that
> documentation said that the reference count would *not* be increased. I
> distinctly remember that, because it surprised me -- I actually recalled
> an *even earlier* experience reading the documentation, which had again
> stated that "O" would increase the reference count.
Maybe here:
https://docs.python.org/2/c-api/arg.html#building-values
Looks like another incompatibility between python 2 and 3.
Or maybe misreading the wrong part of the page. PyArg_ParseTuple()
and Py_BuildValue() are listed on the same page, and intentionally use
similar-looking format strings, so you have to check _which_ part of
the page the "O" you are reading about is associated with. The first
"O" is under PyArg_ParseTuple() and friends, and intentionally does
not increase reference count (the usesr passed us an Object, we are
parsing it into a placeholder where we don't need to clean up after
ourselves unless we want to add a reference to the object to make it
last beyond the caller), the latter says that "O" does increase the
reference count (we are building up a larger object that will now be
an additional reference path into the object we are passing in).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
8:06 a.m.
On 2/20/23 21:52, Eric Blake wrote:
On Mon, Feb 20, 2023 at 09:08:08PM +0200, Nir Soffer wrote:
> On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek <lersek(a)redhat.com> wrote:
>>
>> On 2/17/23 17:52, Eric Blake wrote:
>>> On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
>>
>>>> - Py_BuildValue with the "O" format specifier transfers the new
list's
>>>> *sole* reference (= ownership) to the just-built higher-level object
"args"
>>>
>>> Reference transfer is done with "N", not "O". That would
be an
>>> alternative to decreasing the refcount of py_array on success, but not
>>> eliminate the need to decrease the refcount on Py_BuildValue failure.
>>>
>>>>
>>>> - when "args" is killed (decref'd), it takes care of
"py_array".
>>>>
>>>> Consequently, if Py_BuildValue fails, "py_array" continues
owning the
>>>> new list -- and I believe that, if we take the new error branch, we leak
>>>> the object pointed-to by "py_array". Is that the case?
>>>
>>> Not quite. "O" is different than "N".
>>
>> I agree with you *now*, looking up the "O" specification at
>> <https://docs.python.org/3/c-api/arg.html#building-values>;.
>>
>> However, when I was writing my email, I looked up Py_BuildValue at that
>> time as well, just elsewhere. I don't know where. Really. And then that
>> documentation said that the reference count would *not* be increased. I
>> distinctly remember that, because it surprised me -- I actually recalled
>> an *even earlier* experience reading the documentation, which had again
>> stated that "O" would increase the reference count.
>
> Maybe here:
> https://docs.python.org/2/c-api/arg.html#building-values
>
> Looks like another incompatibility between python 2 and 3.
Or maybe misreading the wrong part of the page. PyArg_ParseTuple()
and Py_BuildValue() are listed on the same page, and intentionally use
similar-looking format strings, so you have to check _which_ part of
the page the "O" you are reading about is associated with. The first
"O" is under PyArg_ParseTuple() and friends, and intentionally does
not increase reference count (the usesr passed us an Object, we are
parsing it into a placeholder where we don't need to clean up after
ourselves unless we want to add a reference to the object to make it
last beyond the caller), the latter says that "O" does increase the
reference count (we are building up a larger object that will now be
an additional reference path into the object we are passing in).
Yea, I'll just go ahead and call myself an idiot. :)
(Please, please, make documentation fool-proof! I'm not an unrelenting,
unrepentant, principled fool, but still a fool.)
Laszlo
2:07 p.m.
From: Eric Blake <eblake(a)redhat.com>
To: Laszlo Ersek <lersek(a)redhat.com>, docs(a)python.org
Cc: Nir Soffer <nsoffer(a)redhat.com>, aryeyur(a)google.com,
libguestfs(a)redhat.com, shtarkman(a)google.com
Bcc:
Subject: Re: [Libguestfs] [PATCH 1/2] python: Avoid crash if callback
parameters cannot be built
Reply-To:
In-Reply-To: <c8259881-9fac-5987-8ac1-3286ff35adc1(a)redhat.com>
[adding docs(a)python.org, in the hopes that this conversation can spur
an improvement in Python's documentation]
I'm trimming to just context on the doc issue at hand (this question
spawned deep within a thread [1] to libguestfs reference counting bug
in its Python bindings)
[1] https://listman.redhat.com/archives/libguestfs/2023-February/030663.html
On Tue, Feb 21, 2023 at 03:06:47PM +0100, Laszlo Ersek wrote:
On 2/20/23 21:52, Eric Blake wrote:
> On Mon, Feb 20, 2023 at 09:08:08PM +0200, Nir Soffer wrote:
>> On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek <lersek(a)redhat.com> wrote:
>>>
>>> On 2/17/23 17:52, Eric Blake wrote:
>>>> On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
>>>
>>>>> - Py_BuildValue with the "O" format specifier transfers
the new list's
>>>>> *sole* reference (= ownership) to the just-built higher-level object
"args"
>>>>
>>>> Reference transfer is done with "N", not "O". That
would be an
>>>> alternative to decreasing the refcount of py_array on success, but not
>>>> eliminate the need to decrease the refcount on Py_BuildValue failure.
>>>>
>>>>>
>>>>> - when "args" is killed (decref'd), it takes care of
"py_array".
>>>>>
>>>>> Consequently, if Py_BuildValue fails, "py_array" continues
owning the
>>>>> new list -- and I believe that, if we take the new error branch, we
leak
>>>>> the object pointed-to by "py_array". Is that the case?
>>>>
>>>> Not quite. "O" is different than "N".
>>>
>>> I agree with you *now*, looking up the "O" specification at
>>> <https://docs.python.org/3/c-api/arg.html#building-values>;.
>>>
>>> However, when I was writing my email, I looked up Py_BuildValue at that
>>> time as well, just elsewhere. I don't know where. Really. And then that
>>> documentation said that the reference count would *not* be increased. I
>>> distinctly remember that, because it surprised me -- I actually recalled
>>> an *even earlier* experience reading the documentation, which had again
>>> stated that "O" would increase the reference count.
>>
>> Maybe here:
>> https://docs.python.org/2/c-api/arg.html#building-values
>>
>> Looks like another incompatibility between python 2 and 3.
>
> Or maybe misreading the wrong part of the page. PyArg_ParseTuple()
> and Py_BuildValue() are listed on the same page, and intentionally use
> similar-looking format strings, so you have to check _which_ part of
> the page the "O" you are reading about is associated with. The first
> "O" is under PyArg_ParseTuple() and friends, and intentionally does
> not increase reference count (the usesr passed us an Object, we are
> parsing it into a placeholder where we don't need to clean up after
> ourselves unless we want to add a reference to the object to make it
> last beyond the caller), the latter says that "O" does increase the
> reference count (we are building up a larger object that will now be
> an additional reference path into the object we are passing in).
>
Yea, I'll just go ahead and call myself an idiot. :)
(Please, please, make documentation fool-proof! I'm not an unrelenting,
unrepentant, principled fool, but still a fool.)
In short, the issue is that the Python documentation (even in the
latest version at https://docs.python.org/3/c-api/arg.html) has TWO
separate paradigms on the SAME page, and so many format characters
that you can easily get lost in a wall of
X (foo) [bar]
designators that you can easily lose track of whether you are reading
the docs for PyArg_ParseTuple() and friends with paradigm "X (Python
source) [C destination]", or the docs for Py_BuildValue() with
paradigm "X (Python destination) [C source]".
When it comes to reference counting, there is a HUGE and IMPORTANT
difference between the former:
O (object) [PyObject *]
Store a Python object (without any conversion) in a C object
pointer. The C program thus receives the actual object that was
passed. The object’s reference count is not increased. The pointer
stored is not NULL.
and the latter:
O (object) [PyObject *]
Pass a Python object untouched (except for its reference count,
which is incremented by one). If the object passed in is a NULL
pointer, it is assumed that this was caused because the call
producing the argument found an error and set an
exception. Therefore, Py_BuildValue() will return NULL but won’t
raise an exception. If no exception has been raised yet,
SystemError is set.
But as neither instance of "O (object) [PyObject *]" is on the screen
at the same time as the functions to which that format specifier is
documenting, it is EASY for a novice coder to read the wrong one, and
then get the wrong ideas about reference counting when using the "O"
format specifier.
My suggestion: a simple swap of the order of the Py_BuildValue()
notations to:
PyArg_ParseValue()...
X (Python source) [C destination]
Py_BuildValue()...
X [C source] (Python destination]
so that it is always source before destination, and always [] for the
C counterpart and () for the Python counterpart. Another idea might
be splitting the page into two, so that the two walls of format
specifier text cannot be mistaken for one another when rapidly
scrolling through the page looking for a given format letter; but I
personally like both on the same page as the two sets of format
specifiers are (intentionally) related, even if subtly different
depending on direction.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
8:05 a.m.
On 2/20/23 20:08, Nir Soffer wrote:
On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek
<lersek(a)redhat.com> wrote:
>
> On 2/17/23 17:52, Eric Blake wrote:
>> On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
>
>>> - Py_BuildValue with the "O" format specifier transfers the new
list's
>>> *sole* reference (= ownership) to the just-built higher-level object
"args"
>>
>> Reference transfer is done with "N", not "O". That would be
an
>> alternative to decreasing the refcount of py_array on success, but not
>> eliminate the need to decrease the refcount on Py_BuildValue failure.
>>
>>>
>>> - when "args" is killed (decref'd), it takes care of
"py_array".
>>>
>>> Consequently, if Py_BuildValue fails, "py_array" continues owning
the
>>> new list -- and I believe that, if we take the new error branch, we leak
>>> the object pointed-to by "py_array". Is that the case?
>>
>> Not quite. "O" is different than "N".
>
> I agree with you *now*, looking up the "O" specification at
> <https://docs.python.org/3/c-api/arg.html#building-values>;.
>
> However, when I was writing my email, I looked up Py_BuildValue at that
> time as well, just elsewhere. I don't know where. Really. And then that
> documentation said that the reference count would *not* be increased. I
> distinctly remember that, because it surprised me -- I actually recalled
> an *even earlier* experience reading the documentation, which had again
> stated that "O" would increase the reference count.
Maybe here:
https://docs.python.org/2/c-api/arg.html#building-values
Looks like another incompatibility between python 2 and 3.
Yes, thank you! That's it *exactly*!
O (object) [PyObject *]
Store a Python object (without any conversion) in a C object
pointer. The C program thus receives the actual object that was
passed. The object’s reference count is not increased. The pointer
stored is not NULL.
Laszlo
564
days inactive
571
days old
14 comments
5 participants
participants (5)
-
Eric Blake -
Laszlo Ersek -
Nir Soffer -
Richard W.M. Jones -
Yonatan Shtarkman