On Fri, May 29, 2020 at 08:56:20PM +1000, Darren Tucker wrote:
> On Fri, May 29, 2020 at 12:45:12PM +0200, Otto Moerbeek wrote:
> [...]
> > This fails on arm64:
> >
> > /usr/src/usr.bin/ssh/ssh/../misc.c:1181:51: error: passing 'void *' to
> > parameter of incompatible type
> > 'va_list' (aka '__builtin_va_list')
> > ret = vdollar_percent_expand(&err, 1, 0, string, NULL);
> > ^~~~
> > /usr/include/sys/_null.h:10:14: note: expanded from macro 'NULL'
> > #define NULL ((void *)0)
> > ^~~~~~~~~~~
> > /usr/src/usr.bin/ssh/ssh/../misc.c:1060:33: note: passing argument to
> > parameter 'ap' here
> > const char *string, va_list ap)
> > ^
> > 1 error generated.
>
> Sigh, I just found that out in portable too. Here's the message I just
> sent to the other openssh developers. If you have any better
> suggestions or an ok I'd be happy to hear it.
I made a quick fix locally which does the same as the diff below.
Wouldn't know another portable way.
-Otto
>
> ---------------------------------------------------------------------
>
> A brief recap:
> - percent_expand originally was variadic, which made sense since it
> could take a variable number of %-tokens.
> - When we added environment variables, we put it in the same function as
> percent_expand instead of a separate one to prevent unanticipated
> interactions between the two. this became vdollar_percent_expand.
> - There's wrapper functions around that to do percent, dollar or both
> expansions.
> - dollar_expand is not variadic because the variadic args are for
> percent expansion which it doesn't do.
> - originally I passed a NULL as the va_list to vdollar_percent_expand.
> Damien pointed out that there's no guarantee va_list is a pointer
> equivalent, so it was changed to a real va_list which was memset-0ed.
>
> After commiting, I found out that this memset blew up on i386:
>
> misc.c:1182:23: error: ´memset´ call operates on objects of type
> ´char´ while the size is based on a different type ´va_list´ (´char *´)
> [-Werror,-Wsizeof-pointer-memaccess]
> memset(ap, 0, sizeof(ap)); /* unused */
>
> To unbreak the build, I switched it back to passing NULL, which works on at
> least i386 and amd64. Now I'm trying to fix the mess properly.
>
> As far as I can tell, there's no standards-compliant way to either pass
> an unused va_list or initialize an unused real va_list. Googling
> indicates that calling va_start from a non-variadic function will
> generate a compiler warning, although our clang seems OK with it.
>
> This diff below makes dollar_expand variadic, with any such arguments
> ignored in the workhorse function.
>
> Does anyone know a better way to fix this? Passing an uninitialized
> va_list seems wrong even if the called function doesn't use it.
>
> Index: misc.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ssh/misc.c,v
> retrieving revision 1.151
> diff -u -p -r1.151 misc.c
> --- misc.c 29 May 2020 09:02:44 -0000 1.151
> +++ misc.c 29 May 2020 09:41:47 -0000
> @@ -1172,13 +1172,22 @@ vdollar_percent_expand(int *parseerror,
> #undef EXPAND_MAX_KEYS
> }
>
> +/*
> + * Expand only environment variables.
> + * Note that although this function is variadic like the other similar
> + * functions, any such arguments will be unused.
> + */
> +
> char *
> -dollar_expand(int *parseerr, const char *string)
> +dollar_expand(int *parseerr, const char *string, ...)
> {
> char *ret;
> int err;
> + va_list ap;
>
> - ret = vdollar_percent_expand(&err, 1, 0, string, NULL);
> + va_start(ap, string);
> + ret = vdollar_percent_expand(&err, 1, 0, string, ap);
> + va_end(ap);
> if (parseerr != NULL)
> *parseerr = err;
> return ret;
> Index: misc.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/ssh/misc.h,v
> retrieving revision 1.86
> diff -u -p -r1.86 misc.h
> --- misc.h 29 May 2020 04:25:40 -0000 1.86
> +++ misc.h 29 May 2020 09:41:48 -0000
> @@ -69,7 +69,7 @@ long convtime(const char *);
> const char *fmt_timeframe(time_t t);
> char *tilde_expand_filename(const char *, uid_t);
>
> -char *dollar_expand(int *, const char *string);
> +char *dollar_expand(int *, const char *string, ...);
> char *percent_expand(const char *, ...) __attribute__((__sentinel__));
> char *percent_dollar_expand(const char *, ...) __attribute__((__sentinel__));
> char *tohex(const void *, size_t);
>
> --
> Darren Tucker (dtucker at dtucker.net)
> GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
> Good judgement comes with experience. Unfortunately, the experience
> usually comes from bad judgement.
