[PATCH nbdkit 2/2] server: Avoid modifying argv by saving keys in a list and freeing on exit.
Unfortunately you cannot restore argv by setting *p = '=' :-( The reason is we advertize that plugins are allowed to save they ‘const char *key’ pointer passed to them in .config, but assigning *p = '=' changes the key string from "key" back to "key=value". Surprisingly only test-eval.sh actually broke, but other plugins are undoubtedly affected. My alternate fix is fairly horrible, but passes all the tests and valgrind. Rich.
This reverts commit ea4bb3dde2823231eeae1b5408e1bfadf8b95267. --- server/main.c | 1 - 1 file changed, 1 deletion(-) diff --git a/server/main.c b/server/main.c index a1d307b1..dbeca624 100644 --- a/server/main.c +++ b/server/main.c @@ -621,7 +621,6 @@ main (int argc, char *argv[]) if (p && is_config_key (argv[optind], p - argv[optind])) { /* key=value */ *p = '\0'; top->config (top, argv[optind], p+1); - *p = '='; } else if (magic_config_key == NULL) { if (i == 0) /* magic script parameter */ -- 2.24.1
--- server/main.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/server/main.c b/server/main.c index dbeca624..c65c78f7 100644 --- a/server/main.c +++ b/server/main.c @@ -614,13 +614,32 @@ main (int argc, char *argv[]) * scripting languages, if magic_config_key == NULL then if the * first parameter is bare it is prefixed with the key "script", and * any other bare parameters are errors. + * + * Keys must live for the life of nbdkit. Since we want to avoid + * modifying argv (so that /proc/PID/cmdline remains sane) but we + * need to create a key from argv[i] = "key=value" we must save the + * keys in an array which is freed at the end of main(). */ + char **keys = calloc (argc, sizeof (char *)); + if (keys == NULL) { + perror ("calloc"); + exit (EXIT_FAILURE); + } + magic_config_key = top->magic_config_key (top); for (i = 0; optind < argc; ++i, ++optind) { + size_t n; + p = strchr (argv[optind], '='); - if (p && is_config_key (argv[optind], p - argv[optind])) { /* key=value */ - *p = '\0'; - top->config (top, argv[optind], p+1); + n = p - argv[optind]; + if (p && is_config_key (argv[optind], n)) { /* Is it key=value? */ + keys[optind] = strndup (argv[optind], n); + + if (keys[optind] == NULL) { + perror ("strndup"); + exit (EXIT_FAILURE); + } + top->config (top, keys[optind], p+1); } else if (magic_config_key == NULL) { if (i == 0) /* magic script parameter */ @@ -679,6 +698,10 @@ main (int argc, char *argv[]) crypto_free (); close_quit_pipe (); + for (i = 1; i < argc; ++i) + free (keys[i]); + free (keys); + /* Note: Don't exit here, otherwise this won't work when compiled * for libFuzzer. */ -- 2.24.1
On Tue, Feb 18, 2020 at 10:52:19AM +0000, Richard W.M. Jones wrote:
Unfortunately you cannot restore argv by setting *p = '=' :-(
The reason is we advertize that plugins are allowed to save they ‘const char *key’ pointer passed to them in .config, but assigning *p = '=' changes the key string from "key" back to "key=value".
Surprisingly only test-eval.sh actually broke, but other plugins are undoubtedly affected.
Most plugins only compare key to some string, but the eval plugin actually saves the key which is why it fails. I have pushed the revert commit (part 1). Rich.
My alternate fix is fairly horrible, but passes all the tests and valgrind.
Rich.
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 2/18/20 4:52 AM, Richard W.M. Jones wrote:
Unfortunately you cannot restore argv by setting *p = '=' :-(
The reason is we advertize that plugins are allowed to save they ‘const char *key’ pointer passed to them in .config, but assigning *p = '=' changes the key string from "key" back to "key=value".
D'oh. And my bad for only running the subset of the testsuite that I was working on ('make -C tests check TESTS=test-vddk.sh') rather than the full suite, or I would have noticed the regression from my one-liner. Thanks for fixing it.
Surprisingly only test-eval.sh actually broke, but other plugins are undoubtedly affected.
My alternate fix is fairly horrible, but passes all the tests and valgrind.
It looks right to me - if we can't modify argv[] in place, we have to copy off a stable version somewhere. Your patch is definitely longer than my one-liner, but I don't see any way to shorten it. ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones