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

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

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/b1b375e2670a58fc37cb4c2629ed73b045159918/Python/modsupport.c#L463

and:

https://github.com/python/cpython/blob/b1b375e2670a58fc37cb4c2629ed73b045159918/Python/modsupport.c#L475

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

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