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

Proposed by Marco Trevisan (Treviño) on 2012-10-01
Status: Merged
Approved by: Michal Hruby on 2012-10-01
Approved revision: 491
Merged at revision: 485
Proposed branch: lp:~3v1n0/bamf/mime-types-fix-crash-1058260
Merge into: lp:bamf/0.4
Diff against target: 341 lines (+97/-61)
4 files modified
lib/libbamf/bamf-application.c (+2/-4)
src/bamf-application.c (+42/-57)
tests/bamfdaemon/data/mime-types.desktop (+10/-0)
tests/bamfdaemon/test-application.c (+43/-0)
To merge this branch: bzr merge lp:~3v1n0/bamf/mime-types-fix-crash-1058260
Reviewer Review Type Date Requested Status
Michal Hruby (community) 2012-10-01 Approve on 2012-10-01
Review via email: mp+127225@code.launchpad.net

Commit message

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

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

Description of the change

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

Tests added.

To post a comment you must log in.
Michal Hruby (mhr3) wrote :

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
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) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/libbamf/bamf-application.c'
2--- lib/libbamf/bamf-application.c 2012-08-21 22:02:12 +0000
3+++ lib/libbamf/bamf-application.c 2012-10-01 12:12:23 +0000
4@@ -69,7 +69,6 @@
5 gchar *desktop_file;
6 GList *cached_xids;
7 gchar **cached_mimes;
8- gboolean mimes_initialized;
9 int show_stubs;
10 };
11
12@@ -79,7 +78,7 @@
13 GError *error = NULL;
14 gchar **mimes = NULL;
15
16- if (application->priv->mimes_initialized)
17+ if (application->priv->cached_mimes)
18 return g_strdupv (application->priv->cached_mimes);
19
20 if (!_bamf_view_remote_ready (BAMF_VIEW (application)))
21@@ -97,7 +96,7 @@
22
23 return NULL;
24 }
25- application->priv->mimes_initialized = TRUE;
26+
27 application->priv->cached_mimes = g_strdupv (mimes);
28
29 return mimes;
30@@ -357,7 +356,6 @@
31 g_strfreev (self->priv->cached_mimes);
32
33 self->priv->cached_mimes = g_strdupv ((gchar**)mimes);
34- self->priv->mimes_initialized = TRUE;
35 }
36
37 static void
38
39=== modified file 'src/bamf-application.c'
40--- src/bamf-application.c 2012-09-14 19:04:14 +0000
41+++ src/bamf-application.c 2012-10-01 12:12:23 +0000
42@@ -44,11 +44,9 @@
43 char * app_type;
44 char * icon;
45 char * wmclass;
46+ char ** mimes;
47 gboolean is_tab_container;
48 gboolean show_stubs;
49-
50- gchar **mimes;
51- gboolean mimes_initialized;
52 };
53
54 enum {
55@@ -70,32 +68,28 @@
56
57 void
58 bamf_application_supported_mime_types_changed (BamfApplication *application,
59- const gchar **maybe_mimes)
60+ const gchar **new_mimes)
61 {
62- gchar **mimes;
63+ gchar **mimes = (gchar **) new_mimes;
64
65- if (!maybe_mimes)
66+ if (!new_mimes)
67 {
68 gchar *empty[] = {NULL};
69-
70 mimes = g_strdupv (empty);
71 }
72-
73- mimes = (gchar **)maybe_mimes;
74
75 g_signal_emit_by_name (application->priv->dbus_iface, "supported-mime-types-changed", mimes);
76- application->priv->mimes_initialized = TRUE;
77+
78+ if (!new_mimes)
79+ {
80+ g_strfreev (mimes);
81+ mimes = NULL;
82+ }
83
84 if (application->priv->mimes)
85 g_strfreev (application->priv->mimes);
86
87 application->priv->mimes = mimes;
88-
89- if (maybe_mimes == NULL)
90- {
91- g_strfreev (mimes);
92- }
93-
94 }
95
96 static gboolean
97@@ -116,21 +110,19 @@
98 GError *error = NULL;
99
100 g_key_file_load_from_file (key_file, desktop_file, (GKeyFileFlags) 0, &error);
101- application->priv->mimes_initialized = TRUE;
102
103 if (error)
104 {
105- g_key_file_free(key_file);
106+ g_key_file_free (key_file);
107 g_error_free (error);
108 return NULL;
109 }
110
111 char** mimes = g_key_file_get_string_list (key_file, "Desktop Entry", "MimeType", NULL, NULL);
112+ g_signal_emit (application, application_signals[SUPPORTED_MIMES_CHANGED], 0, mimes);
113
114 g_key_file_free (key_file);
115
116- g_signal_emit (application, application_signals[SUPPORTED_MIMES_CHANGED], 0, mimes);
117-
118 return mimes;
119 }
120
121@@ -139,14 +131,10 @@
122 {
123 g_return_val_if_fail (BAMF_IS_APPLICATION (application), NULL);
124
125- if (application->priv->mimes_initialized)
126+ if (application->priv->mimes)
127 return g_strdupv (application->priv->mimes);
128
129 gchar **mimes = BAMF_APPLICATION_GET_CLASS (application)->get_supported_mime_types (application);
130-
131- if (application->priv->mimes)
132- g_strfreev (application->priv->mimes);
133-
134 application->priv->mimes = mimes;
135
136 return g_strdupv (mimes);
137@@ -798,11 +786,11 @@
138 {
139 GVariant *out_variant;
140 BamfView *focusable_child;
141-
142+
143 out_variant = NULL;
144-
145+
146 focusable_child = bamf_application_get_focusable_child (self);
147-
148+
149 if (focusable_child == NULL)
150 {
151 out_variant = g_variant_new("(s)", "");
152@@ -810,9 +798,9 @@
153 else
154 {
155 const gchar *path;
156-
157+
158 path = bamf_view_get_path (BAMF_VIEW (focusable_child));
159-
160+
161 out_variant = g_variant_new("(s)", path);
162 }
163
164@@ -835,29 +823,26 @@
165
166 static gboolean
167 on_dbus_handle_supported_mime_types (BamfDBusItemApplication *interface,
168- GDBusMethodInvocation *invocation,
169- BamfApplication *self)
170+ GDBusMethodInvocation *invocation,
171+ BamfApplication *self)
172 {
173+ GVariant *list;
174+ GVariant *value;
175+
176 gchar **mimes = bamf_application_get_supported_mime_types (self);
177
178- GVariantBuilder *builder;
179- GVariant *value;
180- gchar **it;
181-
182- builder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
183 if (mimes)
184- for (it = mimes; *it; it++)
185- {
186- g_variant_builder_add (builder, "s", *it);
187- }
188- value = g_variant_new ("(as)", builder);
189-
190- g_dbus_method_invocation_return_value (invocation,
191- value);
192-
193- g_variant_builder_unref (builder);
194-
195- g_strfreev (mimes);
196+ {
197+ list = g_variant_new_strv ((const gchar**) mimes, -1);
198+ g_strfreev (mimes);
199+ }
200+ else
201+ {
202+ list = g_variant_new_strv (NULL, 0);
203+ }
204+
205+ value = g_variant_new ("(@as)", list);
206+ g_dbus_method_invocation_return_value (invocation, value);
207
208 return TRUE;
209 }
210@@ -868,15 +853,15 @@
211 BamfApplication *self)
212 {
213 gchar *name, *path;
214-
215+
216 bamf_application_get_application_menu (self, &name, &path);
217-
218+
219 name = name ? name : "";
220 path = path ? path : "";
221-
222+
223 g_dbus_method_invocation_return_value (invocation,
224 g_variant_new ("(ss)", name, path));
225-
226+
227 return TRUE;
228 }
229
230@@ -961,7 +946,7 @@
231 priv->app_type = g_strdup ("system");
232 priv->show_stubs = TRUE;
233 priv->wmclass = NULL;
234-
235+
236 /* Initializing the dbus interface */
237 priv->dbus_iface = bamf_dbus_item_application_skeleton_new ();
238
239@@ -1025,8 +1010,8 @@
240 klass->supported_mimes_changed = bamf_application_supported_mime_types_changed;
241
242 g_type_class_add_private (klass, sizeof (BamfApplicationPrivate));
243-
244- application_signals[SUPPORTED_MIMES_CHANGED] =
245+
246+ application_signals[SUPPORTED_MIMES_CHANGED] =
247 g_signal_new ("supported-mimes-changed",
248 G_OBJECT_CLASS_TYPE (klass),
249 G_SIGNAL_RUN_FIRST,
250@@ -1034,7 +1019,7 @@
251 NULL, NULL,
252 g_cclosure_marshal_generic,
253 G_TYPE_NONE, 1,
254- G_TYPE_STRV);
255+ G_TYPE_STRV);
256 }
257
258 BamfApplication *
259
260=== added file 'tests/bamfdaemon/data/mime-types.desktop'
261--- tests/bamfdaemon/data/mime-types.desktop 1970-01-01 00:00:00 +0000
262+++ tests/bamfdaemon/data/mime-types.desktop 2012-10-01 12:12:23 +0000
263@@ -0,0 +1,10 @@
264+#!/usr/bin/env xdg-open
265+
266+[Desktop Entry]
267+Name=Mime Text Editor
268+GenericName=Text Editor
269+Exec=mime-test-text-editor %F
270+Terminal=false
271+Type=Application
272+MimeType=text/plain;text/x-chdr;text/x-csrc;text/html;text/css;text/x-diff;application/xml;
273+Categories=TextEditor;Development;Utility;
274
275=== modified file 'tests/bamfdaemon/test-application.c'
276--- tests/bamfdaemon/test-application.c 2012-07-17 19:03:36 +0000
277+++ tests/bamfdaemon/test-application.c 2012-10-01 12:12:23 +0000
278@@ -31,6 +31,8 @@
279 static void test_allocation (void);
280 static void test_desktop_file (void);
281 static void test_desktop_no_icon (void);
282+static void test_get_mime_types (void);
283+static void test_get_mime_types_none (void);
284 static void test_urgent (void);
285 static void test_active (void);
286 static void test_get_xids (void);
287@@ -55,6 +57,8 @@
288 g_test_add_func (DOMAIN"/Allocation", test_allocation);
289 g_test_add_func (DOMAIN"/DesktopFile", test_desktop_file);
290 g_test_add_func (DOMAIN"/DesktopFile/NoIcon", test_desktop_no_icon);
291+ g_test_add_func (DOMAIN"/DesktopFile/MimeTypes/Valid", test_get_mime_types);
292+ g_test_add_func (DOMAIN"/DesktopFile/MimeTypes/None", test_get_mime_types_none);
293 g_test_add_func (DOMAIN"/ManagesXid", test_manages_xid);
294 g_test_add_func (DOMAIN"/Xids", test_get_xids);
295 g_test_add_func (DOMAIN"/Events/Active", test_active);
296@@ -117,6 +121,45 @@
297 }
298
299 static void
300+test_get_mime_types (void)
301+{
302+ BamfApplication *application;
303+ const char* mime_types_desktop = TESTDIR"/bamfdaemon/data/mime-types.desktop";
304+
305+ application = bamf_application_new_from_desktop_file (mime_types_desktop);
306+ g_assert_cmpstr (bamf_application_get_desktop_file (application), ==, mime_types_desktop);
307+
308+ gchar** mimes = bamf_application_get_supported_mime_types (application);
309+
310+ g_assert_cmpuint (g_strv_length (mimes), ==, 7);
311+ g_assert_cmpstr (mimes[0], ==, "text/plain");
312+ g_assert_cmpstr (mimes[1], ==, "text/x-chdr");
313+ g_assert_cmpstr (mimes[2], ==, "text/x-csrc");
314+ g_assert_cmpstr (mimes[3], ==, "text/html");
315+ g_assert_cmpstr (mimes[4], ==, "text/css");
316+ g_assert_cmpstr (mimes[5], ==, "text/x-diff");
317+ g_assert_cmpstr (mimes[6], ==, "application/xml");
318+
319+ g_strfreev (mimes);
320+ g_object_unref (application);
321+}
322+
323+static void
324+test_get_mime_types_none (void)
325+{
326+ BamfApplication *application;
327+ const char* mime_types_desktop = TESTDIR"/bamfdaemon/data/test-bamf-app.desktop";
328+
329+ application = bamf_application_new_from_desktop_file (mime_types_desktop);
330+ g_assert_cmpstr (bamf_application_get_desktop_file (application), ==, mime_types_desktop);
331+
332+ gchar** mimes = bamf_application_get_supported_mime_types (application);
333+ g_assert (!mimes);
334+
335+ g_object_unref (application);
336+}
337+
338+static void
339 on_urgent_changed (BamfApplication *application, gboolean result, gpointer data)
340 {
341 signal_seen = TRUE;

Subscribers

People subscribed via source and target branches