Re: [Bug-wget] [PATCH v4] (resend) Add --use-askpass=COMMAND support

[Liam R. Howlett]

* Tim Ruehsen <address@hidden> [160907 04:49]:
> Hi Liam,
> 
> I made a mistake and accidentally pushed your v4 patch to master a few days 
> ago.
> 
> Here is a fix for several issues (warnings, C89 incompatibility, using 
> strpbrk 
> to find end-of-line, append newline to error messages).
> 
> I removed your extra checks, so that a user can just work with executables in 
> the PATH (posix_spawnp searches the path):
>   wget --use-askpass=ssh-askpass <URL>
> 
> If the executable does not exists (or something goes wrong when executing), 
> wget will error & stop at that point anyways.
> 
> Here is the fix, WDYT ?

I like the change to use strpbrk.  This patch works with my testing.

Just a few comments:


1. Since you are using xmemdup0, can't you also drop these lines?
(1101-1102):
      /* Make sure there is a trailing 0 */
            tmp[bytes] = '\0';

I think you can change the read line to remove the -1 from the size of
the read (line 1092) if you drop the above:

- bytes = read (com[0], tmp, sizeof (tmp) - 1);
+ bytes = read (com[0], tmp, sizeof (tmp));


2. Is there a whitespace issue here?
+  if ((p = strpbrk(tmp, "\r\n")))
                   ^- Whitespace missing?

Thanks,
Liam

> 
> Regards, Tim

> From=20afb66eb94f5e5c4a0cbfed6a1e95d08418bc0dd6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Tim=20R=C3=BChsen?= <address@hidden>
> Date: Wed, 7 Sep 2016 10:40:43 +0200
> Subject: [PATCH] Code cleanup for --use-askpass
> 
> * bootstrap.conf: Add xmemdup0 and strpbrk.
> * src/init.c (cmd_use_askpass): Add 'const' to char *,
>   remove check for file existence.
> * src/main.c (run_use_askpass): C89 compat init of argv,
>   added \n to error messages,
>   fixed stripping of \n and \r from input,
>   make run_use_askpass and use_askpass static.
> ---
>  bootstrap.conf |  2 ++
>  src/init.c     | 20 +++-----------------
>  src/main.c     | 33 ++++++++++++++++++++-------------
>  3 files changed, 25 insertions(+), 30 deletions(-)
> 
> diff --git a/bootstrap.conf b/bootstrap.conf
> index e839277..e706a37 100644
> --- a/bootstrap.conf
> +++ b/bootstrap.conf
> @@ -90,6 +90,7 @@ stdbool
>  stdint
>  strcase
>  strerror_r-posix
> +strpbrk
>  strptime
>  strtok_r
>  strtoll
> @@ -102,6 +103,7 @@ update-copyright
>  vasprintf
>  vsnprintf
>  write
> +xmemdup0
>  xstrndup
>  "
>  
> diff --git a/src/init.c b/src/init.c
> index dbf356a..b7f573a 100644
> --- a/src/init.c
> +++ b/src/init.c
> @@ -1388,18 +1388,11 @@ cmd_time (const char *com, const char *val, void 
> *place)
>  static bool
>  cmd_use_askpass (const char *com _GL_UNUSED, const char *val, void *place)
>  {
> -  char *env_name = "WGET_ASKPASS";
> -  char *env;
> +  const char *env_name = "WGET_ASKPASS";
> +  const char *env;
>  
>    if (val && *val)
> -    {
> -      if (!file_exists_p (val))
> -        {
> -          fprintf (stderr, _("%s does not exist.\n"), val);
> -          exit (WGET_EXIT_GENERIC_ERROR);
> -        }
> -      return cmd_string (com, val, place);
> -    }
> +    return cmd_string (com, val, place);
>  
>    env = getenv (env_name);
>    if (!(env && *env))
> @@ -1414,13 +1407,6 @@ cmd_use_askpass (const char *com _GL_UNUSED, const 
> char *val, void *place)
>        exit (WGET_EXIT_GENERIC_ERROR);
>      }
>  
> -  if (!file_exists_p (env))
> -    {
> -      fprintf (stderr, _("%s points to %s, which does not exist.\n"),
> -              env_name, env);
> -      exit (WGET_EXIT_GENERIC_ERROR);
> -    }
> -
>    return cmd_string (com, env, place);
>  }
>  
> diff --git a/src/main.c b/src/main.c
> index 749ec3c..9d0d5d3 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -61,6 +61,7 @@ as that of the covered work.  */
>  #include "version.h"
>  #include "c-strcase.h"
>  #include "dirname.h"
> +#include "xmemdup0.h"
>  #include <getopt.h>
>  #include <getpass.h>
>  #include <quote.h>
> @@ -1038,7 +1039,7 @@ prompt_for_password (void)
>  
>  
>  /* Execute external application opt.use_askpass */
> -void
> +static void
>  run_use_askpass (char *question, char **answer)
>  {
>    char tmp[1024];
> @@ -1046,12 +1047,12 @@ run_use_askpass (char *question, char **answer)
>    int status;
>    int com[2];
>    ssize_t bytes = 0;
> -  char * const argv[] = { opt.use_askpass, question, NULL };
> +  char *argv[3], *p;
>    posix_spawn_file_actions_t fa;
>  
>    if (pipe (com) == -1)
>      {
> -      fprintf (stderr, _("Cannot create pipe"));
> +      fprintf (stderr, _("Cannot create pipe\n"));
>        exit (WGET_EXIT_GENERIC_ERROR);
>      }
>  
> @@ -1059,7 +1060,7 @@ run_use_askpass (char *question, char **answer)
>    if (status)
>      {
>        fprintf (stderr,
> -              _("Error initializing spawn file actions for use-askpass: %d"),
> +              _("Error initializing spawn file actions for use-askpass: 
> %d\n"),
>                status);
>        exit (WGET_EXIT_GENERIC_ERROR);
>      }
> @@ -1068,15 +1069,21 @@ run_use_askpass (char *question, char **answer)
>    if (status)
>      {
>        fprintf (stderr,
> -              _("Error setting spawn file actions for use-askpass: %d"),
> +              _("Error setting spawn file actions for use-askpass: %d\n"),
>                status);
>        exit (WGET_EXIT_GENERIC_ERROR);
>      }
>  
> +  /* C89 initializer lists must be computable at load time,
> +   * thus this explicit initialization. */
> +  argv[0] = opt.use_askpass;
> +  argv[1] = question;
> +  argv[2] = NULL;
> +
>    status = posix_spawnp (&pid, opt.use_askpass, &fa, NULL, argv, environ);
>    if (status)
>      {
> -      fprintf (stderr, "Error spawning %s: %d", opt.use_askpass, status);
> +      fprintf (stderr, "Error spawning %s: %d\n", opt.use_askpass, status);
>        exit (WGET_EXIT_GENERIC_ERROR);
>      }
>  
> @@ -1090,19 +1097,19 @@ run_use_askpass (char *question, char **answer)
>                opt.use_askpass, question, strerror (errno));
>        exit (WGET_EXIT_GENERIC_ERROR);
>      }
> -  /* Set the end byte to \0, and decrement bytes */
> -  tmp[bytes--] = '\0';
> +
> +  /* Make sure there is a trailing 0 */
> +  tmp[bytes] = '\0';
>  
>    /* Remove a possible new line */
> -  while (bytes >= 0 &&
> -        (tmp[bytes] == '\0' || tmp[bytes] == '\n' || tmp[bytes] == '\r'))
> -    tmp[bytes--] = '\0';
> +  if ((p = strpbrk(tmp, "\r\n")))
> +    bytes = p - tmp;
>  
> -  *answer = xmemdup (tmp, bytes + 2);
> +  *answer = xmemdup0 (tmp, bytes);
>  }
>  
>  /* set the user name and password*/
> -void
> +static void
>  use_askpass (struct url *u)
>  {
>    static char question[1024];
> -- 
> 2.9.3
>