[libvirt PATCH 00/19] 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, fixes the buffer overflow and converts (most) memory handling to g_auto*. 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. Please feel free to comment on whether the copyright year in the file's header should be updated and whether a prefix for the function names and the new type is required. Cheers, Tim Tim Wiederhake (19): 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: Convert parseArguments to g_auto* commandhelper: Convert printEnvironment to g_auto* commandhelper: Convert printCwd to g_auto* commandhelper: Convert main to g_auto* tests/commandhelper.c | 295 +++++++++++++++++++++++++++--------------- 1 file changed, 188 insertions(+), 107 deletions(-) -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@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> --- 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
Comparing only the keys produces the same result as comparing keys and value. The latter saves two invocations of each `strndup` and `free`. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 2b937979c0..05e3879688 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -39,22 +39,7 @@ 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; + return strcmp(*astrptr, *bstrptr); } int main(int argc, char **argv) { -- 2.26.2
On Fri, Jan 29, 2021 at 17:16:13 +0100, Tim Wiederhake wrote:
Comparing only the keys produces the same result as comparing keys and value. The latter saves two invocations of each `strndup` and `free`.
env variable names allow also ascii chars with lower index than '=' such as numbers: $ TEST=asdf TEST1=asdf env | grep TEST | LANG=C sort TEST1=asdf TEST=asdf $ TEST=asdf TEST1=asdf env | grep TEST | cut -d "=" -f 1 | LANG=C sort TEST TEST1
Preparation for later conversion to g_auto* memory handling. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 05e3879688..2be121ce2c 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -61,7 +61,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]); @@ -79,7 +79,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]; @@ -222,8 +222,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
On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
Preparation for later conversion to g_auto* memory handling.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 05e3879688..2be121ce2c 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -61,7 +61,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]); @@ -79,7 +79,7 @@ int main(int argc, char **argv) { }
if (!(newenv = malloc(sizeof(*newenv) * n))) - abort(); + goto cleanup;
Any reason for not converting this malloc to g_new directly? you get rid of abort()/cleanup entirely. Especially since the patches at the end of the series switch to g_auto(ptr). If there's a strong reason against using glibs allocators, in such case the cleanups shouldn't be added either.
for (i = 0; i < n; i++) { newenv[i] = environ[i]; @@ -222,8 +222,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
On Fri, Jan 29, 2021 at 18:17:36 +0100, Peter Krempa wrote:
On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
Preparation for later conversion to g_auto* memory handling.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 05e3879688..2be121ce2c 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -61,7 +61,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]); @@ -79,7 +79,7 @@ int main(int argc, char **argv) { }
if (!(newenv = malloc(sizeof(*newenv) * n))) - abort(); + goto cleanup;
Any reason for not converting this malloc to g_new directly? you get rid of abort()/cleanup entirely.
Especially since the patches at the end of the series switch to g_auto(ptr).
If there's a strong reason against using glibs allocators, in such case the cleanups shouldn't be added either.
Using glibs allocators would simplify also further patches, so if there isn't a particular reason why you chose to use calloc it would seem better to use g_new right away and prevent adding additional cleanup labels.
On Fri, Jan 29, 2021 at 06:17:36PM +0100, Peter Krempa wrote:
On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
Preparation for later conversion to g_auto* memory handling.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 05e3879688..2be121ce2c 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -61,7 +61,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]); @@ -79,7 +79,7 @@ int main(int argc, char **argv) { }
if (!(newenv = malloc(sizeof(*newenv) * n))) - abort(); + goto cleanup;
Any reason for not converting this malloc to g_new directly? you get rid of abort()/cleanup entirely.
commandhelper isn't permitted to link to glib, because we need a clean execution environment for counting FDs.
Especially since the patches at the end of the series switch to g_auto(ptr).
If there's a strong reason against using glibs allocators, in such case the cleanups shouldn't be added either.
We get away with the g_auto usage because its merely a macro Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jan 29, 2021 at 17:52:35 +0000, Daniel Berrange wrote:
On Fri, Jan 29, 2021 at 06:17:36PM +0100, Peter Krempa wrote:
On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
Preparation for later conversion to g_auto* memory handling.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 05e3879688..2be121ce2c 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -61,7 +61,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]); @@ -79,7 +79,7 @@ int main(int argc, char **argv) { }
if (!(newenv = malloc(sizeof(*newenv) * n))) - abort(); + goto cleanup;
Any reason for not converting this malloc to g_new directly? you get rid of abort()/cleanup entirely.
commandhelper isn't permitted to link to glib, because we need a clean execution environment for counting FDs.
Especially since the patches at the end of the series switch to g_auto(ptr).
If there's a strong reason against using glibs allocators, in such case the cleanups shouldn't be added either.
We get away with the g_auto usage because its merely a macro
Hmm, yeah. It looks a bit weird though.
On Fri, Jan 29, 2021 at 17:52:35 +0000, Daniel Berrange wrote:
On Fri, Jan 29, 2021 at 06:17:36PM +0100, Peter Krempa wrote:
On Fri, Jan 29, 2021 at 17:16:14 +0100, Tim Wiederhake wrote:
Preparation for later conversion to g_auto* memory handling.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 05e3879688..2be121ce2c 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -61,7 +61,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]); @@ -79,7 +79,7 @@ int main(int argc, char **argv) { }
if (!(newenv = malloc(sizeof(*newenv) * n))) - abort(); + goto cleanup;
Any reason for not converting this malloc to g_new directly? you get rid of abort()/cleanup entirely.
commandhelper isn't permitted to link to glib, because we need a clean execution environment for counting FDs.
I must say, that switching to meson made this quite fragile. Prior to this series I see: $ ldd ~/build/libvirt/gcc/tests/commandhelper linux-vdso.so.1 (0x00007ffefbf04000) libc.so.6 => /lib64/libc.so.6 (0x00007f40644bb000) /lib64/ld-linux-x86-64.so.2 (0x00007f40646ab000) Now this series touches nothing related to build system and linking, yet commiting these patches would result in: $ ldd ~/build/libvirt/gcc/tests/commandhelper linux-vdso.so.1 (0x00007ffdeeda6000) libglib-2.0.so.0 => /lib64/libglib-2.0.so.0 (0x00007fc5190e5000) libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fc5190ca000) libc.so.6 => /lib64/libc.so.6 (0x00007fc518eff000) libpcre.so.1 => /lib64/libpcre.so.1 (0x00007fc518e86000) libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fc518e64000) /lib64/ld-linux-x86-64.so.2 (0x00007fc51923c000) If patches 17-18 are left out we get: $ ldd ~/build/libvirt/gcc/tests/commandhelper linux-vdso.so.1 (0x00007ffd9bfb7000) libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fc728358000) libc.so.6 => /lib64/libc.so.6 (0x00007fc72818d000) /lib64/ld-linux-x86-64.so.2 (0x00007fc728398000) (libgcc_s added) Pulling in deps automatically for stuff which is deliberately avoiding some libraries is counterproductive here. I hope same doesn't happen with some of the setuid binaries where we deliberately avoid libvirt.so
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 2be121ce2c..a964420d81 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -56,6 +56,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; @@ -72,6 +73,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; } } @@ -139,7 +142,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> --- tests/commandhelper.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index a964420d81..a77bee931f 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -65,8 +65,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]); @@ -78,6 +76,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> --- tests/commandhelper.c | 83 +++++++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 26 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index a77bee931f..c44322502f 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 *const*astrptr = a; @@ -43,39 +88,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]); } @@ -116,7 +145,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; } @@ -144,7 +173,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); @@ -155,8 +184,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; } @@ -164,12 +193,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__ @@ -200,7 +229,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; @@ -210,7 +239,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]) @@ -227,6 +256,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> --- tests/commandhelper.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index c44322502f..aa346f1dfd 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 *const*astrptr = a; @@ -105,9 +114,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> --- tests/commandhelper.c | 57 +++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index aa346f1dfd..14c7302633 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -96,11 +96,44 @@ static int envsort(const void *a, const void *b) return strcmp(*astrptr, *bstrptr); } +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; @@ -116,25 +149,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; @@ -265,8 +282,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> --- tests/commandhelper.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 14c7302633..fa32f6a435 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -130,10 +130,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; @@ -152,19 +174,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> --- tests/commandhelper.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index fa32f6a435..01dd6f9e45 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -153,6 +153,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; @@ -162,7 +175,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; @@ -177,17 +189,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> --- tests/commandhelper.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 01dd6f9e45..929de7a05d 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -166,10 +166,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]; @@ -191,22 +215,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> --- tests/commandhelper.c | 65 ++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 929de7a05d..d501e33e88 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -191,39 +191,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"); @@ -298,11 +279,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 | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index d501e33e88..72a3e89da1 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -194,13 +194,22 @@ 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; + + if (!(buffers = calloc(args->numreadfds, 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; @@ -282,8 +291,14 @@ static int printInput(struct Arguments *args) ret = 0; cleanup: - for (i = 0; i < G_N_ELEMENTS(buffers); i++) - free(buffers[i]); + if (buffers) { + for (i = 0; i < args->numreadfds; i++) + free(buffers[i]); + } + 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 | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 72a3e89da1..6d5fe04042 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; } -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 6d5fe04042..e616f92987 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -35,24 +35,32 @@ extern char **environ; # define VIR_FROM_THIS VIR_FROM_NONE -struct Arguments { +typedef struct Arguments { int *readfds; int numreadfds; bool daemonize_check; bool close_stdin; -}; +} Arguments; + +static void cleanupArguments(struct Arguments* args) { + if (args) + free(args->readfds); + + free(args); +} + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Arguments, cleanupArguments); static struct Arguments *parseArguments(int argc, char** argv) { - struct Arguments* args = NULL; - int ret = -1; + g_autoptr(Arguments) args = NULL; 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 +73,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 +87,7 @@ 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; + return g_steal_pointer(&args); } static void printArguments(FILE *log, int argc, char** argv) -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index e616f92987..26a7de5149 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -108,16 +108,15 @@ static int envsort(const void *a, const void *b) static int printEnvironment(FILE *log) { - char **newenv; + g_autofree char **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]; @@ -132,12 +131,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
On Fri, Jan 29, 2021 at 17:16:27 +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index e616f92987..26a7de5149 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -108,16 +108,15 @@ static int envsort(const void *a, const void *b)
static int printEnvironment(FILE *log) { - char **newenv; + g_autofree char **newenv = NULL;
Per discussion on 4/19, this isn't allowed to link to glib, thus g_autofree should not work as it uses g_free internally.
On Fri, Jan 29, 2021 at 07:08:27PM +0100, Peter Krempa wrote:
On Fri, Jan 29, 2021 at 17:16:27 +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index e616f92987..26a7de5149 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -108,16 +108,15 @@ static int envsort(const void *a, const void *b)
static int printEnvironment(FILE *log) { - char **newenv; + g_autofree char **newenv = NULL;
Per discussion on 4/19, this isn't allowed to link to glib, thus g_autofree should not work as it uses g_free internally.
I guess this is my mistake in the meson rewrite. Specifically commit 8fe8df4b2026ca21db123ddc0cdd8451a2e224dd where originally it did not link to anything but with meson it now can link with a lot of libraries specified in tests_dep. That probably needs to be fixed to make it work as expected. Pavel
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 26a7de5149..3bd7d2b28d 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -172,7 +172,7 @@ static void printDaemonization(FILE *log, struct Arguments *args) static int printCwd(FILE *log) { - char *cwd = NULL; + g_autofree char *cwd = NULL; char *display; if (!(cwd = getcwd(NULL, 0))) @@ -191,7 +191,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 | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 3bd7d2b28d..940a979a3f 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -306,39 +306,31 @@ 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; + g_autoptr(Arguments) args = parseArguments(argc, argv); + g_autoptr(FILE) log = fopen(abs_builddir "/commandhelper.log", "w"); if (!log || !args) - goto cleanup; + 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; + return EXIT_FAILURE; - ret = EXIT_SUCCESS; - - cleanup: - if (args) - free(args); - if (log) - fclose(log); - return ret; + return EXIT_SUCCESS; } #else -- 2.26.2
On Fri, Jan 29, 2021 at 17:16:10 +0100, Tim Wiederhake wrote:
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, fixes the buffer overflow and converts (most) memory handling to g_auto*.
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.
Please feel free to comment on whether the copyright year in the file's header should be updated and whether a prefix for the function names and the new type is required.
No and no. For patches 1-2,4-16,19: Reviewed-by: Peter Krempa <pkrempa@redhat.com> 17, 18 use g_autofree which uses g_free which shouldn't be available.
participants (4)
-
Daniel P. Berrangé -
Pavel Hrdina -
Peter Krempa -
Tim Wiederhake