This is an automated email from the git hooks/post-receive script. x2go pushed a commit to branch master in repository libx2goclient. commit b74bd64a3434da8d9954baad0eac09ac9ce063cd Author: Mihai Moldovan <ionic@ionic.de> Date: Sat Jun 27 14:40:49 2020 +0200 src/x2goclient-openssh-version.c: rework std{out,err} size checking. Use loops instead of copy-pasting the same code twice, since we really need to do (almost) exactly the same thing twice. Truncate both streams on newlines. We're not interested in additional lines. Keep the (hopefully) correct size truncation around. --- src/x2goclient-openssh-version.c | 216 ++++++++++++++++++++++++++++----------- 1 file changed, 155 insertions(+), 61 deletions(-) diff --git a/src/x2goclient-openssh-version.c b/src/x2goclient-openssh-version.c index 0b10ab6..f169428 100644 --- a/src/x2goclient-openssh-version.c +++ b/src/x2goclient-openssh-version.c @@ -421,73 +421,165 @@ X2GoClientOpenSSHVersion* x2goclient_openssh_version_fetch_openssh_version (GErr g_propagate_prefixed_error (gerr, ssh_err, "Communication with OpenSSH version fetching subprocess failed: "); } else { - gsize ssh_stdout_size = 0, ssh_stderr_size = 0; - const gchar *ssh_stdout_str = g_bytes_get_data (ssh_stdout, &ssh_stdout_size), - *ssh_stderr_str = g_bytes_get_data (ssh_stderr, &ssh_stderr_size); - int ssh_stdout_size_sanitized = 0, ssh_stderr_size_sanitized = 0; - gchar *ssh_stdout_str_sanitized = 0, *ssh_stderr_str_sanitized = NULL; + /* First element is stdout, second one stderr. */ + gsize ssh_std_sizes[2] = { 0 }; + const gchar *ssh_std_strs[2] = { g_bytes_get_data (ssh_stdout, ssh_std_sizes), + g_bytes_get_data (ssh_stderr, (ssh_std_sizes + 1)) }; + int ssh_std_sizes_sanitized[2] = { 0 }; + gchar *ssh_std_strs_sanitized[2] = { NULL }; + + for (size_t i = 0; i < (sizeof (ssh_std_strs) / sizeof (*ssh_std_strs)); ++i) { + const gchar *comp = ""; + switch (i) { + case (0): + comp = "stdout"; + break; + case (1): + comp = "stderr"; + break; + default: + comp = "unknown"; + break; + } - /* - * FIXME: something about this section looks odd, revisit. - * Also, we should probably drop everything but the first line - * early on. - */ - /* Sanity check on output size. */ - if (INT_MAX < ssh_stdout_size) { - g_log (NULL, G_LOG_LEVEL_WARNING, "OpenSSH returned more than %d bytes on stdout, this is unusual and will be truncated.", INT_MAX); + if (ssh_std_strs[i]) { + /* + * Truncate on first newline character. + * + * Yes, we could, theoretically, just replace the first occurrence of + * the newline character with zero and continue with the truncated + * result, but we'd potentially carry a lot of other useless data + * around. + * + * Better clean it up and free up a bit of memory. + * + * N.B.: The cast here looks weird and it's arguably ugly, but it's + * actually safe. For historical reasons, free () (and + * wrappers) have been declared as taking a non-const pointer + * to data. Whether this is correct or not, and especially + * whether this should have been a const pointer from the + * beginning rather than a non-const one is still actively + * debated. Some people (like Linus Torvalds[0]) argue that + * glibc is broken for several reasons: + * - free () does not actually change any data but + * invalidates the pointer itself (and, by extension, any + * data it is pointing to, but, again, crucially is NOT + * modifying this data) + * - types should be as tight as possible to avoid the need + * of casts + * - the "const" keyword does not mean "constant data" (as + * often wrongly assumed), but actually that it should be + * impossible to modify the data via this very pointer, + * i.e., that it is a pointer property, not a data + * property. + * Then, there's this other camp of people that argue that + * constifying something is a *contract* not to change it + * (usually not even via a different pointer) and that free () + * CAN and actually WILL change the memory pointed to. The + * first argument is a "gut feeling" and not backed by any + * standard (and, IMHO, bogus), but the second argument can + * surprisingly be true. The way memory is allocated and + * accounted for is application- (or system-) defined and this + * opens the door to *a lot* of weird things. For instance, + * the system could track allocated memory (or, e.g., + * statistics) by using data after the allocated chunk of + * memory returned to the application - using the pointer to + * access and, crucially, modify this private data. This + * freedom would naturally not be possible if the input + * pointer was const. + * So much for legacy. + * But why is it safe HERE? + * The reason is quite simple: we *know* that the called + * function WILL NOT modify the input string, not even call + * free () on it, if we explicitly disable that feature via + * its boolean parameter. We could make the input string + * parameter const instead, but this would require a cast when + * calling g_free (), which seems to be frowned upon. Thus, + * we'll keep it taking a non-const pointer for the time being + * and cast data passed to it if necessary. + * + * [0] https://web.archive.org/https://yarchive.net/comp/const.html + */ + ssh_std_strs_sanitized[i] = x2goclient_strbrk_dup ((gchar **)(ssh_std_strs + i), + '\n', FALSE, + (ssh_std_sizes + i)); + + /* + * Next, truncate for size. + * + * We know that the original data is valid. + * + * The sanitized array entry might contain valid data if we already + * truncated based on a newline previously. + * + * Regardless, the original size entries will always contain our + * desired data. + */ + if (INT_MAX < ssh_std_sizes[i]) { + g_log (NULL, G_LOG_LEVEL_WARNING, "OpenSSH returned more than %d bytes on %s, this is unusual and will be truncated.", + INT_MAX, comp); + + ssh_std_sizes_sanitized[i] = INT_MAX; + } + else { + ssh_std_sizes_sanitized[i] = ssh_std_sizes[i]; + } - ssh_stdout_size_sanitized = INT_MAX; - } - else { - ssh_stdout_size_sanitized = ssh_stdout_size; - } + /* + * Do NOT use g_strndup () here. + * + * It might sound exactly like what we want, but really isn't. + * + * The issue is that g_strndup () will always allocate an n-bytes-sized + * buffer and optionally pad the string with NULL bytes. + * + * That's not really a problem if the string actually is to be + * truncated, since in that case the result will be smaller than the + * original string anyway, but a problem if the size is huge, but the + * string small. + * + * In the latter case, we don't want to create a useless 2 GiB (bounded + * by INT_MAX) string for a string that is actually quite small. + * + * Interestingly, POSIX describes strndup as allocating memory "as if + * by using malloc ()", but doesn't mention the actual resulting size. + * In the informal section, buffer sizes are described as either + * (size + 1) or ((strnlen (s, size)) + 1), which, again, could lead + * to the same problem. + */ + if (ssh_std_strs_sanitized[i]) { + gchar *tmp = g_strdup_printf ("%.*s", ssh_std_sizes_sanitized[i], ssh_std_strs_sanitized[i]); + + /* Remember to not leak out old data! */ + g_free (ssh_std_strs_sanitized[i]); + + ssh_std_strs_sanitized[i] = tmp; + } + else { + /* We'll need to copy, so no leak here. */ + ssh_std_strs_sanitized[i] = g_strdup_printf ("%.*s", ssh_std_sizes_sanitized[i], ssh_std_strs[i]); + } + + g_log (NULL, G_LOG_LEVEL_DEBUG, "%s:\n>>>%.*s<<<", comp, ssh_std_sizes_sanitized[i], ssh_std_strs_sanitized[i]); + } + else { + g_log (NULL, G_LOG_LEVEL_DEBUG, "%s: no data", comp); + } - if (ssh_stdout_str) { /* - * Do NOT use g_strndup () here. - * - * It might sound exactly like what we want, but really isn't. - * - * The issue is that g_strndup () will always allocate an n-bytes-sized - * buffer and optionally pad the string with NULL bytes. - * - * That's not really a problem if the string actually is to be - * truncated, since in that case the result will be smaller than the - * original string anyway, but a problem if the size is huge, but the - * string small. - * - * In the latter case, we don't want to create a useless 2 GiB (bounded - * by INT_MAX) string for a string that is actually quite small. - * - * Interestingly, POSIX describes strndup as allocating memory "as if - * by using malloc ()", but doesn't mention the actual resulting size. - * In the informal section, buffer sizes are described as either - * (size + 1) or ((strnlen (s, size)) + 1), which, again, could lead - * to the same problem. + * Add additional newline unless we're at the end to hopefully make + * output more readable. */ - ssh_stdout_str_sanitized = g_strdup_printf ("%.*s", ssh_stdout_size_sanitized, ssh_stdout_str); - } - else { - ssh_stderr_size_sanitized = ssh_stderr_size; - } - - if (INT_MAX < ssh_stderr_size) { - g_log (NULL, G_LOG_LEVEL_WARNING, "OpenSSH returned more than %d bytes on stderr, this is unusual and will be truncated.", INT_MAX); - - ssh_stderr_size_sanitized = INT_MAX; - } - - if (ssh_stderr_str) { - ssh_stderr_str_sanitized = g_strdup_printf ("%.*s", ssh_stderr_size_sanitized, ssh_stderr_str); + if (((sizeof (ssh_std_strs) / sizeof (*ssh_std_strs)) - 1) != i) { + g_log (NULL, G_LOG_LEVEL_DEBUG, ""); + } } - g_log (NULL, G_LOG_LEVEL_DEBUG, "Stdout:\n>>>%.*s<<<\nStderr:\n>>>%.*s<<<", ssh_stdout_size_sanitized, ssh_stdout_str_sanitized, ssh_stderr_size_sanitized, ssh_stderr_str_sanitized); - - if (ssh_stdout_size_sanitized) { - g_log (NULL, G_LOG_LEVEL_WARNING, "OpenSSH version command wrote data on stdout, this is unexpected and will be ignored.\nData: %*.s", ssh_stdout_size_sanitized, ssh_stdout_str_sanitized); + if (ssh_std_sizes_sanitized[0]) { + g_log (NULL, G_LOG_LEVEL_WARNING, "OpenSSH version command wrote data on stdout, this is unexpected and will be ignored.\nData: %*.s", ssh_std_sizes_sanitized[0], ssh_std_strs_sanitized[0]); } - if (!(ssh_stderr_size_sanitized)) { + if (!(ssh_std_sizes_sanitized[1])) { g_set_error_literal (gerr, X2GOCLIENT_OPENSSH_VERSION_ERROR, X2GOCLIENT_OPENSSH_VERSION_ERROR_OPENSSH_VERSION_NO_STDERR, "OpenSSH version command wrote nothing to stderr, this is unexpected. Can't parse version string."); } else { @@ -500,7 +592,7 @@ X2GoClientOpenSSHVersion* x2goclient_openssh_version_fetch_openssh_version (GErr /* gerr is supposed to be empty at that point. */ g_assert ((NULL == gerr) || (NULL == *gerr)); - cont = x2goclient_openssh_version_parse (ret, ssh_stderr_str_sanitized, gerr); + cont = x2goclient_openssh_version_parse (ret, ssh_std_strs_sanitized[1], gerr); if (!(cont)) { /* Get rid of the version struct. It's bogus anyway. */ @@ -511,8 +603,10 @@ X2GoClientOpenSSHVersion* x2goclient_openssh_version_fetch_openssh_version (GErr } } - g_free (ssh_stdout_str_sanitized); - g_free (ssh_stderr_str_sanitized); + for (size_t i = 0; i < (sizeof (ssh_std_strs_sanitized) / sizeof (*ssh_std_strs_sanitized)); ++i) { + g_free (ssh_std_strs_sanitized[i]); + ssh_std_strs_sanitized[i] = NULL; + } g_bytes_unref (ssh_stdout); ssh_stdout = NULL; -- Alioth's /home/x2go-admin/maintenancescripts/git/hooks/post-receive-email on /srv/git/code.x2go.org/libx2goclient.git