After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 753311 - Crashes when data connection setup failed
Crashes when data connection setup failed
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: sftp backend
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2015-08-06 09:11 UTC by Ondrej Holy
Modified: 2015-09-23 08:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
sftp: Fix crashes when data connection setup failed (3.01 KB, patch)
2015-08-06 09:11 UTC, Ondrej Holy
none Details | Review
sftp: Fix crashes when data connection setup failed (3.06 KB, patch)
2015-08-10 07:22 UTC, Ondrej Holy
accepted-commit_now Details | Review
sftp: Don't call g_vfs_backend_force_unmount multiple times (1.56 KB, patch)
2015-09-05 12:42 UTC, Ross Lagerwall
none Details | Review

Description Ondrej Holy 2015-08-06 09:11:43 UTC
Created attachment 308840 [details] [review]
sftp: Fix crashes when data connection setup failed

destroy_connection() is called twice when setup of data connection failed.
Consequently g_hash_table_destroy() is called on already destroyed hash
table on finalize. Set conn->expected_replies to NULL in destroy_connection()
to fix the issue.

There is another crash on force unmount when setup of data connection failed.
fail_jobs() is called on already destroyed connection. Check usability of
the connection before use to avoid the crash.

Finally move the hast table initialization into the setup_connection,
because the initialization in g_vfs_backend_sftp_init() is confusing.
Comment 1 Ross Lagerwall 2015-08-10 06:20:45 UTC
Review of attachment 308840 [details] [review]:

::: daemon/gvfsbackendsftp.c
@@ +1315,3 @@
   gpointer key, value;
 
+  g_return_if_fail (connection_is_usable (conn));

According to the docs, g_return_val_if_fail is only supposed to be used to guard against programmer errors, whereas in this case it is expected that fail_jobs may be called with an unsable connection.
Comment 2 Ondrej Holy 2015-08-10 07:22:10 UTC
Created attachment 308997 [details] [review]
sftp: Fix crashes when data connection setup failed

Damn, fixed, thanks for review!
Comment 3 Ross Lagerwall 2015-09-05 12:42:31 UTC
Review of attachment 308997 [details] [review]:

Looks good!
Comment 4 Ross Lagerwall 2015-09-05 12:42:51 UTC
Created attachment 310691 [details] [review]
sftp: Don't call g_vfs_backend_force_unmount multiple times

If the remote connection(s) are closed (e.g. the remote host is
rebooted), ensure that g_vfs_backend_force_unmount is only called once.

This avoids a warning like the following:
forced_unregister_mount_callback
forced_unregister_mount_callback

** (process:12472): WARNING **: Error unregistering mount: Mountpoint
not registered (g-io-error-quark, 16)

Additionally, remove some backend member variables.
Comment 5 Ross Lagerwall 2015-09-05 12:43:16 UTC
The above patch fixes a warning that I noticed while testing.
Comment 6 Ondrej Holy 2015-09-23 08:20:00 UTC
Thanks for the patch, looks good, and sorry, I forgot on this bug. I've pushed the patches as commit 46487b4 and commit e1a3c29 .