Re: [Qemu-devel] [PATCH v3 3/3] net: tap: refactor net_bridge_run_helper routine

[Daniel P . Berrangé]

On Mon, Jul 01, 2019 at 06:05:58PM +0530, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> Refactor 'net_bridge_run_helper' routine to avoid buffer
> formatting to prepare 'helper_cmd' and using shell to invoke
> helper command. Instead directly execute helper program with
> due arguments.
> 
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  net/tap.c | 43 +++++++++----------------------------------
>  1 file changed, 9 insertions(+), 34 deletions(-)
> 
> Update v3: remove buffer formatting and use of shell to invoke helper
>   -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00071.html
> 
> diff --git a/net/tap.c b/net/tap.c
> index e8aadd8d4b..bc9b3407a6 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -478,7 +478,6 @@ static int net_bridge_run_helper(const char *helper, 
> const char *bridge,
>      sigset_t oldmask, mask;
>      int pid, status;
>      char *args[5];
> -    char **parg;
>      int sv[2];
>  
>      sigemptyset(&mask);
> @@ -498,9 +497,6 @@ static int net_bridge_run_helper(const char *helper, 
> const char *bridge,
>      }
>      if (pid == 0) {
>          int open_max = sysconf(_SC_OPEN_MAX), i;
> -        char fd_buf[6+10];
> -        char br_buf[6+IFNAMSIZ] = {0};
> -        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
>  
>          for (i = 3; i < open_max; i++) {
>              if (i != sv[1]) {
> @@ -508,39 +504,18 @@ static int net_bridge_run_helper(const char *helper, 
> const char *bridge,
>              }
>          }
>  
> -        snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
> +        args[0] = (char *)helper;
> +        args[1] = (char *)"--use-vnet";
> +        args[2] = g_strdup_printf("%s%d", "--fd=", sv[1]);
> +        args[3] = g_strdup_printf("%s%s", "--br=", bridge);
> +        args[4] = NULL;
>  
> -        if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
> -            /* assume helper is a command */
> +        execv(helper, args);
>  
> -            if (strstr(helper, "--br=") == NULL) {
> -                snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> -            }
> +        g_free(args[2]);
> +        g_free(args[3]);
> +        fprintf(stderr, "failed to execute helper: %s\n", helper);
>  
> -            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
> -                     helper, "--use-vnet", fd_buf, br_buf);
> -
> -            parg = args;
> -            *parg++ = (char *)"sh";
> -            *parg++ = (char *)"-c";
> -            *parg++ = helper_cmd;
> -            *parg++ = NULL;
> -
> -            execv("/bin/sh", args);
> -        } else {
> -            /* assume helper is just the executable path name */
> -
> -            snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> -
> -            parg = args;
> -            *parg++ = (char *)helper;
> -            *parg++ = (char *)"--use-vnet";
> -            *parg++ = fd_buf;
> -            *parg++ = br_buf;
> -            *parg++ = NULL;
> -
> -            execv(helper, args);
> -        }

Hmm, it seems I was probaly a bit too optimistic in my suggestion to
drop use of shell entirely.

The original code was passing through to the shell to handle the case
where the user requested

   -netdev bridge,helper="/path/to/helper myarg otherarg"

In theory any parts could contain shell meta characters, but even if
they don't we'll have slightly broken compat with this change.

The QEMU man page has never documented that you can pass a command
and args, which get sent via the shell though. It only ever documented
the helper arg as being a plain qualified binary path.

So the question is how strictly we need to consider compatiblity.

The "if it isn't documented it never existed" option is to use your
patch here.

The moderately aggressive option is to just use g_shell_parse_argv()
to split the "helper" into a set of argv we can exec directly, and
declare that we don't support shell meta characters in helper.

The safest option is to put in a place a deprecation saying we'll
drop use of shell in future, only implementing the agressive
option in a later release.

Perhaps from your POV, the easy thing is to avoid this entire
question - just leave the code calling shell, but switch to
g_strdup_printf instead of snprintf.

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 :|