From 27aacd022bd01a805f470994f34a9605055c89ff Mon Sep 17 00:00:00 2001 From: Hans Petter Jansson Date: Mon, 20 Oct 2008 09:03:43 +0000 Subject: Import from trunk: 2008-10-17 Hans Petter Jansson Import from trunk: Attempt to prevent potential race conditions in the FUSE backend when file handles get closed while still in use in another thread, if that ever happens. * client/gvfsfusedaemon.c (file_handle_new): Insert new file handles in global hash table of active file handles. (file_handle_unref): Clarify the code and comments a little. (file_handle_free): Remove file handle from global table of active handles. (reindex_file_handle_for_path) (get_file_handle_for_path) (get_or_create_file_handle_for_path): global_fh_table -> global_path_to_fh_map. (get_file_handle_from_info): New function that recovers our file handle from a fuse_file_info struct, but only if it exists in the global table of valid handles. (vfs_getattr): Remove code that acquired and locked the file handle for the path we operate on. No locking is required here. (vfs_open): Assign file handle to fuse_file_info while holding lock. Purely a formality that makes code easier to read. (vfs_create): Ditto. (vfs_release): Use get_file_handle_from_info () so the file handle is validated. (vfs_read): Hold a ref to the file handle while it's in use. If handle is invalid, raise EINVAL. (vfs_ftruncate): Ditto. (vfs_write): Ditto. (vfs_rename): Cosmetic change. (vfs_unlink): Ditto. (vfs_truncate): Add helpful comment. (vfs_init): Create global table of active file handles. svn path=/branches/gnome-2-24/; revision=2063 --- ChangeLog | 36 ++++++++ client/gvfsfusedaemon.c | 217 +++++++++++++++++++++++++++++------------------- 2 files changed, 168 insertions(+), 85 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6b83cc3c..4b0c19b6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,39 @@ +2008-10-17 Hans Petter Jansson + + Import from trunk: + + Attempt to prevent potential race conditions in the FUSE backend + when file handles get closed while still in use in another thread, + if that ever happens. + + * client/gvfsfusedaemon.c (file_handle_new): Insert new file + handles in global hash table of active file handles. + (file_handle_unref): Clarify the code and comments a little. + (file_handle_free): Remove file handle from global table of + active handles. + (reindex_file_handle_for_path) + (get_file_handle_for_path) + (get_or_create_file_handle_for_path): global_fh_table -> + global_path_to_fh_map. + (get_file_handle_from_info): New function that recovers our file + handle from a fuse_file_info struct, but only if it exists in + the global table of valid handles. + (vfs_getattr): Remove code that acquired and locked the file handle + for the path we operate on. No locking is required here. + (vfs_open): Assign file handle to fuse_file_info while holding lock. + Purely a formality that makes code easier to read. + (vfs_create): Ditto. + (vfs_release): Use get_file_handle_from_info () so the file handle + is validated. + (vfs_read): Hold a ref to the file handle while it's in use. If + handle is invalid, raise EINVAL. + (vfs_ftruncate): Ditto. + (vfs_write): Ditto. + (vfs_rename): Cosmetic change. + (vfs_unlink): Ditto. + (vfs_truncate): Add helpful comment. + (vfs_init): Create global table of active file handles. + 2008-10-20 Alexander Larsson Import from trunk: diff --git a/client/gvfsfusedaemon.c b/client/gvfsfusedaemon.c index d87dcf86..b61e96c2 100644 --- a/client/gvfsfusedaemon.c +++ b/client/gvfsfusedaemon.c @@ -79,22 +79,23 @@ typedef struct { size_t pos; } FileHandle; -static GThread *subthread = NULL; -static GMainLoop *subthread_main_loop = NULL; -static GVfs *gvfs = NULL; +static GThread *subthread = NULL; +static GMainLoop *subthread_main_loop = NULL; +static GVfs *gvfs = NULL; -static GVolumeMonitor *volume_monitor = NULL; +static GVolumeMonitor *volume_monitor = NULL; /* Contains pointers to MountRecord */ -static GList *mount_list = NULL; -static GMutex *mount_list_mutex; +static GList *mount_list = NULL; +static GMutex *mount_list_mutex; -static time_t daemon_creation_time; -static uid_t daemon_uid; -static gid_t daemon_gid; +static time_t daemon_creation_time; +static uid_t daemon_uid; +static gid_t daemon_gid; -static GStaticMutex global_mutex = G_STATIC_MUTEX_INIT; -static GHashTable *global_fh_table = NULL; +static GStaticMutex global_mutex = G_STATIC_MUTEX_INIT; +static GHashTable *global_path_to_fh_map = NULL; +static GHashTable *global_active_fh_map = NULL; /* ------- * * Helpers * @@ -194,6 +195,8 @@ file_handle_new (const gchar *path) file_handle->op = FILE_OP_NONE; file_handle->path = g_strdup (path); + g_hash_table_insert (global_active_fh_map, file_handle, file_handle); + return file_handle; } @@ -209,14 +212,18 @@ file_handle_unref (FileHandle *file_handle) { if (g_atomic_int_dec_and_test (&file_handle->refcount)) { + gint refs; + g_static_mutex_lock (&global_mutex); /* Test again, since e.g. get_file_handle_for_path() might have * snatched the global mutex and revived the file handle between - * g_atomic_int_dec_and_test() and us obtaining the lock. */ + * g_atomic_int_dec_and_test() and us obtaining the global lock. */ - if (g_atomic_int_get (&file_handle->refcount) == 0) - g_hash_table_remove (global_fh_table, file_handle->path); + refs = g_atomic_int_get (&file_handle->refcount); + + if (refs == 0) + g_hash_table_remove (global_path_to_fh_map, file_handle->path); g_static_mutex_unlock (&global_mutex); } @@ -252,6 +259,8 @@ file_handle_close_stream (FileHandle *file_handle) static void file_handle_free (FileHandle *file_handle) { + g_hash_table_remove (global_active_fh_map, file_handle); + file_handle_close_stream (file_handle); g_mutex_free (file_handle->mutex); g_free (file_handle->path); @@ -265,12 +274,12 @@ get_file_handle_for_path (const gchar *path) g_static_mutex_lock (&global_mutex); - fh = g_hash_table_lookup (global_fh_table, path); + fh = g_hash_table_lookup (global_path_to_fh_map, path); + if (fh) file_handle_ref (fh); g_static_mutex_unlock (&global_mutex); - return fh; } @@ -281,7 +290,7 @@ get_or_create_file_handle_for_path (const gchar *path) g_static_mutex_lock (&global_mutex); - fh = g_hash_table_lookup (global_fh_table, path); + fh = g_hash_table_lookup (global_path_to_fh_map, path); if (fh) { @@ -290,11 +299,30 @@ get_or_create_file_handle_for_path (const gchar *path) else { fh = file_handle_new (path); - g_hash_table_insert (global_fh_table, fh->path, fh); + g_hash_table_insert (global_path_to_fh_map, fh->path, fh); } g_static_mutex_unlock (&global_mutex); + return fh; +} + +static FileHandle * +get_file_handle_from_info (struct fuse_file_info *fi) +{ + FileHandle *fh; + g_static_mutex_lock (&global_mutex); + + fh = GET_FILE_HANDLE (fi); + + /* If the file handle is still valid, its value won't change. If + * invalid, it's set to NULL. */ + fh = g_hash_table_lookup (global_active_fh_map, fh); + + if (fh) + file_handle_ref (fh); + + g_static_mutex_unlock (&global_mutex); return fh; } @@ -306,17 +334,17 @@ reindex_file_handle_for_path (const gchar *old_path, const gchar *new_path) g_static_mutex_lock (&global_mutex); - if (!g_hash_table_lookup_extended (global_fh_table, old_path, + if (!g_hash_table_lookup_extended (global_path_to_fh_map, old_path, (gpointer *) &old_path_internal, (gpointer *) &fh)) goto out; - g_hash_table_steal (global_fh_table, old_path); + g_hash_table_steal (global_path_to_fh_map, old_path); g_free (fh->path); fh->path = g_strdup (new_path); - g_hash_table_insert (global_fh_table, fh->path, fh); + g_hash_table_insert (global_path_to_fh_map, fh->path, fh); out: g_static_mutex_unlock (&global_mutex); @@ -779,14 +807,9 @@ vfs_getattr (const gchar *path, struct stat *sbuf) { GFile *file; gint result = 0; - FileHandle *fh; debug_print ("vfs_getattr: %s\n", path); - fh = get_file_handle_for_path (path); - if (fh) - g_mutex_lock (fh->mutex); - memset (sbuf, 0, sizeof (*sbuf)); sbuf->st_dev = 0; /* dev_t ID of device containing file */ @@ -825,12 +848,6 @@ vfs_getattr (const gchar *path, struct stat *sbuf) result = -ENOENT; } - if (fh) - { - g_mutex_unlock (fh->mutex); - file_handle_unref (fh); - } - debug_print ("vfs_getattr: -> %s\n", strerror (-result)); return result; @@ -954,7 +971,9 @@ vfs_open (const gchar *path, struct fuse_file_info *fi) if (file_type == G_FILE_TYPE_REGULAR) { FileHandle *fh = get_or_create_file_handle_for_path (path); - + + g_mutex_lock (fh->mutex); + /* File exists */ SET_FILE_HANDLE (fi, fh); @@ -963,8 +982,6 @@ vfs_open (const gchar *path, struct fuse_file_info *fi) /* Set up a stream here, so we can check for errors */ - g_mutex_lock (fh->mutex); - if (fi->flags & O_WRONLY || fi->flags & O_RDWR) result = setup_output_stream (file, fh); else @@ -1053,10 +1070,10 @@ vfs_create (const gchar *path, mode_t mode, struct fuse_file_info *fi) /* Success */ - SET_FILE_HANDLE (fi, fh); - g_mutex_lock (fh->mutex); + SET_FILE_HANDLE (fi, fh); + file_handle_close_stream (fh); fh->stream = file_output_stream; fh->op = FILE_OP_WRITE; @@ -1087,12 +1104,16 @@ vfs_create (const gchar *path, mode_t mode, struct fuse_file_info *fi) static gint vfs_release (const gchar *path, struct fuse_file_info *fi) { - FileHandle *fh = GET_FILE_HANDLE (fi); + FileHandle *fh = get_file_handle_from_info (fi); debug_print ("vfs_release: %s\n", path); if (fh) - file_handle_unref (fh); + { + /* get_file_handle_from_info () adds a "working ref", so unref twice. */ + file_handle_unref (fh); + file_handle_unref (fh); + } return 0; } @@ -1209,23 +1230,32 @@ vfs_read (const gchar *path, gchar *buf, size_t size, if ((file = file_from_full_path (path))) { - FileHandle *fh = GET_FILE_HANDLE (fi); + FileHandle *fh = get_file_handle_from_info (fi); - g_mutex_lock (fh->mutex); + if (fh) + { + g_mutex_lock (fh->mutex); - result = setup_input_stream (file, fh); + result = setup_input_stream (file, fh); - if (result == 0) - { - result = read_stream (fh, buf, size, offset); + if (result == 0) + { + result = read_stream (fh, buf, size, offset); + } + else + { + debug_print ("vfs_read: failed to setup input_stream!\n"); + } + + g_mutex_unlock (fh->mutex); + file_handle_unref (fh); } else { - debug_print ("vfs_read: failed to setup input_stream!\n"); + result = -EINVAL; } g_object_unref (file); - g_mutex_unlock (fh->mutex); } else { @@ -1328,18 +1358,27 @@ vfs_write (const gchar *path, const gchar *buf, size_t len, off_t offset, if ((file = file_from_full_path (path))) { - FileHandle *fh = GET_FILE_HANDLE (fi); + FileHandle *fh = get_file_handle_from_info (fi); - g_mutex_lock (fh->mutex); + if (fh) + { + g_mutex_lock (fh->mutex); - result = setup_output_stream (file, fh); - if (result == 0) + result = setup_output_stream (file, fh); + if (result == 0) + { + result = write_stream (fh, buf, len, offset); + } + + g_mutex_unlock (fh->mutex); + file_handle_unref (fh); + } + else { - result = write_stream (fh, buf, len, offset); + result = -EINVAL; } g_object_unref (file); - g_mutex_unlock (fh->mutex); } else { @@ -1497,9 +1536,7 @@ vfs_rename (const gchar *old_path, const gchar *new_path) if (old_file && new_file) { - FileHandle *fh; - - fh = get_file_handle_for_path (old_path); + FileHandle *fh = get_file_handle_for_path (old_path); if (fh) { @@ -1561,9 +1598,7 @@ vfs_unlink (const gchar *path) if (file) { - FileHandle *fh; - - fh = get_file_handle_for_path (path); + FileHandle *fh = get_file_handle_for_path (path); if (fh) { @@ -1706,40 +1741,49 @@ vfs_ftruncate (const gchar *path, off_t size, struct fuse_file_info *fi) if (file) { - FileHandle *fh = GET_FILE_HANDLE (fi); + FileHandle *fh = get_file_handle_from_info (fi); - g_mutex_lock (fh->mutex); + if (fh) + { + g_mutex_lock (fh->mutex); - result = setup_output_stream (file, fh); + result = setup_output_stream (file, fh); - if (result == 0) - { - if (g_seekable_can_truncate (G_SEEKABLE (fh->stream))) - { - g_seekable_truncate (fh->stream, size, NULL, &error); - } - else if (size == 0) + if (result == 0) { - g_output_stream_close (fh->stream, NULL, NULL); - g_object_unref (fh->stream); - fh->stream = NULL; + if (g_seekable_can_truncate (G_SEEKABLE (fh->stream))) + { + g_seekable_truncate (fh->stream, size, NULL, &error); + } + else if (size == 0) + { + g_output_stream_close (fh->stream, NULL, NULL); + g_object_unref (fh->stream); + fh->stream = NULL; - fh->stream = g_file_replace (file, 0, FALSE, 0, NULL, &error); - } - else - { - result = -ENOTSUP; - } + fh->stream = g_file_replace (file, 0, FALSE, 0, NULL, &error); + } + else + { + result = -ENOTSUP; + } - if (error) - { - result = -errno_from_error (error); - g_error_free (error); + if (error) + { + result = -errno_from_error (error); + g_error_free (error); + } } + + g_mutex_unlock (fh->mutex); + file_handle_unref (fh); + } + else + { + result = -EINVAL; } g_object_unref (file); - g_mutex_unlock (fh->mutex); } else { @@ -1767,6 +1811,7 @@ vfs_truncate (const gchar *path, off_t size) GFileOutputStream *file_output_stream = NULL; FileHandle *fh; + /* Get a file handle just to lock the path while we're working */ fh = get_file_handle_for_path (path); if (fh) g_mutex_lock (fh->mutex); @@ -2105,8 +2150,10 @@ vfs_init (struct fuse_conn_info *conn) daemon_gid = getgid (); mount_list_mutex = g_mutex_new (); - global_fh_table = g_hash_table_new_full (g_str_hash, g_str_equal, - NULL, (GDestroyNotify) file_handle_free); + global_path_to_fh_map = g_hash_table_new_full (g_str_hash, g_str_equal, + NULL, (GDestroyNotify) file_handle_free); + global_active_fh_map = g_hash_table_new_full (g_direct_hash, g_direct_equal, + NULL, NULL); dbus_error_init (&error); -- cgit v1.2.1