[gvfs] mtp: Fix crashes when `LIBMTP_devicestorage_t` `StorageDescription = NULL`.



commit 0cdd813fc2ea63518189e72e647fc492db382eab
Author: Niklas Hambüchen <mail nh2 me>
Date:   Sun Nov 15 18:22:07 2020 +0100

    mtp: Fix crashes when `LIBMTP_devicestorage_t` `StorageDescription = NULL`.
    
    The MTP spec section 5.2.2.7 allows `StorageDescription` to be
    an empty string.
    
    `libmtp` currently translates this to
    
        char * StorageDescription = NULL
    
    instead of `""` in `ptp_unpack_SI()` via `ptp_unpack_string()`.
    (I'm not sure if it's good that it does that, to be followed up on separately.)
    
    `create_storage_name()` until now returned
    `g_strdup(storage->StorageDescription)`, which returns `NULL` if `NULL` is
    given, and thus `get_storage_info()` would eventually call
    
        char *storage_name = NULL = create_storage_name(storage);
        g_file_info_set_name (info, storage_name = NULL);
        g_file_info_set_display_name (info, storage_name = NULL);
    
    resulting in assertion failures in `gvfsd`:
    
        g_file_info_set_name: assertion 'name != NULL' failed
        g_file_info_set_display_name: assertion 'display_name != NULL' failed
    
    as well as crashes in file managers like Thunar:
    
        g_file_get_child: assertion 'name != NULL' failed
    
    and warnings in Nautilus like:
    
        Got GFileInfo with NULL name in mtp://Ricoh_Company__Ltd._RICOH_THETA_V_00165759/, ignoring. This 
shouldn't happen unless the gvfs backend is broken.
    
    This commit fixes it by adding a contract to `create_storage_name()`
    that it will never represent empty strings as NULL.

 daemon/gvfsbackendmtp.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)
---
diff --git a/daemon/gvfsbackendmtp.c b/daemon/gvfsbackendmtp.c
index d3c6daf2..176b3528 100644
--- a/daemon/gvfsbackendmtp.c
+++ b/daemon/gvfsbackendmtp.c
@@ -132,6 +132,18 @@ handle_event (EventData *data, GVfsBackendMtp *backend);
  * Storage name helper
  ************************************************/
 
+/**
+ * create_storage_name:
+ *
+ * Returns a unique, printable storage name for a LIBMTP_devicestorage_t
+ * based on its StorageDescription, appending the storage ID if necessary
+ * to make it unique.
+ *
+ * The caller takes ownership of the returned string.
+ * This function never returns NULL strings.
+ *
+ * The passed-in `storage->StorageDescription` may be NULL.
+ */
 static char *create_storage_name (const LIBMTP_devicestorage_t *storage)
 {
   /* The optional post-fixing of storage's name with ID requires us to
@@ -145,6 +157,9 @@ static char *create_storage_name (const LIBMTP_devicestorage_t *storage)
   gboolean is_unique = TRUE;
   const LIBMTP_devicestorage_t *tmp_storage;
 
+  /* `storage->StorageDescription` may be NULL, so we ensure to only use
+     functions that can handle this, like `g_strcmp0()`. */
+
   /* Forward search for duplicates */
   for (tmp_storage = storage->next; tmp_storage != 0; tmp_storage = tmp_storage->next) {
     if (!g_strcmp0 (storage->StorageDescription, tmp_storage->StorageDescription)) {
@@ -167,7 +182,17 @@ static char *create_storage_name (const LIBMTP_devicestorage_t *storage)
   /* If description is unique, we can use it as storage name; otherwise,
      we add storage ID to it */
   if (is_unique) {
-    return g_strdup (storage->StorageDescription);
+    /* Never return a NULL string (`g_strdup` returns NULL on NULL).
+       Use the storage ID on empty strings to avoid duplicate entries
+       for devices with multiple storages without description. */
+    if (storage->StorageDescription && strlen (storage->StorageDescription) > 0) {
+      return g_strdup (storage->StorageDescription);
+    } else {
+      /* Translators: This is shown as the name for MTP devices
+       *              without StorageDescription.
+       *              The %X is the formatted storage ID. */
+      return g_strdup_printf (_("Storage (%X)"), storage->id);
+    }
   } else {
     return g_strdup_printf ("%s (%X)", storage->StorageDescription, storage->id);
   }


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