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

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

[Sascha Silbe]

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 <silbe@linux.vnet.ibm.com>
---
 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);
+}
+
+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);
 }
 
 int main(int argc, char **argv)
 {
+    gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
+    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;
 }
-- 
1.9.1

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

[Peter Maydell]

On 15 July 2016 at 17:24, Sascha Silbe <silbe@linux.vnet.ibm.com> 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 <silbe@linux.vnet.ibm.com>

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

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

[Sascha Silbe]

Dear Peter,

Peter Maydell <peter.maydell@linaro.org> writes:

> On 15 July 2016 at 17:24, Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
[...]
>> 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 <silbe@linux.vnet.ibm.com>
>
> 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...

I have totally forgotten about it. Would probably have remembered the
next time "make check" failed on a shared machine. ;)


[tests/test-logging.c:test_parse_path()]
>> +    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);
[...]
> This could usefully be refactored to have a helper function that does
> the g_build_filename/qemu_set_log_filename/g_free.

I wasn't sure about it but it actually ended up being a bit nicer than
what I had before.


>> +static void rmtree(gchar const *root)
[...]
> 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).

I don't really like the rm spawning solution either. But the above plus
error handling was a bit much for a single test for my taste.

Is there some place I could stick something like the above so it could
at least be reused by other tests in the future? (Even though I very
much hate the idea of implementing yet another rmtree()).


[...]
>>  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.

Bummer. The old interfaces were really awkward. Is there somewhere I
could put a compatibility wrapper, implementing g_dir_make_tmp() using
the old interfaces?


Thanks for the review!

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

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

[Peter Maydell]

On 15 August 2016 at 19:24, Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On 15 July 2016 at 17:24, Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
> [...]

>>> +static void rmtree(gchar const *root)
> [...]
>> 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).
>
> I don't really like the rm spawning solution either. But the above plus
> error handling was a bit much for a single test for my taste.
>
> Is there some place I could stick something like the above so it could
> at least be reused by other tests in the future? (Even though I very
> much hate the idea of implementing yet another rmtree()).

tests/libqtest.c is probably as good a place as any.

> [...]
>>>  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.
>
> Bummer. The old interfaces were really awkward. Is there somewhere I
> could put a compatibility wrapper, implementing g_dir_make_tmp() using
> the old interfaces?

include/glib-compat.h is where we provide compatible reimplementations
of glib function for older setups.

thanks
-- PMM

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

[Peter Maydell]

On 15 August 2016 at 19:24, Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
> Dear Peter,
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 15 July 2016 at 17:24, Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
> [...]
>>> 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 <silbe@linux.vnet.ibm.com>
>>
>> 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...
>
> I have totally forgotten about it. Would probably have remembered the
> next time "make check" failed on a shared machine. ;)

Are you planning to send a v2 of this patch? I was hoping we could
fix the non-deleted logfiles for qemu 2.7.0 but it's getting a bit
late in the cycle...

thanks
-- PMM

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

[Sascha Silbe]

Dear Peter,

Peter Maydell <peter.maydell@linaro.org> writes:

> Are you planning to send a v2 of this patch? I was hoping we could
> fix the non-deleted logfiles for qemu 2.7.0 but it's getting a bit
> late in the cycle...

I'll try cooking up a version that's good enough for 2.7. I expected it
to land in 2.8 so I wasn't in a hurry.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641