10:16 a.m.
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
10:16 a.m.
New subject: [libvirt PATCH 01/19] commandhelper: Remove origenv variable
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 02/19] commandhelper: Remove numpollfds variable
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 03/19] commandhelper: Simplify envsort
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(a)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
10:50 a.m.
New subject: [libvirt PATCH 03/19] commandhelper: Simplify envsort
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
10:16 a.m.
New subject: [libvirt PATCH 04/19] commandhelper: Consolidate error paths
Preparation for later conversion to g_auto* memory handling.
Signed-off-by: Tim Wiederhake <twiederh(a)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
11:17 a.m.
New subject: [libvirt PATCH 04/19] commandhelper: Consolidate error paths
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(a)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
11:28 a.m.
New subject: [libvirt PATCH 04/19] commandhelper: Consolidate error paths
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(a)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.
11:52 a.m.
New subject: [libvirt PATCH 04/19] commandhelper: Consolidate error paths
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(a)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 :|
12:06 p.m.
New subject: [libvirt PATCH 04/19] commandhelper: Consolidate error paths
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(a)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.
12:19 p.m.
New subject: [libvirt PATCH 04/19] commandhelper: Consolidate error paths
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(a)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
10:16 a.m.
New subject: [libvirt PATCH 05/19] commandhelper: Consolidate argument parsing
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 06/19] commandhelper: Split argument parsing and printing
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 07/19] commandhelper: Factor out parseArguments
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 08/19] commandhelper: Factor out printArguments
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 09/19] commandhelper: Factor out printEnvironment
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 10/19] commandhelper: Factor out printFds
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 11/19] commandhelper: Factor out printDaemonization
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 12/19] commandhelper: Factor out printCwd
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 13/19] commandhelper: Factor out printInput
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 14/19] commandhelper: Make number of fds variable in printInput
Fixes a buffer overflow triggered when more than three "--readfd"
arguments were given on the command line.
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 15/19] commandhelper: Make number of fds variable in parseArguments
Fixes a buffer overflow triggered when more than three "--readfd"
arguments were given on the command line.
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 16/19] commandhelper: Convert parseArguments to g_auto*
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 17/19] commandhelper: Convert printEnvironment to g_auto*
Signed-off-by: Tim Wiederhake <twiederh(a)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
12:08 p.m.
New subject: [libvirt PATCH 17/19] commandhelper: Convert printEnvironment to g_auto*
On Fri, Jan 29, 2021 at 17:16:27 +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh(a)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.
12:22 p.m.
New subject: [libvirt PATCH 17/19] commandhelper: Convert printEnvironment to g_auto*
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(a)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
10:16 a.m.
New subject: [libvirt PATCH 18/19] commandhelper: Convert printCwd to g_auto*
Signed-off-by: Tim Wiederhake <twiederh(a)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
10:16 a.m.
New subject: [libvirt PATCH 19/19] commandhelper: Convert main to g_auto*
Signed-off-by: Tim Wiederhake <twiederh(a)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
12:11 p.m.
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(a)redhat.com>
17, 18 use g_autofree which uses g_free which shouldn't be available.
1426
days inactive
1426
days old
28 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Pavel Hrdina -
Peter Krempa -
Tim Wiederhake