Merge lp:~3v1n0/bamf/mime-types-fix-crash-1058260 into lp:bamf/0.4

[Marco Trevisan (Treviño)]

Daemon, BamfApplication: fix a crash on getting the supported mimes

Also remove the unneeded mimes_initialized, just use the pointer value and add tests.

Fix the function bamf_application_default_get_supported_mime_types that was leading to a crash in default implementation.

Tests added.

[Michal Hruby (mhr3)]

66 + if (!new_mimes)
67 {
68 gchar *empty[] = {NULL};
69 -
70 - mimes = g_strdupv (empty);
71 + mimes = empty;
72 }

Ouch, looks pretty dangerous, you're taking address of a temporary that's not valid outside of that block.

181 + for (it = mimes; *it; it++)
182 + {
183 + g_variant_builder_add (&b, "s", *it);
184 + }

Why so complicated? How about g_variant_new_strv()? (or perhaps `g_variant_new("(@as)", g_variant_new_strv(...));`)

review: Needs Fixing

[lp:~3v1n0/bamf/mime-types-fix-crash-1058260]

487. By Marco Trevisan (Treviño) on 2012-10-01

Daemon, tests: add one more mime types test (for invalid mimes)

488. By Marco Trevisan (Treviño) on 2012-10-01

Daemon, BamfApplication: be safer on managing empty mimes

489. By Marco Trevisan (Treviño) on 2012-10-01

BamfApplication: use g_variant_new_strv for building string arrays

490. By Marco Trevisan (Treviño) on 2012-10-01

Tests, Daemon: application code cleanup

491. By Marco Trevisan (Treviño) on 2012-10-01

Daemon, BamfApplication: no need to have an empty array... Just pass null to g_variant_new_strv

[Michal Hruby (mhr3)]

LGTM

review: Approve

=== modified file 'lib/libbamf/bamf-application.c'
--- lib/libbamf/bamf-application.c	2012-08-21 22:02:12 +0000
+++ lib/libbamf/bamf-application.c	2012-10-01 12:12:23 +0000
@@ -69,7 +69,6 @@
   gchar           *desktop_file;
   GList           *cached_xids;
   gchar           **cached_mimes;
-  gboolean        mimes_initialized;
   int             show_stubs;
 };
 
@@ -79,7 +78,7 @@
   GError *error = NULL;
   gchar **mimes = NULL;
 
-  if (application->priv->mimes_initialized)
+  if (application->priv->cached_mimes)
     return g_strdupv (application->priv->cached_mimes);
 
   if (!_bamf_view_remote_ready (BAMF_VIEW (application)))
@@ -97,7 +96,7 @@
 
       return NULL;
     }
-  application->priv->mimes_initialized = TRUE;
+
   application->priv->cached_mimes = g_strdupv (mimes);
 
   return mimes;
@@ -357,7 +356,6 @@
     g_strfreev (self->priv->cached_mimes);
 
   self->priv->cached_mimes = g_strdupv ((gchar**)mimes);
-  self->priv->mimes_initialized = TRUE;
 }
 
 static void

=== modified file 'src/bamf-application.c'
--- src/bamf-application.c	2012-09-14 19:04:14 +0000
+++ src/bamf-application.c	2012-10-01 12:12:23 +0000
@@ -44,11 +44,9 @@
   char * app_type;
   char * icon;
   char * wmclass;
+  char ** mimes;
   gboolean is_tab_container;
   gboolean show_stubs;
-
-  gchar **mimes;
-  gboolean mimes_initialized;
 };
 
 enum {
@@ -70,32 +68,28 @@
 
 void
 bamf_application_supported_mime_types_changed (BamfApplication *application,
-                                               const gchar **maybe_mimes)
+                                               const gchar **new_mimes)
 {
-  gchar **mimes;
+  gchar **mimes = (gchar **) new_mimes;
 
-  if (!maybe_mimes)
+  if (!new_mimes)
     {
       gchar *empty[] = {NULL};
-
       mimes = g_strdupv (empty);
     }
-  
-  mimes = (gchar **)maybe_mimes;
 
   g_signal_emit_by_name (application->priv->dbus_iface, "supported-mime-types-changed", mimes);
-  application->priv->mimes_initialized = TRUE;
+
+  if (!new_mimes)
+  {
+    g_strfreev (mimes);
+    mimes = NULL;
+  }
 
   if (application->priv->mimes)
     g_strfreev (application->priv->mimes);
 
   application->priv->mimes = mimes;
-  
-  if (maybe_mimes == NULL)
-    {
-      g_strfreev (mimes);
-    }
-
 }
 
 static gboolean
@@ -116,21 +110,19 @@
   GError *error = NULL;
 
   g_key_file_load_from_file (key_file, desktop_file, (GKeyFileFlags) 0, &error);
-  application->priv->mimes_initialized = TRUE;
 
   if (error)
     {
-      g_key_file_free(key_file);
+      g_key_file_free (key_file);
       g_error_free (error);
       return NULL;
     }
 
   char** mimes = g_key_file_get_string_list (key_file, "Desktop Entry", "MimeType", NULL, NULL);
+  g_signal_emit (application, application_signals[SUPPORTED_MIMES_CHANGED], 0, mimes);
 
   g_key_file_free (key_file);
 
-  g_signal_emit (application, application_signals[SUPPORTED_MIMES_CHANGED], 0, mimes);
-
   return mimes;
 }
 
@@ -139,14 +131,10 @@
 {
   g_return_val_if_fail (BAMF_IS_APPLICATION (application), NULL);
 
-  if (application->priv->mimes_initialized)
+  if (application->priv->mimes)
     return g_strdupv (application->priv->mimes);
 
   gchar **mimes = BAMF_APPLICATION_GET_CLASS (application)->get_supported_mime_types (application);
-
-  if (application->priv->mimes)
-    g_strfreev (application->priv->mimes);
-
   application->priv->mimes = mimes;
 
   return g_strdupv (mimes);
@@ -798,11 +786,11 @@
 {
   GVariant *out_variant;
   BamfView *focusable_child;
-  
+
   out_variant = NULL;
-  
+
   focusable_child = bamf_application_get_focusable_child (self);
-  
+
   if (focusable_child == NULL)
     {
       out_variant = g_variant_new("(s)", "");
@@ -810,9 +798,9 @@
   else
     {
       const gchar *path;
-      
+
       path = bamf_view_get_path (BAMF_VIEW (focusable_child));
-      
+
       out_variant = g_variant_new("(s)", path);
     }
 
@@ -835,29 +823,26 @@
 
 static gboolean
 on_dbus_handle_supported_mime_types (BamfDBusItemApplication *interface,
-                          GDBusMethodInvocation *invocation,
-                          BamfApplication *self)
+                                     GDBusMethodInvocation *invocation,
+                                     BamfApplication *self)
 {
+  GVariant *list;
+  GVariant *value;
+
   gchar **mimes = bamf_application_get_supported_mime_types (self);
 
-  GVariantBuilder *builder;
-  GVariant *value;
-  gchar **it;
-
-  builder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
   if (mimes)
-    for (it = mimes; *it; it++)
-      {
-        g_variant_builder_add (builder, "s", *it);
-      }
-  value = g_variant_new ("(as)", builder);
-
-  g_dbus_method_invocation_return_value (invocation,
-                                         value);
-
-  g_variant_builder_unref (builder);
-
-  g_strfreev (mimes);
+    {
+      list = g_variant_new_strv ((const gchar**) mimes, -1);
+      g_strfreev (mimes);
+    }
+  else
+    {
+      list = g_variant_new_strv (NULL, 0);
+    }
+
+  value = g_variant_new ("(@as)", list);
+  g_dbus_method_invocation_return_value (invocation, value);
 
   return TRUE;
 }
@@ -868,15 +853,15 @@
                                  BamfApplication *self)
 {
   gchar *name, *path;
-  
+
   bamf_application_get_application_menu (self, &name, &path);
-  
+
   name = name ? name : "";
   path = path ? path : "";
-  
+
   g_dbus_method_invocation_return_value (invocation,
                                          g_variant_new ("(ss)", name, path));
-  
+
   return TRUE;
 }
 
@@ -961,7 +946,7 @@
   priv->app_type = g_strdup ("system");
   priv->show_stubs = TRUE;
   priv->wmclass = NULL;
-  
+
   /* Initializing the dbus interface */
   priv->dbus_iface = bamf_dbus_item_application_skeleton_new ();
 
@@ -1025,8 +1010,8 @@
   klass->supported_mimes_changed = bamf_application_supported_mime_types_changed;
 
   g_type_class_add_private (klass, sizeof (BamfApplicationPrivate));
-  
-  application_signals[SUPPORTED_MIMES_CHANGED] = 
+
+  application_signals[SUPPORTED_MIMES_CHANGED] =
     g_signal_new ("supported-mimes-changed",
                   G_OBJECT_CLASS_TYPE (klass),
                   G_SIGNAL_RUN_FIRST,
@@ -1034,7 +1019,7 @@
                   NULL, NULL,
                   g_cclosure_marshal_generic,
                   G_TYPE_NONE, 1,
-                  G_TYPE_STRV);           
+                  G_TYPE_STRV);
 }
 
 BamfApplication *

=== added file 'tests/bamfdaemon/data/mime-types.desktop'
--- tests/bamfdaemon/data/mime-types.desktop	1970-01-01 00:00:00 +0000
+++ tests/bamfdaemon/data/mime-types.desktop	2012-10-01 12:12:23 +0000
@@ -0,0 +1,10 @@
+#!/usr/bin/env xdg-open
+
+[Desktop Entry]
+Name=Mime Text Editor
+GenericName=Text Editor
+Exec=mime-test-text-editor %F
+Terminal=false
+Type=Application
+MimeType=text/plain;text/x-chdr;text/x-csrc;text/html;text/css;text/x-diff;application/xml;
+Categories=TextEditor;Development;Utility;

=== modified file 'tests/bamfdaemon/test-application.c'
--- tests/bamfdaemon/test-application.c	2012-07-17 19:03:36 +0000
+++ tests/bamfdaemon/test-application.c	2012-10-01 12:12:23 +0000
@@ -31,6 +31,8 @@
 static void test_allocation          (void);
 static void test_desktop_file        (void);
 static void test_desktop_no_icon     (void);
+static void test_get_mime_types      (void);
+static void test_get_mime_types_none (void);
 static void test_urgent              (void);
 static void test_active              (void);
 static void test_get_xids            (void);
@@ -55,6 +57,8 @@
   g_test_add_func (DOMAIN"/Allocation", test_allocation);
   g_test_add_func (DOMAIN"/DesktopFile", test_desktop_file);
   g_test_add_func (DOMAIN"/DesktopFile/NoIcon", test_desktop_no_icon);
+  g_test_add_func (DOMAIN"/DesktopFile/MimeTypes/Valid", test_get_mime_types);
+  g_test_add_func (DOMAIN"/DesktopFile/MimeTypes/None", test_get_mime_types_none);
   g_test_add_func (DOMAIN"/ManagesXid", test_manages_xid);
   g_test_add_func (DOMAIN"/Xids", test_get_xids);
   g_test_add_func (DOMAIN"/Events/Active", test_active);
@@ -117,6 +121,45 @@
 }
 
 static void
+test_get_mime_types (void)
+{
+  BamfApplication *application;
+  const char* mime_types_desktop = TESTDIR"/bamfdaemon/data/mime-types.desktop";
+
+  application = bamf_application_new_from_desktop_file (mime_types_desktop);
+  g_assert_cmpstr (bamf_application_get_desktop_file (application), ==, mime_types_desktop);
+
+  gchar** mimes = bamf_application_get_supported_mime_types (application);
+
+  g_assert_cmpuint (g_strv_length (mimes), ==, 7);
+  g_assert_cmpstr (mimes[0], ==, "text/plain");
+  g_assert_cmpstr (mimes[1], ==, "text/x-chdr");
+  g_assert_cmpstr (mimes[2], ==, "text/x-csrc");
+  g_assert_cmpstr (mimes[3], ==, "text/html");
+  g_assert_cmpstr (mimes[4], ==, "text/css");
+  g_assert_cmpstr (mimes[5], ==, "text/x-diff");
+  g_assert_cmpstr (mimes[6], ==, "application/xml");
+
+  g_strfreev (mimes);
+  g_object_unref (application);
+}
+
+static void
+test_get_mime_types_none (void)
+{
+  BamfApplication *application;
+  const char* mime_types_desktop = TESTDIR"/bamfdaemon/data/test-bamf-app.desktop";
+
+  application = bamf_application_new_from_desktop_file (mime_types_desktop);
+  g_assert_cmpstr (bamf_application_get_desktop_file (application), ==, mime_types_desktop);
+
+  gchar** mimes = bamf_application_get_supported_mime_types (application);
+  g_assert (!mimes);
+
+  g_object_unref (application);
+}
+
+static void
 on_urgent_changed (BamfApplication *application, gboolean result, gpointer data)
 {
   signal_seen = TRUE;