On 15 July 2016 at 17:24, Sascha Silbe <si...@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 <si...@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
