Libguestfs - Feb 2020 - [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