[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.



On Tue, Jul 16, 2019 at 09:03:55AM -0500, Eric Blake wrote:
> > +       (* Persistent closures need an explicit function to decrement
> > +        * the closure refcounts and free the user_data struct.
> > +        *)
> > +       if persistent then (
> > +         pr "static void\n";
> > +         pr "free_%s_user_data (void *vp)\n" name;
> > +         pr "{\n";
> > +         pr "  struct %s_user_data *user_data = vp;\n" name;
> > +         pr "\n";
> > +         List.iter (
> > +           fun { cbname } ->
> > +             pr "  Py_DECREF (user_data->%s);\n" cbname
> 
> Oh, good fix. We were not previously incrementing the ref-count of the
> Python Callable, and could have ended up deferencing a garbage-collected
> object.

Yeah I actually hit this in the revised test case that uses lambdas,
because it turns out that lambdas in Python are refcounted (but
staticly defined functions are not).

> > +  (* Allocate the persistent closure user_data. *)
> >    List.iter (
> >      function
> > -    | ArrayAndLen _
> > -    | Bool _
> > -    | BytesIn _
> > -    | BytesPersistIn _
> > -    | BytesOut _
> > -    | BytesPersistOut _
> > -    | Callback _ -> ()
> > -    | CallbackPersist (n, _) ->
> > -       pr "  %s_data = malloc (sizeof *%s_data);\n" n n;
> > -       pr "  if (%s_data == NULL) {\n" n;
> > +    | Closure (false, _) -> ()
> > +    | Closure (true, cls) ->
> > +       pr "  user_data = malloc (sizeof *user_data);\n";
> > +       pr "  if (user_data == NULL) {\n";
> >         pr "    PyErr_NoMemory ();\n";
> >         pr "    return NULL;\n";
> >         pr "  }\n"
> 
> Memory leak. If Closure contains two callbacks, and the first malloc()
> succeeds while the second fails, we are not reclaiming the first.
> Should our generated python code take advantage of
> __attribute__((cleanup)) to make it easier to write?

Actually I don't think there's a memory leak here because Closure only
occurs once in the argument list (I have added a check for this based
on your comment).

_However_ there is certainly a more general problem that the Python
bindings don't carefully clean up allocations on error paths.
attribute((cleanup)) doesn't help in this particular case (but would
in others) since we need the malloc to persist after the call.

> > @@ -4017,6 +4017,20 @@ let print_python_binding name { args; ret } =
> >    pr "))\n";
> >    pr "    return NULL;\n";
> >  
> > +  (* If the parameter has persistent closures then we need to
> > +   * make sure the ref count remains positive.
> > +   *)
> > +  List.iter (
> > +    function
> > +    | Closure (false, _) -> ()
> > +    | Closure (true, cls) ->
> > +       List.iter (
> > +         fun { cbname } ->
> > +           pr "  Py_INCREF (user_data->%s);\n" cbname
> > +       ) cls
> > +    | _ -> ()
> > +  ) args;
> > +
> 
> Any reason this loop is a separate pass, rather than...
> 
> >    pr "  h = get_handle (py_h);\n";
> >    List.iter (
> >      function
> > @@ -4044,19 +4058,20 @@ let print_python_binding name { args; ret } =
> >         pr "  %s = malloc (%s);\n" n count
> >      | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
> >         pr "  %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
> > -    | Callback (n, _) | CallbackPersist (n, _) ->
> > -       pr "  if (!PyCallable_Check (%s_data->fn)) {\n" n;
> > -       pr "    PyErr_SetString (PyExc_TypeError,\n";
> > -       pr "                     \"callback parameter %s is not callable\");\n"
> > -          n;
> > -       pr "    return NULL;\n";
> > -       pr "  }\n"
> > +    | Closure (_, cls) ->
> > +       List.iter (
> > +         fun { cbname } ->
> > +           pr "  if (!PyCallable_Check (user_data->%s)) {\n" cbname;
> > +           pr "    PyErr_SetString (PyExc_TypeError,\n";
> > +           pr "                     \"callback parameter %s is not callable\");\n" cbname;
> > +           pr "    return NULL;\n";
> > +           pr "  }\n"
> > +       ) cls
> 
> ...done here?

Py_INCREF is only done for persistent closures, so the loops are
slightly different (note ‘Closure (false, _) -> ()’ in the false case
in the first loop).  We could combine them into one loop with a test
of the persistent boolean, but heh, loops are cheap in the generator :-)

> Also, you have a reference leaks: if the python code
> passes a non-Callable, you fail to DECREF it (made trickier when the
> Closure has two callbacks, and only the second callback is not Callable).

Yes, this is one of the many ways that the Python code will leak on
error paths.

> We still need to add coverage of h.aio_pread_structured_callback, to
> prove that this actually did what we wanted when two callbacks are
> present (the test will fail without this patch).

Yes it _should_ be fixed by this patch.

I have corrected all the stuff above (except for the general memory
leaks on error) and have pushed a version which includes also OCaml
changes.  I will prepare a new patch soon for fixing
nbd_add_close_callback.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]