From 74f60c99446ad592b13119da8c79ccd19256b715 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Mon, 20 Jul 2026 15:56:16 +0300 Subject: opener: shell-argv command parsing with %f as one safe arg (hu0) _expand_command now parses the command template with g_shell_parse_argv FIRST, then substitutes %f into the already-parsed argv elements, so %f is always exactly one argv value regardless of spaces or shell metacharacters in the path. Previously it split on literal spaces, breaking quoted commands like sh -c '...' and mis-splitting paths with spaces. opener_launch propagates parse errors (returns FALSE, sets GError) instead of launching malformed argv. A non-local GFile (no path) is now rejected with G_IO_ERROR_NOT_SUPPORTED rather than producing an argv with an embedded NULL. Tests: quoted, quoted-with-%f, escaped spaces, option flags, weird filename (quoted + unquoted), malformed (unclosed quote), empty command, direct argv-shape regression (one argv value), and no-local-path rejection. --- src/opener.c | 65 +++++++++++----- tests/test_opener.c | 219 +++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 237 insertions(+), 47 deletions(-) diff --git a/src/opener.c b/src/opener.c index f9afb1b..822e332 100644 --- a/src/opener.c +++ b/src/opener.c @@ -51,9 +51,6 @@ opener_get_progs(Opener *o) { return o ? o->p_progs : NULL; } -/* Expand %f → shell-escaped path. Returns a newly-allocated argv vector - * (NULL-terminated). Caller frees with g_strfreev. */ - /* Replace all occurrences of c_old with c_new in c_str. Caller frees. */ static char * _str_replace(const char *c_str, const char *c_old, const char *c_new) { @@ -74,25 +71,51 @@ _str_replace(const char *c_str, const char *c_old, const char *c_new) { return g_string_free(p_out, FALSE); } -static char ** -_expand_command(const char *c_cmd, GFile *p_file) { +/* Expand %f → the file path inside an already-parsed argv. + * + * The command template is parsed with g_shell_parse_argv FIRST, so quotes, + * escapes, and option flags are resolved correctly; %f is then substituted + * into the parsed argv elements. This guarantees %f is always exactly one + * argv value no matter what spaces or shell metacharacters the path contains + * (substitution happens after shell parsing, never before). + * + * Returns a newly-allocated, NULL-terminated argv vector; caller frees with + * g_strfreev. On a parse error returns NULL and sets p_err — never launches + * malformed argv. Non-static (with the `_` prefix retained to mark it as + * non-public) so tests/test_opener.c can verify the argv shape directly via + * an extern prototype, without polluting the public opener.h. */ +char ** +_expand_command(const char *c_cmd, GFile *p_file, GError **p_err) { + g_return_val_if_fail(c_cmd != NULL, NULL); + g_return_val_if_fail(G_IS_FILE(p_file), NULL); char *c_path = g_file_get_path(p_file); - /* Simple tokenisation: split on spaces, replace %f with path. */ - char **parts = g_strsplit(c_cmd, " ", -1); - GPtrArray *argv = g_ptr_array_new(); - for (guint i = 0; parts[i]; i++) { - if (g_str_equal(parts[i], "%f")) - g_ptr_array_add(argv, g_strdup(c_path)); - else if (strstr(parts[i], "%f")) { - char *r = _str_replace(parts[i], "%f", c_path); - g_ptr_array_add(argv, r ? r : g_strdup(parts[i])); - } else - g_ptr_array_add(argv, g_strdup(parts[i])); + if (c_path == NULL) { + g_set_error(p_err, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED, + "opener: file has no local path"); + return NULL; + } + /* Parse the raw template first: resolves quotes/escapes/options. */ + char **argv = NULL; + if (!g_shell_parse_argv(c_cmd, NULL, &argv, p_err)) { + g_free(c_path); + return NULL; + } + /* Substitute %f into each parsed element so the path is always one argv + * value, regardless of spaces or shell metacharacters it contains. */ + for (guint i = 0; argv[i]; i++) { + if (g_str_equal(argv[i], "%f")) { + g_free(argv[i]); + argv[i] = g_strdup(c_path); + } else if (strstr(argv[i], "%f")) { + char *r = _str_replace(argv[i], "%f", c_path); + if (r) { + g_free(argv[i]); + argv[i] = r; + } + } } - g_ptr_array_add(argv, NULL); - g_strfreev(parts); g_free(c_path); - return (char **)g_ptr_array_free(argv, FALSE); + return argv; } gboolean @@ -101,7 +124,9 @@ opener_launch(Opener *o, GFile *p_file, const OpenerProg *p_prog, (void)o; g_return_val_if_fail(G_IS_FILE(p_file), FALSE); g_return_val_if_fail(p_prog, FALSE); - char **argv = _expand_command(p_prog->c_command, p_file); + char **argv = _expand_command(p_prog->c_command, p_file, p_err); + if (argv == NULL) + return FALSE; GSubprocess *p_sub = g_subprocess_newv((const char *const *)argv, G_SUBPROCESS_FLAGS_NONE, p_err); g_strfreev(argv); diff --git a/tests/test_opener.c b/tests/test_opener.c index a364bdb..6e3cd1d 100644 --- a/tests/test_opener.c +++ b/tests/test_opener.c @@ -1,8 +1,18 @@ -/* test_opener.c — %f expansion + launch with true/false. */ +/* test_opener.c — %f expansion + launch with true/false. + * + * The argv-shape regression test (test_expand_weird_one_argv) calls + * _expand_command directly. That function is non-static in opener.c (with the + * `_` prefix retained to mark it as non-public) exactly so this test can + * inspect the parsed argv; it is declared here via an extern prototype rather + * than exposed in the public opener.h, to avoid public-header pollution. */ #include "opener.h" #include #include +/* Non-static internal from opener.c (test-only accessor); see top-of-file + * comment. */ +extern char **_expand_command(const char *c_cmd, GFile *p_file, GError **p_err); + static GFile * make_tmp_file(void) { GError *e = NULL; @@ -17,6 +27,49 @@ make_tmp_file(void) { return f; } +/* Build a temp dir containing a file whose name has spaces, $, and backticks + * — the same "weird" pattern the original test_weird_filename used. */ +static GFile * +make_weird_file(void) { + GError *e = NULL; + char *d = g_dir_make_tmp("ggaze-opener-XXXXXX", &e); + g_assert_no_error(e); + char *p = g_build_filename(d, "file with spaces $HOME `whoami`.jpg", NULL); + GFile *f = g_file_new_for_path(p); + g_file_replace_contents(f, "x", 1, NULL, FALSE, + G_FILE_CREATE_REPLACE_DESTINATION, NULL, NULL, NULL); + g_free(p); + g_free(d); + return f; +} + +/* Delete p_file and its temp parent directory (recursing any leftover + * children so the dir removal succeeds). */ +static void +cleanup_file(GFile *f) { + if (!f) + return; + GFile *d = g_file_get_parent(f); + g_file_delete(f, NULL, NULL); + if (d) { + GFileEnumerator *en = g_file_enumerate_children( + d, "standard::name", G_FILE_QUERY_INFO_NONE, NULL, NULL); + if (en) { + GFileInfo *i; + while ((i = g_file_enumerator_next_file(en, NULL, NULL))) { + GFile *c = g_file_get_child(d, g_file_info_get_name(i)); + g_file_delete(c, NULL, NULL); + g_object_unref(c); + g_object_unref(i); + } + g_object_unref(en); + } + g_file_delete(d, NULL, NULL); + g_object_unref(d); + } + g_object_unref(f); +} + static void test_launch_true(void) { GFile *f = make_tmp_file(); @@ -26,7 +79,7 @@ test_launch_true(void) { g_assert_true(opener_launch(o, f, &prog, &e)); g_assert_no_error(e); opener_delete(o); - g_object_unref(f); + cleanup_file(f); } static void @@ -38,41 +91,144 @@ test_launch_false(void) { g_assert_true(opener_launch(o, f, &prog, &e)); g_assert_no_error(e); opener_delete(o); - g_object_unref(f); + cleanup_file(f); } static void test_weird_filename(void) { - GError *e = NULL; - char *d = g_dir_make_tmp("ggaze-opener-XXXXXX", &e); - g_assert_no_error(e); - char *p = g_build_filename(d, "file with spaces $HOME `whoami`.jpg", NULL); - GFile *f = g_file_new_for_path(p); - g_file_replace_contents(f, "x", 1, NULL, FALSE, - G_FILE_CREATE_REPLACE_DESTINATION, NULL, NULL, NULL); + GFile *f = make_weird_file(); Opener *o = opener_new(); OpenerProg prog = {"true", "true %f"}; + GError *e = NULL; g_assert_true(opener_launch(o, f, &prog, &e)); g_assert_no_error(e); opener_delete(o); + cleanup_file(f); +} + +static void +test_quoted(void) { + GFile *f = make_tmp_file(); + Opener *o = opener_new(); + OpenerProg prog = {"sh -c 'true'", "sh -c 'true'"}; + GError *e = NULL; + g_assert_true(opener_launch(o, f, &prog, &e)); + g_assert_no_error(e); + opener_delete(o); + cleanup_file(f); +} + +static void +test_quoted_with_pctf(void) { + GFile *f = make_tmp_file(); + Opener *o = opener_new(); + OpenerProg prog = {"sh -c 'true %f'", "sh -c 'true %f'"}; + GError *e = NULL; + g_assert_true(opener_launch(o, f, &prog, &e)); + g_assert_no_error(e); + opener_delete(o); + cleanup_file(f); +} + +static void +test_escaped_spaces(void) { + GFile *f = make_tmp_file(); + Opener *o = opener_new(); + OpenerProg prog = {"escaped", "true a\\ b"}; + GError *e = NULL; + g_assert_true(opener_launch(o, f, &prog, &e)); + g_assert_no_error(e); + opener_delete(o); + cleanup_file(f); +} + +static void +test_option_flags(void) { + GFile *f = make_tmp_file(); + Opener *o = opener_new(); + OpenerProg prog = {"opts", "true -verbose %f"}; + GError *e = NULL; + g_assert_true(opener_launch(o, f, &prog, &e)); + g_assert_no_error(e); + opener_delete(o); + cleanup_file(f); +} + +static void +test_weird_filename_quoted(void) { + GFile *f = make_weird_file(); + Opener *o = opener_new(); + OpenerProg prog = {"sh -c 'true %f'", "sh -c 'true %f'"}; + GError *e = NULL; + /* %f is substituted inside the already-parsed single argv element + * `true %f` → `true /weird/path`, passed as ONE arg to `sh -c`. */ + g_assert_true(opener_launch(o, f, &prog, &e)); + g_assert_no_error(e); + opener_delete(o); + cleanup_file(f); +} + +static void +test_malformed(void) { + GFile *f = make_tmp_file(); + Opener *o = opener_new(); + OpenerProg prog = {"bad", "sh -c 'echo hello"}; + GError *e = NULL; + g_assert_false(opener_launch(o, f, &prog, &e)); + g_assert_nonnull(e); + g_error_free(e); + opener_delete(o); + cleanup_file(f); +} + +static void +test_empty(void) { + GFile *f = make_tmp_file(); + Opener *o = opener_new(); + OpenerProg prog = {"empty", ""}; + GError *e = NULL; + g_assert_false(opener_launch(o, f, &prog, &e)); + g_assert_nonnull(e); + g_error_free(e); + opener_delete(o); + cleanup_file(f); +} + +/* The key regression test: with a weird filename (spaces + shell + * metacharacters) and an UNQUOTED %f template, the path must land in the + * parsed argv as exactly ONE element — not split on the path's spaces. `true` + * would ignore extra args, so a launch-only check cannot prove this; we + * inspect the argv shape directly via _expand_command. */ +static void +test_expand_weird_one_argv(void) { + GFile *f = make_weird_file(); + GError *e = NULL; + char **argv = _expand_command("true %f", f, &e); + g_assert_no_error(e); + g_assert_nonnull(argv); + /* argv = {"true", "", NULL} — exactly three slots. */ + g_assert_cmpstr(argv[0], ==, "true"); + g_assert_nonnull(argv[1]); + g_assert_cmpstr(argv[2], ==, NULL); + /* The path element must equal the file's path verbatim, spaces and all. */ + char *c_path = g_file_get_path(f); + g_assert_cmpstr(argv[1], ==, c_path); + g_free(c_path); + g_strfreev(argv); + cleanup_file(f); +} + +/* A non-local GFile (no path) must be rejected with a GError rather than + * producing an argv with an embedded NULL that silently drops trailing args. */ +static void +test_expand_no_local_path(void) { + GFile *f = g_file_new_for_uri("https://example.com/img.jpg"); + GError *e = NULL; + char **argv = _expand_command("editor %f", f, &e); + g_assert_null(argv); + g_assert_nonnull(e); + g_error_free(e); g_object_unref(f); - g_free(p); - GFile *dd = g_file_new_for_path(d); - GFileEnumerator *en = g_file_enumerate_children( - dd, "standard::name", G_FILE_QUERY_INFO_NONE, NULL, NULL); - if (en) { - GFileInfo *i; - while ((i = g_file_enumerator_next_file(en, NULL, NULL))) { - GFile *c = g_file_get_child(dd, g_file_info_get_name(i)); - g_file_delete(c, NULL, NULL); - g_object_unref(c); - g_object_unref(i); - } - g_object_unref(en); - } - g_file_delete(dd, NULL, NULL); - g_object_unref(dd); - g_free(d); } int @@ -81,5 +237,14 @@ main(int argc, char **argv) { g_test_add_func("/opener/true", test_launch_true); g_test_add_func("/opener/false", test_launch_false); g_test_add_func("/opener/weird_filename", test_weird_filename); + g_test_add_func("/opener/quoted", test_quoted); + g_test_add_func("/opener/quoted_with_pctf", test_quoted_with_pctf); + g_test_add_func("/opener/escaped_spaces", test_escaped_spaces); + g_test_add_func("/opener/option_flags", test_option_flags); + g_test_add_func("/opener/weird_filename_quoted", test_weird_filename_quoted); + g_test_add_func("/opener/malformed", test_malformed); + g_test_add_func("/opener/empty", test_empty); + g_test_add_func("/opener/expand_weird_one_argv", test_expand_weird_one_argv); + g_test_add_func("/opener/expand_no_local_path", test_expand_no_local_path); return g_test_run(); } \ No newline at end of file -- cgit v1.2.3