[patch] concurrency fixes for the fuse daemon

[David Zeuthen <david fubar dk>]

Hey,

So I've been stress-testing the fuse backend and found that it tended to
segfault under load. My test case is a MTP device (Sansa Sandisk with
basically the default factory-installed music) and running e.g.

 rhythmbox /home/davidz/.gvfs/gphoto2 mount on usb%3A001,061

So SEGV's only happens when running multi-threaded which lead me to look
at locking / reference handling. I ended up using valgrind to figure out
that we're trying to access a FileHandle object that has been deleted
already. Which, not surprisingly, leads to a segfault.

The problem, I think, is that FileHandle objects are stored in the FUSE
user data and not reference counted. And this is a problem as
FileHandles are keyed off by path yet you can have several fuse
operations on the same file happening at the same time. So if you have
two operations in flight on the same file they'll share the FileHandle
object and then on the first vfs_release we free it and then we segfault
on the operation.

The attached patch adds some reference counting to FileHandle; seems to
fix the problems for me; at least no more valgrind warnings and no
SEGV's. As I'm not 100% familiar with the fuse daemon code base maybe
there's an easier and/or more correct fix. The locking seems a bit
fishy/racy in general but, as I said, I'm not 100% familiar with the
code base nor the operating constraints of libfuse. 

Please review carefully. Thanks!

     David

Index: client/gvfsfusedaemon.c
===================================================================
--- client/gvfsfusedaemon.c	(revision 1744)
+++ client/gvfsfusedaemon.c	(working copy)
@@ -71,6 +71,8 @@
 } FileOp;
 
 typedef struct {
+  gint      refcount;
+
   GMutex   *mutex;
   FileOp    op;
   gpointer  stream;
@@ -188,12 +190,26 @@
   FileHandle *file_handle;
 
   file_handle = g_new0 (FileHandle, 1);
+  file_handle->refcount = 1;
   file_handle->mutex = g_mutex_new ();
   file_handle->op = FILE_OP_NONE;
 
   return file_handle;
 }
 
+static FileHandle *
+file_handle_ref (FileHandle *file_handle)
+{
+  g_atomic_int_inc (&file_handle->refcount);
+  return file_handle;
+}
+
+static gboolean
+file_handle_unref (FileHandle *file_handle)
+{
+  return g_atomic_int_dec_and_test (&file_handle->refcount);
+}
+
 static void
 file_handle_close_stream (FileHandle *file_handle)
 {
@@ -278,21 +294,19 @@
   g_static_mutex_unlock (&global_mutex);
 }
 
-static gboolean
+static void
 free_file_handle_for_path (const gchar *path)
 {
   FileHandle *fh;
 
-  fh = get_file_handle_for_path (path);
+  g_static_mutex_lock (&global_mutex);
+  fh = g_hash_table_lookup (global_fh_table, path);
   if (fh)
     {
-      g_static_mutex_lock (&global_mutex);
-      g_hash_table_remove (global_fh_table, path);
-      g_static_mutex_unlock (&global_mutex);
-      return TRUE;
+      if (file_handle_unref (fh))
+        g_hash_table_remove (global_fh_table, path);
     }
-
-  return FALSE;
+  g_static_mutex_unlock (&global_mutex);
 }
 
 static MountRecord *
@@ -923,6 +937,7 @@
               
               /* File exists */
 
+              file_handle_ref (fh);
               SET_FILE_HANDLE (fi, fh);
 
               debug_print ("vfs_open: flags=%o\n", fi->flags);
@@ -1013,6 +1028,7 @@
 
               /* Success */
 
+              file_handle_ref (fh);
               SET_FILE_HANDLE (fi, fh);
 
               g_assert (fh->stream == NULL);