Re: [Qemu-devel] [PATCH] test-logging: don't hard-code paths in /tmp

[Peter Maydell]

On 15 July 2016 at 17:24, Sascha Silbe <address@hidden> wrote:
> Since f6880b7f [qemu-log: support simple pid substitution for logs],
> test-logging creates files with hard-coded names in /tmp. In the best
> case, this prevents multiple developers from running "make check" on
> the same machine. In the worst case, it allows for symlink attacks,
> enabling an attacker to overwrite files that are writable to the
> developer running "make check".
>
> Instead of hard-coding the paths, create a temporary directory using
> g_dir_make_tmp() and clean it up afterwards.
>
> Fixes: f6880b7f ("qemu-log: support simple pid substitution for logs")
> Signed-off-by: Sascha Silbe <address@hidden>

Thanks for this patch -- I just noticed that the test was leaving
temporary files not cleaned up, which brought me to this patch
by searching the mail archives...

> ---
>  tests/test-logging.c | 42 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/tests/test-logging.c b/tests/test-logging.c
> index cdf13c6..faebc35 100644
> --- a/tests/test-logging.c
> +++ b/tests/test-logging.c
> @@ -86,24 +86,52 @@ static void test_parse_range(void)
>      error_free_or_abort(&err);
>  }
>
> -static void test_parse_path(void)
> +static void test_parse_path(gconstpointer data)
>  {
> +    gchar const *tmp_path = data;
> +    gchar *plain_path = g_build_filename(tmp_path, "qemu.log", NULL);
> +    gchar *pid_infix_path = g_build_filename(tmp_path, "qemu-%d.log", NULL);
> +    gchar *pid_suffix_path = g_build_filename(tmp_path, "qemu.log.%d", NULL);
> +    gchar *double_pid_path = g_build_filename(tmp_path, "qemu-%d%d.log", 
> NULL);
>      Error *err = NULL;
>
> -    qemu_set_log_filename("/tmp/qemu.log", &error_abort);
> -    qemu_set_log_filename("/tmp/qemu-%d.log", &error_abort);
> -    qemu_set_log_filename("/tmp/qemu.log.%d", &error_abort);
> +    qemu_set_log_filename(plain_path, &error_abort);
> +    qemu_set_log_filename(pid_infix_path, &error_abort);
> +    qemu_set_log_filename(pid_suffix_path, &error_abort);
>
> -    qemu_set_log_filename("/tmp/qemu-%d%d.log", &err);
> +    qemu_set_log_filename(double_pid_path, &err);
>      error_free_or_abort(&err);
> +
> +    g_free(double_pid_path);
> +    g_free(pid_suffix_path);
> +    g_free(pid_infix_path);
> +    g_free(plain_path);

This could usefully be refactored to have a helper function that does
the g_build_filename/qemu_set_log_filename/g_free.

> +static void rmtree(gchar const *root)
> +{
> +    /* There should really be a g_rmtree(). Implementing it ourselves
> +     * isn't really worth it just for a test, so let's just use rm. */
> +    gchar const *rm_args[] = { "rm", "-rf", root, NULL };
> +    g_spawn_sync(NULL, (gchar **)rm_args, NULL,
> +                 G_SPAWN_SEARCH_PATH, NULL, NULL,
> +                 NULL, NULL, NULL, NULL);
>  }

I don't really like spawning rm here for this. We know we
don't have any subdirectories here so we can just
   gdir = g_dir_open(root, 0, NULL);
   for (;;) {
       const char *f = g_dir_read_name(gdir);
       if (!f) {
           break;
       }
       g_remove(f);
   }
   g_dir_close(gdir);
   g_rmdir(root);

(add error handling to taste).

>
>  int main(int argc, char **argv)
>  {
> +    gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);

Unfortunately g_dir_make_tmp() only came in in glib 2.30, and we have
to be able to build with glib 2.22.

> +    int rc;
> +
>      g_test_init(&argc, &argv, NULL);
> +    g_assert_nonnull(tmp_path);
>
>      g_test_add_func("/logging/parse_range", test_parse_range);
> -    g_test_add_func("/logging/parse_path", test_parse_path);
> +    g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path);
>
> -    return g_test_run();
> +    rc = g_test_run();
> +
> +    rmtree(tmp_path);
> +    g_free(tmp_path);
> +    return rc;
>  }

thanks
-- PMM