Libguestfs - Sep 2014 - [PATCH] lua: improve handle closing

Swap guestfs_close and free_per_handle_table, so first the handle is
removed from the table and then freed. This avoids passing a stale
pointer to free_per_handle_table (which right now is not an issue, but
better make sure it is never so).
---
Maybe this could also fix the case described by the comment there,
as when a new 'g' with the address of an old 'g' would be
created,
the old 'g' is now already gone from the per-handle table.

 generator/lua.ml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator/lua.ml b/generator/lua.ml
index 5d5619c..fa6e9f2 100644
--- a/generator/lua.ml
+++ b/generator/lua.ml
@@ -168,13 +168,13 @@ guestfs_lua_create (lua_State *L)
 static void
 close_handle (lua_State *L, guestfs_h *g)
 {
-  guestfs_close (g);
   /* There is a potential and hard-to-solve race here: If another
    * thread allocates another 'g' at the same address, then
    * get_per_handle_table might be called with the same address
    * before we call free_per_handle_table here.  XXX
    */
   free_per_handle_table (L, g);
+  guestfs_close (g);
 }
 
 /* Finalizer. */
-- 
1.9.3

On Wed, Sep 10, 2014 at 01:13:07PM +0200, Pino Toscano
wrote:
> Swap guestfs_close and free_per_handle_table, so first the handle is
> removed from the table and then freed. This avoids passing a stale
> pointer to free_per_handle_table (which right now is not an issue, but
> better make sure it is never so).
> ---
> Maybe this could also fix the case described by the comment there,
> as when a new 'g' with the address of an old 'g' would be
created,
> the old 'g' is now already gone from the per-handle table.
> 
>  generator/lua.ml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/generator/lua.ml b/generator/lua.ml
> index 5d5619c..fa6e9f2 100644
> --- a/generator/lua.ml
> +++ b/generator/lua.ml
> @@ -168,13 +168,13 @@ guestfs_lua_create (lua_State *L)
>  static void
>  close_handle (lua_State *L, guestfs_h *g)
>  {
> -  guestfs_close (g);
>    /* There is a potential and hard-to-solve race here: If another
>     * thread allocates another 'g' at the same address, then
>     * get_per_handle_table might be called with the same address
>     * before we call free_per_handle_table here.  XXX
>     */
>    free_per_handle_table (L, g);
> +  guestfs_close (g);
>  }
I have a vague feeling that the order of operations here was important
for some reason.  And presumably I wouldn't have written that comment
if the solution was as simple as swapping the two statements around.

So .. not sure.  I think we should leave this alone.

I can't find anything in the git commits to indicate what the problem
might have been.

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