Libguestfs - Feb 2023 - [PATCH 1/2] python: Avoid crash if callback parameters cannot be built

On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek <lersek at 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

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 at 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

On 2/20/23 20:08, Nir Soffer wrote:
> On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek <lersek at 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