[libvirt PATCH v2 00/20] Overhaul test/commandhelper.c
I stumbled upon a buffer overflow / stack smash present in "test/commandhelper.c" that could be triggered by e.g. $ ./tests/commandhelper --readfd 0 --readfd 0 --readfd 0 --readfd x Could not parse fd x *** stack smashing detected ***: terminated Aborted (core dumped) This series cleans up the file and fixes the buffer overflow. Note that it does not touch the "prevent malloc with zero size" issue discussed in https://www.redhat.com/archives/libvir-list/2021-January/msg01160.html, this will be done in the other series. V1: https://www.redhat.com/archives/libvir-list/2021-January/msg01246.html Changes since V1: * Patch 3: Treat '=' as null byte in string comparison, preserving the "compare only the keys" semantics. * Patch 14: Overallocate 'buffers' by one, to null terminate the list of strings. This makes the cleanup function of 'buffers' independent of 'args->numreadfs'. * Patch 15: Fix a memory leak (that was fixed in last patch anyway). * Patch 16..: Rewritten to explicitly not use any glib code. * Added conversion of 'printInput' to automatic memory management . Cheers, Tim Tim Wiederhake (20): commandhelper: Remove origenv variable commandhelper: Remove numpollfds variable commandhelper: Simplify envsort commandhelper: Consolidate error paths commandhelper: Consolidate argument parsing commandhelper: Split argument parsing and printing commandhelper: Factor out parseArguments commandhelper: Factor out printArguments commandhelper: Factor out printEnvironment commandhelper: Factor out printFds commandhelper: Factor out printDaemonization commandhelper: Factor out printCwd commandhelper: Factor out printInput commandhelper: Make number of fds variable in printInput commandhelper: Make number of fds variable in parseArguments commandhelper: Use automatic memory management in parseArguments commandhelper: Use automatic memory management in printEnvironment commandhelper: Use automatic memory management in printCwd commandhelper: Use automatic memory management in printInput commandhelper: Use automatic memory management in main tests/commandhelper.c | 344 ++++++++++++++++++++++++++++-------------- 1 file changed, 229 insertions(+), 115 deletions(-) -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index ba5681b715..c2040b76f0 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -60,7 +60,6 @@ static int envsort(const void *a, const void *b) int main(int argc, char **argv) { size_t i, n; int open_max; - char **origenv; char **newenv = NULL; char *cwd; FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); @@ -92,23 +91,16 @@ int main(int argc, char **argv) { } } - origenv = environ; - n = 0; - while (*origenv != NULL) { - n++; - origenv++; + for (n = 0; environ[n]; n++) { } if (!(newenv = malloc(sizeof(*newenv) * n))) abort(); - origenv = environ; - n = i = 0; - while (*origenv != NULL) { - newenv[i++] = *origenv; - n++; - origenv++; + for (i = 0; i < n; i++) { + newenv[i] = environ[i]; } + qsort(newenv, n, sizeof(newenv[0]), envsort); for (i = 0; i < n; i++) { -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index c2040b76f0..2b937979c0 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -67,7 +67,6 @@ int main(int argc, char **argv) { int readfds[3] = { STDIN_FILENO, }; int numreadfds = 1; struct pollfd fds[3]; - int numpollfds = 0; char *buffers[3] = {NULL, NULL, NULL}; size_t buflen[3] = {0, 0, 0}; char c; @@ -167,21 +166,20 @@ int main(int argc, char **argv) { fflush(stderr); for (i = 0; i < numreadfds; i++) { - fds[numpollfds].fd = readfds[i]; - fds[numpollfds].events = POLLIN; - fds[numpollfds].revents = 0; - numpollfds++; + fds[i].fd = readfds[i]; + fds[i].events = POLLIN; + fds[i].revents = 0; } for (;;) { unsigned ctr = 0; - if (poll(fds, numpollfds, -1) < 0) { + if (poll(fds, numreadfds, -1) < 0) { printf("poll failed: %s\n", strerror(errno)); goto cleanup; } - for (i = 0; i < numpollfds; i++) { + for (i = 0; i < numreadfds; i++) { short revents = POLLIN | POLLHUP | POLLERR; # ifdef __APPLE__ @@ -212,7 +210,7 @@ int main(int argc, char **argv) { } } } - for (i = 0; i < numpollfds; i++) { + for (i = 0; i < numreadfds; i++) { if (fds[i].events) { ctr++; break; @@ -222,7 +220,7 @@ int main(int argc, char **argv) { break; } - for (i = 0; i < numpollfds; i++) { + for (i = 0; i < numreadfds; i++) { if (fwrite(buffers[i], 1, buflen[i], stdout) != buflen[i]) goto cleanup; if (fwrite(buffers[i], 1, buflen[i], stderr) != buflen[i]) -- 2.26.2
This saves two invocations of each `strndup` and `free`. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 2b937979c0..22835302b8 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -37,24 +37,19 @@ extern char **environ; static int envsort(const void *a, const void *b) { - const char *const*astrptr = a; - const char *const*bstrptr = b; - const char *astr = *astrptr; - const char *bstr = *bstrptr; - char *aeq = strchr(astr, '='); - char *beq = strchr(bstr, '='); - char *akey; - char *bkey; - int ret; - - if (!(akey = strndup(astr, aeq - astr))) - abort(); - if (!(bkey = strndup(bstr, beq - bstr))) - abort(); - ret = strcmp(akey, bkey); - free(akey); - free(bkey); - return ret; + const char *astr = *(const char**)a; + const char *bstr = *(const char**)b; + + while (true) { + char achar = (*astr == '=') ? '\0' : *astr; + char bchar = (*bstr == '=') ? '\0' : *bstr; + + if ((achar == '\0') || (achar != bchar)) + return achar - bchar; + + astr++; + bstr++; + } } int main(int argc, char **argv) { -- 2.26.2
Preparation for later conversion to g_auto* memory handling. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 22835302b8..1ee697498c 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -71,7 +71,7 @@ int main(int argc, char **argv) { ssize_t got; if (!log) - return ret; + goto cleanup; for (i = 1; i < argc; i++) { fprintf(log, "ARG:%s\n", argv[i]); @@ -89,7 +89,7 @@ int main(int argc, char **argv) { } if (!(newenv = malloc(sizeof(*newenv) * n))) - abort(); + goto cleanup; for (i = 0; i < n; i++) { newenv[i] = environ[i]; @@ -232,8 +232,10 @@ int main(int argc, char **argv) { cleanup: for (i = 0; i < G_N_ELEMENTS(buffers); i++) free(buffers[i]); - fclose(log); - free(newenv); + if (newenv) + free(newenv); + if (log) + fclose(log); return ret; } -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 1ee697498c..62e1b98c60 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -66,6 +66,7 @@ int main(int argc, char **argv) { size_t buflen[3] = {0, 0, 0}; char c; bool daemonize_check = false; + bool close_stdin = false; size_t daemonize_retries = 3; char buf[1024]; ssize_t got; @@ -82,6 +83,8 @@ int main(int argc, char **argv) { goto cleanup; } else if (STREQ(argv[i], "--check-daemonize")) { daemonize_check = true; + } else if (STREQ(argv[i], "--close-stdin")) { + close_stdin = true; } } @@ -149,7 +152,7 @@ int main(int argc, char **argv) { fprintf(log, "UMASK:%04o\n", umask(0)); - if (argc > 1 && STREQ(argv[1], "--close-stdin")) { + if (close_stdin) { if (freopen("/dev/null", "r", stdin) != stdin) goto cleanup; usleep(100*1000); -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 62e1b98c60..24373d6c07 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -75,8 +75,6 @@ int main(int argc, char **argv) { goto cleanup; for (i = 1; i < argc; i++) { - fprintf(log, "ARG:%s\n", argv[i]); - if (STREQ(argv[i - 1], "--readfd") && sscanf(argv[i], "%u%c", &readfds[numreadfds++], &c) != 1) { printf("Could not parse fd %s\n", argv[i]); @@ -88,6 +86,10 @@ int main(int argc, char **argv) { } } + for (i = 1; i < argc; i++) { + fprintf(log, "ARG:%s\n", argv[i]); + } + for (n = 0; environ[n]; n++) { } -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 83 +++++++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 26 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 24373d6c07..f7e406ec83 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -35,6 +35,51 @@ extern char **environ; # define VIR_FROM_THIS VIR_FROM_NONE +struct Arguments { + int readfds[3]; + int numreadfds; + bool daemonize_check; + bool close_stdin; +}; + +static struct Arguments *parseArguments(int argc, char** argv) +{ + struct Arguments* args = NULL; + int ret = -1; + size_t i; + + if (!(args = calloc(1, sizeof(*args)))) + goto cleanup; + + args->numreadfds = 1; + args->readfds[0] = STDIN_FILENO; + + for (i = 1; i < argc; i++) { + if (STREQ(argv[i - 1], "--readfd")) { + char c; + + if (1 != sscanf(argv[i], "%u%c", + &args->readfds[args->numreadfds++], &c)) { + printf("Could not parse fd %s\n", argv[i]); + goto cleanup; + } + } else if (STREQ(argv[i], "--check-daemonize")) { + args->daemonize_check = true; + } else if (STREQ(argv[i], "--close-stdin")) { + args->close_stdin = true; + } + } + + ret = 0; + + cleanup: + if (ret == 0) + return args; + + free(args); + return NULL; +} + static int envsort(const void *a, const void *b) { const char *astr = *(const char**)a; @@ -53,39 +98,23 @@ static int envsort(const void *a, const void *b) } int main(int argc, char **argv) { + struct Arguments *args = parseArguments(argc, argv); size_t i, n; int open_max; char **newenv = NULL; char *cwd; FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); int ret = EXIT_FAILURE; - int readfds[3] = { STDIN_FILENO, }; - int numreadfds = 1; struct pollfd fds[3]; char *buffers[3] = {NULL, NULL, NULL}; size_t buflen[3] = {0, 0, 0}; - char c; - bool daemonize_check = false; - bool close_stdin = false; size_t daemonize_retries = 3; char buf[1024]; ssize_t got; - if (!log) + if (!log || !args) goto cleanup; - for (i = 1; i < argc; i++) { - if (STREQ(argv[i - 1], "--readfd") && - sscanf(argv[i], "%u%c", &readfds[numreadfds++], &c) != 1) { - printf("Could not parse fd %s\n", argv[i]); - goto cleanup; - } else if (STREQ(argv[i], "--check-daemonize")) { - daemonize_check = true; - } else if (STREQ(argv[i], "--close-stdin")) { - close_stdin = true; - } - } - for (i = 1; i < argc; i++) { fprintf(log, "ARG:%s\n", argv[i]); } @@ -126,7 +155,7 @@ int main(int argc, char **argv) { while (true) { bool daemonized = getpgrp() != getppid(); - if (daemonize_check && !daemonized && daemonize_retries-- > 0) { + if (args->daemonize_check && !daemonized && daemonize_retries-- > 0) { usleep(100*1000); continue; } @@ -154,7 +183,7 @@ int main(int argc, char **argv) { fprintf(log, "UMASK:%04o\n", umask(0)); - if (close_stdin) { + if (args->close_stdin) { if (freopen("/dev/null", "r", stdin) != stdin) goto cleanup; usleep(100*1000); @@ -165,8 +194,8 @@ int main(int argc, char **argv) { fprintf(stderr, "BEGIN STDERR\n"); fflush(stderr); - for (i = 0; i < numreadfds; i++) { - fds[i].fd = readfds[i]; + for (i = 0; i < args->numreadfds; i++) { + fds[i].fd = args->readfds[i]; fds[i].events = POLLIN; fds[i].revents = 0; } @@ -174,12 +203,12 @@ int main(int argc, char **argv) { for (;;) { unsigned ctr = 0; - if (poll(fds, numreadfds, -1) < 0) { + if (poll(fds, args->numreadfds, -1) < 0) { printf("poll failed: %s\n", strerror(errno)); goto cleanup; } - for (i = 0; i < numreadfds; i++) { + for (i = 0; i < args->numreadfds; i++) { short revents = POLLIN | POLLHUP | POLLERR; # ifdef __APPLE__ @@ -210,7 +239,7 @@ int main(int argc, char **argv) { } } } - for (i = 0; i < numreadfds; i++) { + for (i = 0; i < args->numreadfds; i++) { if (fds[i].events) { ctr++; break; @@ -220,7 +249,7 @@ int main(int argc, char **argv) { break; } - for (i = 0; i < numreadfds; i++) { + for (i = 0; i < args->numreadfds; i++) { if (fwrite(buffers[i], 1, buflen[i], stdout) != buflen[i]) goto cleanup; if (fwrite(buffers[i], 1, buflen[i], stderr) != buflen[i]) @@ -237,6 +266,8 @@ int main(int argc, char **argv) { cleanup: for (i = 0; i < G_N_ELEMENTS(buffers); i++) free(buffers[i]); + if (args) + free(args); if (newenv) free(newenv); if (log) -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index f7e406ec83..ec54f55eb6 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -80,6 +80,15 @@ static struct Arguments *parseArguments(int argc, char** argv) return NULL; } +static void printArguments(FILE *log, int argc, char** argv) +{ + size_t i; + + for (i = 1; i < argc; i++) { + fprintf(log, "ARG:%s\n", argv[i]); + } +} + static int envsort(const void *a, const void *b) { const char *astr = *(const char**)a; @@ -115,9 +124,7 @@ int main(int argc, char **argv) { if (!log || !args) goto cleanup; - for (i = 1; i < argc; i++) { - fprintf(log, "ARG:%s\n", argv[i]); - } + printArguments(log, argc, argv); for (n = 0; environ[n]; n++) { } -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 57 +++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index ec54f55eb6..842cc72942 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -106,11 +106,44 @@ static int envsort(const void *a, const void *b) } } +static int printEnvironment(FILE *log) +{ + char **newenv; + size_t length; + size_t i; + int ret = -1; + + for (length = 0; environ[length]; length++) { + } + + if (!(newenv = malloc(sizeof(*newenv) * length))) + goto cleanup; + + for (i = 0; i < length; i++) { + newenv[i] = environ[i]; + } + + qsort(newenv, length, sizeof(newenv[0]), envsort); + + for (i = 0; i < length; i++) { + /* Ignore the variables used to instruct the loader into + * behaving differently, as they could throw the tests off. */ + if (!STRPREFIX(newenv[i], "LD_")) + fprintf(log, "ENV:%s\n", newenv[i]); + } + + ret = 0; + + cleanup: + if (newenv) + free(newenv); + return ret; +} + int main(int argc, char **argv) { struct Arguments *args = parseArguments(argc, argv); - size_t i, n; + size_t i; int open_max; - char **newenv = NULL; char *cwd; FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); int ret = EXIT_FAILURE; @@ -126,25 +159,9 @@ int main(int argc, char **argv) { printArguments(log, argc, argv); - for (n = 0; environ[n]; n++) { - } - - if (!(newenv = malloc(sizeof(*newenv) * n))) + if (printEnvironment(log) != 0) goto cleanup; - for (i = 0; i < n; i++) { - newenv[i] = environ[i]; - } - - qsort(newenv, n, sizeof(newenv[0]), envsort); - - for (i = 0; i < n; i++) { - /* Ignore the variables used to instruct the loader into - * behaving differently, as they could throw the tests off. */ - if (!STRPREFIX(newenv[i], "LD_")) - fprintf(log, "ENV:%s\n", newenv[i]); - } - open_max = sysconf(_SC_OPEN_MAX); if (open_max < 0) goto cleanup; @@ -275,8 +292,6 @@ int main(int argc, char **argv) { free(buffers[i]); if (args) free(args); - if (newenv) - free(newenv); if (log) fclose(log); return ret; -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 842cc72942..fd45c4fdf7 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -140,10 +140,32 @@ static int printEnvironment(FILE *log) return ret; } +static int printFds(FILE *log) +{ + long int open_max = sysconf(_SC_OPEN_MAX); + size_t i; + + if (open_max < 0) + return -1; + + for (i = 0; i < open_max; i++) { + int ignore; + + if (i == fileno(log)) + continue; + + if (fcntl(i, F_GETFD, &ignore) == -1 && errno == EBADF) + continue; + + fprintf(log, "FD:%zu\n", i); + } + + return 0; +} + int main(int argc, char **argv) { struct Arguments *args = parseArguments(argc, argv); size_t i; - int open_max; char *cwd; FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); int ret = EXIT_FAILURE; @@ -162,19 +184,8 @@ int main(int argc, char **argv) { if (printEnvironment(log) != 0) goto cleanup; - open_max = sysconf(_SC_OPEN_MAX); - if (open_max < 0) + if (printFds(log) != 0) goto cleanup; - for (i = 0; i < open_max; i++) { - int f; - int closed; - if (i == fileno(log)) - continue; - closed = fcntl(i, F_GETFD, &f) == -1 && - errno == EBADF; - if (!closed) - fprintf(log, "FD:%zu\n", i); - } while (true) { bool daemonized = getpgrp() != getppid(); -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index fd45c4fdf7..cceeb1a119 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -163,6 +163,19 @@ static int printFds(FILE *log) return 0; } +static void printDaemonization(FILE *log, struct Arguments *args) +{ + int retries = 3; + + if (args->daemonize_check) { + while ((getpgrp() == getppid()) && (retries-- > 0)) { + usleep(100 * 1000); + } + } + + fprintf(log, "DAEMON:%s\n", getpgrp() != getppid() ? "yes" : "no"); +} + int main(int argc, char **argv) { struct Arguments *args = parseArguments(argc, argv); size_t i; @@ -172,7 +185,6 @@ int main(int argc, char **argv) { struct pollfd fds[3]; char *buffers[3] = {NULL, NULL, NULL}; size_t buflen[3] = {0, 0, 0}; - size_t daemonize_retries = 3; char buf[1024]; ssize_t got; @@ -187,17 +199,7 @@ int main(int argc, char **argv) { if (printFds(log) != 0) goto cleanup; - while (true) { - bool daemonized = getpgrp() != getppid(); - - if (args->daemonize_check && !daemonized && daemonize_retries-- > 0) { - usleep(100*1000); - continue; - } - - fprintf(log, "DAEMON:%s\n", daemonized ? "yes" : "no"); - break; - } + printDaemonization(log, args); if (!(cwd = getcwd(NULL, 0))) goto cleanup; -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index cceeb1a119..ce3f64fc9d 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -176,10 +176,34 @@ static void printDaemonization(FILE *log, struct Arguments *args) fprintf(log, "DAEMON:%s\n", getpgrp() != getppid() ? "yes" : "no"); } +static int printCwd(FILE *log) +{ + char *cwd = NULL; + char *display; + + if (!(cwd = getcwd(NULL, 0))) + return -1; + + if ((strlen(cwd) > strlen(".../commanddata")) && + (STREQ(cwd + strlen(cwd) - strlen("/commanddata"), "/commanddata"))) { + strcpy(cwd, ".../commanddata"); + } + + display = cwd; + +# ifdef __APPLE__ + if (strstr(cwd, "/private")) + display = cwd + strlen("/private"); +# endif + + fprintf(log, "CWD:%s\n", display); + free(cwd); + return 0; +} + int main(int argc, char **argv) { struct Arguments *args = parseArguments(argc, argv); size_t i; - char *cwd; FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); int ret = EXIT_FAILURE; struct pollfd fds[3]; @@ -201,22 +225,8 @@ int main(int argc, char **argv) { printDaemonization(log, args); - if (!(cwd = getcwd(NULL, 0))) + if (printCwd(log) != 0) goto cleanup; - if (strlen(cwd) > strlen(".../commanddata") && - STREQ(cwd + strlen(cwd) - strlen("/commanddata"), "/commanddata")) - strcpy(cwd, ".../commanddata"); -# ifdef __APPLE__ - char *noprivateprefix = NULL; - if (strstr(cwd, "/private")) - noprivateprefix = cwd + strlen("/private"); - else - noprivateprefix = cwd; - fprintf(log, "CWD:%s\n", noprivateprefix); -# else - fprintf(log, "CWD:%s\n", cwd); -# endif - free(cwd); fprintf(log, "UMASK:%04o\n", umask(0)); -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandhelper.c | 65 ++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index ce3f64fc9d..8a9a3c96a0 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -201,39 +201,20 @@ static int printCwd(FILE *log) return 0; } -int main(int argc, char **argv) { - struct Arguments *args = parseArguments(argc, argv); - size_t i; - FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); - int ret = EXIT_FAILURE; +static int printInput(struct Arguments *args) +{ + char buf[1024]; struct pollfd fds[3]; char *buffers[3] = {NULL, NULL, NULL}; size_t buflen[3] = {0, 0, 0}; - char buf[1024]; + int ret = -1; + size_t i; ssize_t got; - if (!log || !args) - goto cleanup; - - printArguments(log, argc, argv); - - if (printEnvironment(log) != 0) - goto cleanup; - - if (printFds(log) != 0) - goto cleanup; - - printDaemonization(log, args); - - if (printCwd(log) != 0) - goto cleanup; - - fprintf(log, "UMASK:%04o\n", umask(0)); - if (args->close_stdin) { if (freopen("/dev/null", "r", stdin) != stdin) goto cleanup; - usleep(100*1000); + usleep(100 * 1000); } fprintf(stdout, "BEGIN STDOUT\n"); @@ -308,11 +289,43 @@ int main(int argc, char **argv) { fprintf(stderr, "END STDERR\n"); fflush(stderr); - ret = EXIT_SUCCESS; + ret = 0; cleanup: for (i = 0; i < G_N_ELEMENTS(buffers); i++) free(buffers[i]); + return ret; +} + +int main(int argc, char **argv) { + struct Arguments *args = parseArguments(argc, argv); + FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); + int ret = EXIT_FAILURE; + + if (!log || !args) + goto cleanup; + + printArguments(log, argc, argv); + + if (printEnvironment(log) != 0) + goto cleanup; + + if (printFds(log) != 0) + goto cleanup; + + printDaemonization(log, args); + + if (printCwd(log) != 0) + goto cleanup; + + fprintf(log, "UMASK:%04o\n", umask(0)); + + if (printInput(args) != 0) + goto cleanup; + + ret = EXIT_SUCCESS; + + cleanup: if (args) free(args); if (log) -- 2.26.2
Fixes a buffer overflow triggered when more than three "--readfd" arguments were given on the command line. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 8a9a3c96a0..ac64505461 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -204,13 +204,23 @@ static int printCwd(FILE *log) static int printInput(struct Arguments *args) { char buf[1024]; - struct pollfd fds[3]; - char *buffers[3] = {NULL, NULL, NULL}; - size_t buflen[3] = {0, 0, 0}; + struct pollfd *fds = NULL; + char **buffers = NULL; + size_t *buflen = NULL; int ret = -1; size_t i; ssize_t got; + if (!(fds = calloc(args->numreadfds, sizeof(*fds)))) + goto cleanup; + + /* plus one NULL terminator */ + if (!(buffers = calloc(args->numreadfds + 1, sizeof(*buffers)))) + goto cleanup; + + if (!(buflen = calloc(args->numreadfds, sizeof(*buflen)))) + goto cleanup; + if (args->close_stdin) { if (freopen("/dev/null", "r", stdin) != stdin) goto cleanup; @@ -292,8 +302,15 @@ static int printInput(struct Arguments *args) ret = 0; cleanup: - for (i = 0; i < G_N_ELEMENTS(buffers); i++) - free(buffers[i]); + if (buffers) { + char **ptr; + for (ptr = buffers; *ptr; ptr++) + free(*ptr); + } + free(fds); + free(buflen); + free(buffers); + return ret; } -- 2.26.2
Fixes a buffer overflow triggered when more than three "--readfd" arguments were given on the command line. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index ac64505461..9f0b7f25ac 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -36,7 +36,7 @@ extern char **environ; # define VIR_FROM_THIS VIR_FROM_NONE struct Arguments { - int readfds[3]; + int *readfds; int numreadfds; bool daemonize_check; bool close_stdin; @@ -51,6 +51,9 @@ static struct Arguments *parseArguments(int argc, char** argv) if (!(args = calloc(1, sizeof(*args)))) goto cleanup; + if (!(args->readfds = calloc(1, sizeof(*args->readfds)))) + goto cleanup; + args->numreadfds = 1; args->readfds[0] = STDIN_FILENO; @@ -58,6 +61,12 @@ static struct Arguments *parseArguments(int argc, char** argv) if (STREQ(argv[i - 1], "--readfd")) { char c; + args->readfds = realloc(args->readfds, + (args->numreadfds + 1) * + sizeof(*args->readfds)); + if (!args->readfds) + goto cleanup; + if (1 != sscanf(argv[i], "%u%c", &args->readfds[args->numreadfds++], &c)) { printf("Could not parse fd %s\n", argv[i]); @@ -76,7 +85,12 @@ static struct Arguments *parseArguments(int argc, char** argv) if (ret == 0) return args; - free(args); + if (args) { + if (args->readfds) + free(args->readfds); + free(args); + } + return NULL; } @@ -343,8 +357,11 @@ int main(int argc, char **argv) { ret = EXIT_SUCCESS; cleanup: - if (args) + if (args) { + if (args->readfds) + free(args->readfds); free(args); + } if (log) fclose(log); return ret; -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 9f0b7f25ac..19dfc09151 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -24,7 +24,9 @@ #include <fcntl.h> #include <sys/stat.h> -#define VIR_NO_GLIB_STDIO /* This file intentionally does not link to libvirt/glib */ +/* This file intentionally does not link to libvirt/glib */ +#define VIR_NO_GLIB_STDIO +#define cleanup(T, F) __attribute__((cleanup(F))) T #include "testutils.h" #ifndef WIN32 @@ -42,17 +44,27 @@ struct Arguments { bool close_stdin; }; +static void cleanupArguments(struct Arguments **ptr) +{ + struct Arguments *args = *ptr; + + if (args) + free(args->readfds); + + free(args); +} + static struct Arguments *parseArguments(int argc, char** argv) { - struct Arguments* args = NULL; - int ret = -1; + cleanup(struct Arguments *, cleanupArguments) args = NULL; + struct Arguments *ret; size_t i; if (!(args = calloc(1, sizeof(*args)))) - goto cleanup; + return NULL; if (!(args->readfds = calloc(1, sizeof(*args->readfds)))) - goto cleanup; + return NULL; args->numreadfds = 1; args->readfds[0] = STDIN_FILENO; @@ -65,12 +77,12 @@ static struct Arguments *parseArguments(int argc, char** argv) (args->numreadfds + 1) * sizeof(*args->readfds)); if (!args->readfds) - goto cleanup; + return NULL; if (1 != sscanf(argv[i], "%u%c", &args->readfds[args->numreadfds++], &c)) { printf("Could not parse fd %s\n", argv[i]); - goto cleanup; + return NULL; } } else if (STREQ(argv[i], "--check-daemonize")) { args->daemonize_check = true; @@ -79,19 +91,9 @@ static struct Arguments *parseArguments(int argc, char** argv) } } - ret = 0; - - cleanup: - if (ret == 0) - return args; - - if (args) { - if (args->readfds) - free(args->readfds); - free(args); - } - - return NULL; + ret = args; + args = NULL; + return ret; } static void printArguments(FILE *log, int argc, char** argv) -- 2.26.2
On Mon, Feb 01, 2021 at 12:28:00 +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 9f0b7f25ac..19dfc09151 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -24,7 +24,9 @@ #include <fcntl.h> #include <sys/stat.h>
-#define VIR_NO_GLIB_STDIO /* This file intentionally does not link to libvirt/glib */ +/* This file intentionally does not link to libvirt/glib */ +#define VIR_NO_GLIB_STDIO
Spurious line break.
On Tue, 2021-02-02 at 14:59 +0100, Peter Krempa wrote:
On Mon, Feb 01, 2021 at 12:28:00 +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 42 ++++++++++++++++++++++----------------- --- 1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 9f0b7f25ac..19dfc09151 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -24,7 +24,9 @@ #include <fcntl.h> #include <sys/stat.h>
-#define VIR_NO_GLIB_STDIO /* This file intentionally does not link to libvirt/glib */ +/* This file intentionally does not link to libvirt/glib */ +#define VIR_NO_GLIB_STDIO
Spurious line break.
Intentional. The comment now refers to not only the "#define VIR_NO_GLIB_STDIO" but also serves as a justification for the custom cleanup macro that otherwise should be replaced by some glib g_* construct.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 19dfc09151..854f3c09bf 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -54,6 +54,12 @@ static void cleanupArguments(struct Arguments **ptr) free(args); } +static void cleanupGeneric(void *ptr) +{ + void **ptrptr = ptr; + free (*ptrptr); +} + static struct Arguments *parseArguments(int argc, char** argv) { cleanup(struct Arguments *, cleanupArguments) args = NULL; @@ -124,16 +130,15 @@ static int envsort(const void *a, const void *b) static int printEnvironment(FILE *log) { - char **newenv; + cleanup(char **, cleanupGeneric) newenv = NULL; size_t length; size_t i; - int ret = -1; for (length = 0; environ[length]; length++) { } if (!(newenv = malloc(sizeof(*newenv) * length))) - goto cleanup; + return -1; for (i = 0; i < length; i++) { newenv[i] = environ[i]; @@ -148,12 +153,7 @@ static int printEnvironment(FILE *log) fprintf(log, "ENV:%s\n", newenv[i]); } - ret = 0; - - cleanup: - if (newenv) - free(newenv); - return ret; + return 0; } static int printFds(FILE *log) -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 854f3c09bf..5f0c1f5a51 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -194,7 +194,7 @@ static void printDaemonization(FILE *log, struct Arguments *args) static int printCwd(FILE *log) { - char *cwd = NULL; + cleanup(char *, cleanupGeneric) cwd = NULL; char *display; if (!(cwd = getcwd(NULL, 0))) @@ -213,7 +213,6 @@ static int printCwd(FILE *log) # endif fprintf(log, "CWD:%s\n", display); - free(cwd); return 0; } -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 52 +++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 5f0c1f5a51..df9be7d424 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -54,6 +54,19 @@ static void cleanupArguments(struct Arguments **ptr) free(args); } +static void cleanupStringList(char ***ptr) +{ + char **strings = *ptr; + + if (strings) { + char **str; + for (str = strings; *str; str++) + free(*str); + } + + free(strings); +} + static void cleanupGeneric(void *ptr) { void **ptrptr = ptr; @@ -219,26 +232,25 @@ static int printCwd(FILE *log) static int printInput(struct Arguments *args) { char buf[1024]; - struct pollfd *fds = NULL; - char **buffers = NULL; - size_t *buflen = NULL; - int ret = -1; + cleanup(struct pollfd *, cleanupGeneric) fds = NULL; + cleanup(char **, cleanupStringList) buffers = NULL; + cleanup(size_t *, cleanupGeneric) buflen = NULL; size_t i; ssize_t got; if (!(fds = calloc(args->numreadfds, sizeof(*fds)))) - goto cleanup; + return -1; /* plus one NULL terminator */ if (!(buffers = calloc(args->numreadfds + 1, sizeof(*buffers)))) - goto cleanup; + return -1; if (!(buflen = calloc(args->numreadfds, sizeof(*buflen)))) - goto cleanup; + return -1; if (args->close_stdin) { if (freopen("/dev/null", "r", stdin) != stdin) - goto cleanup; + return -1; usleep(100 * 1000); } @@ -258,7 +270,7 @@ static int printInput(struct Arguments *args) if (poll(fds, args->numreadfds, -1) < 0) { printf("poll failed: %s\n", strerror(errno)); - goto cleanup; + return -1; } for (i = 0; i < args->numreadfds; i++) { @@ -277,7 +289,7 @@ static int printInput(struct Arguments *args) got = read(fds[i].fd, buf, sizeof(buf)); if (got < 0) - goto cleanup; + return -1; if (got == 0) { /* do not want to hear from this fd anymore */ fds[i].events = 0; @@ -285,7 +297,7 @@ static int printInput(struct Arguments *args) buffers[i] = realloc(buffers[i], buflen[i] + got); if (!buf[i]) { fprintf(stdout, "Out of memory!\n"); - goto cleanup; + return -1; } memcpy(buffers[i] + buflen[i], buf, got); buflen[i] += got; @@ -304,9 +316,9 @@ static int printInput(struct Arguments *args) for (i = 0; i < args->numreadfds; i++) { if (fwrite(buffers[i], 1, buflen[i], stdout) != buflen[i]) - goto cleanup; + return -1; if (fwrite(buffers[i], 1, buflen[i], stderr) != buflen[i]) - goto cleanup; + return -1; } fprintf(stdout, "END STDOUT\n"); @@ -314,19 +326,7 @@ static int printInput(struct Arguments *args) fprintf(stderr, "END STDERR\n"); fflush(stderr); - ret = 0; - - cleanup: - if (buffers) { - char **ptr; - for (ptr = buffers; *ptr; ptr++) - free(*ptr); - } - free(fds); - free(buflen); - free(buffers); - - return ret; + return 0; } int main(int argc, char **argv) { -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index df9be7d424..bf6a5baa40 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -67,6 +67,12 @@ static void cleanupStringList(char ***ptr) free(strings); } +static void cleanupFile(FILE **ptr) +{ + FILE *file = *ptr; + fclose(file); +} + static void cleanupGeneric(void *ptr) { void **ptrptr = ptr; @@ -330,42 +336,34 @@ static int printInput(struct Arguments *args) } int main(int argc, char **argv) { - struct Arguments *args = parseArguments(argc, argv); - FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); - int ret = EXIT_FAILURE; + cleanup(struct Arguments *, cleanupArguments) args = NULL; + cleanup(FILE *, cleanupFile) log = NULL; - if (!log || !args) - goto cleanup; + if (!(log = fopen(abs_builddir "/commandhelper.log", "w"))) + return EXIT_FAILURE; + + if (!(args = parseArguments(argc, argv))) + return EXIT_FAILURE; printArguments(log, argc, argv); if (printEnvironment(log) != 0) - goto cleanup; + return EXIT_FAILURE; if (printFds(log) != 0) - goto cleanup; + return EXIT_FAILURE; printDaemonization(log, args); if (printCwd(log) != 0) - goto cleanup; + return EXIT_FAILURE; fprintf(log, "UMASK:%04o\n", umask(0)); if (printInput(args) != 0) - goto cleanup; - - ret = EXIT_SUCCESS; + return EXIT_FAILURE; - cleanup: - if (args) { - if (args->readfds) - free(args->readfds); - free(args); - } - if (log) - fclose(log); - return ret; + return EXIT_SUCCESS; } #else -- 2.26.2
participants (2)
-
Peter Krempa -
Tim Wiederhake