From 90ca3b4dd04a63be4e963d52021de20cd2219cc0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 8 Oct 2018 13:13:22 +1300 Subject: [PATCH 1/3] tests: Fix some minor memory leaks in gsubprocess-testprog This just makes the valgrind logs a bit cleaner so we can find real problems in future. Signed-off-by: Philip Withnall --- gio/tests/gsubprocess-testprog.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gio/tests/gsubprocess-testprog.c b/gio/tests/gsubprocess-testprog.c index 74cb9e2a3..c9b06c2a2 100644 --- a/gio/tests/gsubprocess-testprog.c +++ b/gio/tests/gsubprocess-testprog.c @@ -200,12 +200,17 @@ main (int argc, char **argv) GOptionContext *context; GError *error = NULL; const char *mode; + gboolean ret; context = g_option_context_new ("MODE - Test GSubprocess stuff"); g_option_context_add_main_entries (context, options, NULL); - if (!g_option_context_parse (context, &argc, &argv, &error)) + ret = g_option_context_parse (context, &argc, &argv, &error); + g_option_context_free (context); + + if (!ret) { g_printerr ("%s: %s\n", argv[0], error->message); + g_error_free (error); return 1; } From 4795dadde49dac18a7260044a74bf2236c188595 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 11 Oct 2018 11:29:48 +1300 Subject: [PATCH 2/3] gsubprocess: Clear std buf outputs to NULL on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of sometimes returning a non-NULL buffer, always return NULL. However, keep the documentation as explicitly returning undefined values on failure, so that we can change the behaviour in future if needed. The return values weren’t defined for failure before, so were implicitly returning undefined values. Signed-off-by: Philip Withnall --- gio/gsubprocess.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/gio/gsubprocess.c b/gio/gsubprocess.c index 23bfbddf2..dcdb84d58 100644 --- a/gio/gsubprocess.c +++ b/gio/gsubprocess.c @@ -1784,6 +1784,9 @@ g_subprocess_communicate_finish (GSubprocess *subprocess, * * Like g_subprocess_communicate(), but validates the output of the * process as UTF-8, and returns it as a regular NUL terminated string. + * + * On error, @stdout_buf and @stderr_buf will be set to undefined values and + * should not be used. */ gboolean g_subprocess_communicate_utf8 (GSubprocess *subprocess, @@ -1868,6 +1871,7 @@ communicate_result_validate_utf8 (const char *stream_name, if (!g_utf8_validate (*return_location, -1, &end)) { g_free (*return_location); + *return_location = NULL; g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Invalid UTF-8 in child %s at offset %lu", stream_name, @@ -1900,6 +1904,7 @@ g_subprocess_communicate_utf8_finish (GSubprocess *subprocess, { gboolean ret = FALSE; CommunicateState *state; + gchar *local_stdout_buf = NULL, *local_stderr_buf = NULL; g_return_val_if_fail (G_IS_SUBPROCESS (subprocess), FALSE); g_return_val_if_fail (g_task_is_valid (result, subprocess), FALSE); @@ -1913,11 +1918,11 @@ g_subprocess_communicate_utf8_finish (GSubprocess *subprocess, /* TODO - validate UTF-8 while streaming, rather than all at once. */ - if (!communicate_result_validate_utf8 ("stdout", stdout_buf, + if (!communicate_result_validate_utf8 ("stdout", &local_stdout_buf, state->stdout_buf, error)) goto out; - if (!communicate_result_validate_utf8 ("stderr", stderr_buf, + if (!communicate_result_validate_utf8 ("stderr", &local_stderr_buf, state->stderr_buf, error)) goto out; @@ -1925,5 +1930,14 @@ g_subprocess_communicate_utf8_finish (GSubprocess *subprocess, ret = TRUE; out: g_object_unref (result); + + if (ret && stdout_buf != NULL) + *stdout_buf = g_steal_pointer (&local_stdout_buf); + if (ret && stderr_buf != NULL) + *stderr_buf = g_steal_pointer (&local_stderr_buf); + + g_free (local_stderr_buf); + g_free (local_stdout_buf); + return ret; } From 19c7a7bb233fbe7138d535ac4a2354772edc6486 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 11 Oct 2018 11:29:17 +1300 Subject: [PATCH 3/3] gsubprocess: Add a missing test for invalid UTF-8 output There were tests for invalid UTF-8 output when asynchronously communicating with a subprocess, but nothing for synchronous communication. Add such a test, and refine the code as a result. Signed-off-by: Philip Withnall --- gio/tests/gsubprocess.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c index f617bc599..431828384 100644 --- a/gio/tests/gsubprocess.c +++ b/gio/tests/gsubprocess.c @@ -1190,7 +1190,7 @@ test_communicate_nothing (void) } static void -test_communicate_utf8_invalid (void) +test_communicate_utf8_async_invalid (void) { GSubprocessFlags flags = G_SUBPROCESS_FLAGS_STDOUT_PIPE; GError *error = NULL; @@ -1222,6 +1222,37 @@ test_communicate_utf8_invalid (void) g_object_unref (proc); } +/* Test that invalid UTF-8 received using g_subprocess_communicate_utf8() + * results in an error. */ +static void +test_communicate_utf8_invalid (void) +{ + GSubprocessFlags flags = G_SUBPROCESS_FLAGS_STDOUT_PIPE; + GError *local_error = NULL; + gboolean ret; + GPtrArray *args; + gchar *stdout_str = NULL, *stderr_str = NULL; + GSubprocess *proc; + + args = get_test_subprocess_args ("cat", NULL); + proc = g_subprocess_newv ((const gchar* const*)args->pdata, + G_SUBPROCESS_FLAGS_STDIN_PIPE | flags, + &local_error); + g_assert_no_error (local_error); + g_ptr_array_free (args, TRUE); + + ret = g_subprocess_communicate_utf8 (proc, "\xFF\xFF", NULL, + &stdout_str, &stderr_str, &local_error); + g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_FAILED); + g_error_free (local_error); + g_assert_false (ret); + + g_assert_null (stdout_str); + g_assert_null (stderr_str); + + g_object_unref (proc); +} + static gboolean send_terminate (gpointer user_data) { @@ -1783,6 +1814,7 @@ main (int argc, char **argv) g_free (test_path); } + g_test_add_func ("/gsubprocess/communicate/utf8/async/invalid", test_communicate_utf8_async_invalid); g_test_add_func ("/gsubprocess/communicate/utf8/invalid", test_communicate_utf8_invalid); g_test_add_func ("/gsubprocess/communicate/nothing", test_communicate_nothing); g_test_add_func ("/gsubprocess/terminate", test_terminate);