Merge lp:~ted/ubuntu-app-launch/uri-splitting into lp:ubuntu-app-launch/14.04

[Ted Gould]

Ensure quoted single URIs passed to a %U are unquoted

Fixes the problem (see test) but also takes the time to ensure that all URIs are separate arguments and fixes the tests in response to that. Overall this makes us cleaner and gets rid of some crufty code.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:92
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/157/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-armhf-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-trusty-i386-ci/16

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/upstart-app-launch-ci/157/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

Looks good!

review: Approve

=== modified file 'helpers.c'
--- helpers.c	2013-11-18 21:20:41 +0000
+++ helpers.c	2013-11-22 16:42:53 +0000
@@ -273,121 +273,61 @@
 	return retval;
 }
 
-/* free a string in an array */
+/* Put the list of files into the argument array */
+static inline void
+file_list_handling (GArray * outarray, gchar ** list, gchar * (*dup_func) (const gchar * in))
+{
+	/* No URLs, cool, this is a noop */
+	if (list == NULL || list[0] == NULL) {
+		return;
+	}
+
+	int i;
+	for (i = 0; list[i] != NULL; i++) {
+		gchar * entry = dup_func(list[i]);
+
+		/* No NULLs */
+		if (entry != NULL && entry[0] != '\0') {
+			g_array_append_val(outarray, entry);
+		} else {
+			g_free(entry);
+		}
+	}
+}
+
+/* Parse a desktop exec line and return the next string */
 static void
-free_string (gpointer value)
-{
-	gchar ** str = (gchar **)value;
-	g_free(*str);
-	return;
-}
-
-/* Builds the file list from the URI list */
-static gchar *
-build_file_list (const gchar * uri_list)
-{
-	gchar ** uri_split = NULL;
-	if (!g_shell_parse_argv(uri_list, NULL, &uri_split, NULL)) {
-		return g_strdup("");
-	}
-
-	GArray * outarray = g_array_new(TRUE, FALSE, sizeof(gchar *));
-	g_array_set_clear_func(outarray, free_string);
-
-	int i;
-	for (i = 0; uri_split[i] != NULL; i++) {
-		gchar * path = uri2file(uri_split[i]);
-		g_array_append_val(outarray, path);
-	}
-
-	gchar * filelist = g_strjoinv(" ", (gchar **)outarray->data);
-	g_array_free(outarray, TRUE);
-
-	g_strfreev(uri_split);
-
-	return filelist;
-}
-
-/* Make sure we have the single URI variable */
-static inline void
-ensure_singleuri (gchar ** single_uri, const gchar * uri_list)
-{
-	if (uri_list == NULL) {
-		return;
-	}
-
-	if (*single_uri != NULL) {
-		return;
-	}
-
-	gchar ** uri_split = NULL;
-	if (!g_shell_parse_argv(uri_list, NULL, &uri_split, NULL)) {
-		return;
-	}
-
-	if (uri_split[0] != NULL) {
-		*single_uri = g_strdup(uri_split[0]);
-	}
-
-	g_strfreev(uri_split);
-
-	return;
-}
-
-/* Make sure we have a single file variable */
-static inline void
-ensure_singlefile (gchar ** single_file, const gchar * uri_list)
-{
-	if (uri_list == NULL) {
-		return;
-	}
-
-	if (*single_file != NULL) {
-		return;
-	}
-
-	gchar ** uri_split = NULL;
-	if (!g_shell_parse_argv(uri_list, NULL, &uri_split, NULL)) {
-		return;
-	}
-
-	gchar * first_file = NULL;
-	if (uri_split[0] != NULL) {
-		first_file = uri2file(uri_split[0]);
-	}
-
-	g_strfreev(uri_split);
-
-	if (first_file != NULL) {
-		*single_file = first_file;
-	}
-
-	return;
-}
-
-/* Parse a desktop exec line and return the next string */
-static gchar *
-desktop_exec_segment_parse (const gchar * execline, const gchar * uri_list)
-{
-	gchar ** execsplit = g_strsplit(execline, "%", 0);
+desktop_exec_segment_parse (GArray * finalarray, const gchar * execsegment, gchar ** uri_list)
+{
+	/* No NULL strings */
+	if (execsegment == NULL || execsegment[0] == '\0')
+		return;
+
+	/* Handle %F and %U as an argument on their own as per the spec */
+	if (g_strcmp0(execsegment, "%U") == 0) {
+		return file_list_handling(finalarray, uri_list, g_strdup);
+	}
+	if (g_strcmp0(execsegment, "%F") == 0) {
+		return file_list_handling(finalarray, uri_list, uri2file);
+	}
+
+	/* Start looking at individual codes */
+	gchar ** execsplit = g_strsplit(execsegment, "%", 0);
 
 	/* If we didn't have any codes, just exit here */
 	if (execsplit[1] == NULL) {
 		g_strfreev(execsplit);
-		return g_strdup(execline);
-	}
-
-	if (uri_list != NULL && uri_list[0] == '\0') {
-		uri_list = NULL;
+		gchar * dup = g_strdup(execsegment);
+		g_array_append_val(finalarray, dup);
+		return;
 	}
 
 	int i;
-	gchar * single_uri = NULL;
-	gchar * single_file = NULL;
-	gchar * file_list = NULL;
+
 	gboolean previous_percent = FALSE;
 	GArray * outarray = g_array_new(TRUE, FALSE, sizeof(const gchar *));
 	g_array_append_val(outarray, execsplit[0]);
+	gchar * single_file = NULL;
 
 	/* The variables allowed in an exec line from the Freedesktop.org Desktop
 	   File specification: http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#exec-variables */
@@ -418,22 +358,16 @@
 			g_array_append_val(outarray, skipchar);
 			break;
 		case 'f':
-			ensure_singlefile(&single_file, uri_list);
-
-			if (single_file != NULL) {
+			if (uri_list != NULL && uri_list[0] != NULL) {
+				if (single_file == NULL)
+					single_file = uri2file(uri_list[0]);
 				g_array_append_val(outarray, single_file);
 			}
 
 			g_array_append_val(outarray, skipchar);
 			break;
 		case 'F':
-			if (uri_list != NULL) {
-				if (file_list == NULL) {
-					file_list = build_file_list(uri_list);
-				}
-				g_array_append_val(outarray, file_list);
-			}
-
+			g_warning("Exec line segment has a '%%F' that isn't its own argument '%s', ignoring.", execsegment);
 			g_array_append_val(outarray, skipchar);
 			break;
 		case 'i':
@@ -443,16 +377,12 @@
 			g_array_append_val(outarray, skipchar);
 			break;
 		case 'U':
-			if (uri_list != NULL) {
-				g_array_append_val(outarray, uri_list);
-			}
+			g_warning("Exec line segment has a '%%U' that isn't its own argument '%s', ignoring.", execsegment);
 			g_array_append_val(outarray, skipchar);
 			break;
 		case 'u':
-			ensure_singleuri(&single_uri, uri_list);
-
-			if (single_uri != NULL) {
-				g_array_append_val(outarray, single_uri);
+			if (uri_list != NULL && uri_list[0] != NULL) {
+				g_array_append_val(outarray, uri_list[0]);
 			}
 
 			g_array_append_val(outarray, skipchar);
@@ -467,12 +397,14 @@
 	gchar * output = g_strjoinv(NULL, (gchar **)outarray->data);
 	g_array_free(outarray, TRUE);
 
-	g_free(single_uri);
+	if (output != NULL && output[0] != '\0') {
+		g_array_append_val(finalarray, output);
+	} else {
+		g_free(output);
+	}
+
 	g_free(single_file);
-	g_free(file_list);
 	g_strfreev(execsplit);
-
-	return output;
 }
 
 /* Take a full exec line, split it out, parse the segments and return
@@ -482,6 +414,7 @@
 {
 	GError * error = NULL;
 	gchar ** splitexec = NULL;
+	gchar ** splituris = NULL;
 	gint execitems = 0;
 
 	/* This returns from desktop file style quoting to straight strings with
@@ -496,14 +429,29 @@
 		return NULL;
 	}
 
+	if (urilist != NULL && urilist[0] != '\0') {
+		g_shell_parse_argv(urilist, NULL, &splituris, &error);
+
+		if (error != NULL) {
+			g_warning("Unable to parse URIs '%s': %s", urilist, error->message);
+			g_error_free(error);
+			/* Continuing without URIs */
+			splituris = NULL;
+		}
+	}
+
+
 	GArray * newargv = g_array_new(TRUE, FALSE, sizeof(gchar *));
 	int i;
 	for (i = 0; i < execitems; i++) {
-		gchar * execinserted = desktop_exec_segment_parse(splitexec[i], urilist);
-		g_array_append_val(newargv, execinserted);
+		desktop_exec_segment_parse(newargv, splitexec[i], splituris);
 	}
 	g_strfreev(splitexec);
 
+	if (splituris != NULL) {
+		g_strfreev(splituris);
+	}
+
 	/* Each string here should be its own param */
 
 	return newargv;
 
=== modified file 'tests/helper-test.cc'
--- tests/helper-test.cc	2013-11-18 21:20:41 +0000
+++ tests/helper-test.cc	2013-11-22 16:42:53 +0000
@@ -84,9 +84,8 @@
 
 	/* Little u with a NULL string */
 	output = desktop_exec_parse("foo %u", "");
-	ASSERT_EQ(output->len, 2);
+	ASSERT_EQ(output->len, 1);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "");
 	g_array_free(output, TRUE);
 
 	/* Big %U with a single URL */
@@ -121,9 +120,8 @@
 
 	/* URL is a quote, make sure we handle the error */
 	output = desktop_exec_parse("foo %u", "\"");
-	ASSERT_EQ(output->len, 2);
+	ASSERT_EQ(output->len, 1);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "");
 	g_array_free(output, TRUE);
 
 	/* Lots of quotes, escaped and not */
@@ -141,16 +139,23 @@
 
 	/* Big U with two URLs */
 	output = desktop_exec_parse("foo %U", "http://ubuntu.com http://slashdot.org");
-	ASSERT_EQ(output->len, 2);
+	ASSERT_EQ(output->len, 3);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "http://ubuntu.com http://slashdot.org");
+	ASSERT_STREQ(g_array_index(output, gchar *, 1), "http://ubuntu.com");
+	ASSERT_STREQ(g_array_index(output, gchar *, 2), "http://slashdot.org");
 	g_array_free(output, TRUE);
 
 	/* Big U with no URLs */
 	output = desktop_exec_parse("foo %U", NULL);
+	ASSERT_EQ(output->len, 1);
+	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
+	g_array_free(output, TRUE);
+
+	/* Big U with quoted URL */
+	output = desktop_exec_parse("foo %U", "'http://ubuntu.com'");
 	ASSERT_EQ(output->len, 2);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "");
+	ASSERT_STREQ(g_array_index(output, gchar *, 1), "http://ubuntu.com");
 	g_array_free(output, TRUE);
 
 	/* Big U with URLs that have spaces */
@@ -169,16 +174,14 @@
 
 	/* A %f with a NULL string */
 	output = desktop_exec_parse("foo %f", "");
-	ASSERT_EQ(output->len, 2);
+	ASSERT_EQ(output->len, 1);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "");
 	g_array_free(output, TRUE);
 
 	/* %f with a URL that isn't a file */
 	output = desktop_exec_parse("foo %f", "torrent://moviephone.com/hot-new-movie");
-	ASSERT_EQ(output->len, 2);
+	ASSERT_EQ(output->len, 1);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "");
 	g_array_free(output, TRUE);
 
 	/* Lots of %f combinations */
@@ -200,16 +203,16 @@
 
 	/* Big F with two files */
 	output = desktop_exec_parse("foo %F", "file:///proc/version file:///proc/uptime");
-	ASSERT_EQ(output->len, 2);
+	ASSERT_EQ(output->len, 3);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "/proc/version /proc/uptime");
+	ASSERT_STREQ(g_array_index(output, gchar *, 1), "/proc/version");
+	ASSERT_STREQ(g_array_index(output, gchar *, 2), "/proc/uptime");
 	g_array_free(output, TRUE);
 
 	/* Big F with no files */
 	output = desktop_exec_parse("foo %F", NULL);
-	ASSERT_EQ(output->len, 2);
+	ASSERT_EQ(output->len, 1);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "");
 	g_array_free(output, TRUE);
 
 	/* Groups of percents */
@@ -223,17 +226,8 @@
 
 	/* All the % sequences we don't support */
 	output = desktop_exec_parse("foo %d %D %n %N %v %m %i %c %k", "file:///proc/version");
-	ASSERT_EQ(output->len, 10);
+	ASSERT_EQ(output->len, 1);
 	ASSERT_STREQ(g_array_index(output, gchar *, 0), "foo");
-	ASSERT_STREQ(g_array_index(output, gchar *, 1), "");
-	ASSERT_STREQ(g_array_index(output, gchar *, 2), "");
-	ASSERT_STREQ(g_array_index(output, gchar *, 3), "");
-	ASSERT_STREQ(g_array_index(output, gchar *, 4), "");
-	ASSERT_STREQ(g_array_index(output, gchar *, 5), "");
-	ASSERT_STREQ(g_array_index(output, gchar *, 6), "");
-	ASSERT_STREQ(g_array_index(output, gchar *, 7), "");
-	ASSERT_STREQ(g_array_index(output, gchar *, 8), "");
-	ASSERT_STREQ(g_array_index(output, gchar *, 9), "");
 	g_array_free(output, TRUE);
 
 	return;