Eric Blake
2023-Feb-20 20:52 UTC
[Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built
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
Laszlo Ersek
2023-Feb-21 14:06 UTC
[Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built
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 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). >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