Am 19.10.18 um 17:56 schrieb David Sommerseth:
> From: Heiko Hund <heiko.h...@sophos.com>
>
> The previous implementation had the problem that it was not fully
> compatible with printf() and could only detect % format directives
> following a space character (0x20).
>
> It modifies the format string and inserts marks to separate groups
> before passing it to the regular printf in libc. The marks are
> later used to separate the output string into individual command
> line arguments.
>
> The choice of 0x1D as the argument delimiter is based on the
> assumption that no "regular" string passed to argv_printf_*() will
> ever have to contain that byte (and the fact that it actually is
> the ASCII "group separator" control character, which fits its
> purpose).
>
> This commit has been updated by David Sommerseth based on his feedback
> on the mailing list discussions earlier on.
>
> Signed-off-by: Heiko Hund <heiko.h...@sophos.com>
> Signed-off-by: David Sommerseth <dav...@openvpn.net>
> ---
> src/openvpn/argv.c | 266 ++++++++++++---------------
> src/openvpn/argv.h | 4 +-
> src/openvpn/route.c | 8 +-
> src/openvpn/tun.c | 24 +--
> tests/unit_tests/openvpn/test_argv.c | 58 +++++-
> 5 files changed, 192 insertions(+), 168 deletions(-)
>
> diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
> index 9100a196..9606f382 100644
> --- a/src/openvpn/argv.c
> +++ b/src/openvpn/argv.c
> @@ -131,64 +131,6 @@ argv_insert_head(const struct argv *a, const char *head)
> return r;
> }
>
> -static char *
> -argv_term(const char **f)
> -{
> - const char *p = *f;
> - const char *term = NULL;
> - size_t termlen = 0;
> -
> - if (*p == '\0')
> - {
> - return NULL;
> - }
> -
> - while (true)
> - {
> - const int c = *p;
> - if (c == '\0')
> - {
> - break;
> - }
> - if (term)
> - {
> - if (!isspace(c))
> - {
> - ++termlen;
> - }
> - else
> - {
> - break;
> - }
> - }
> - else
> - {
> - if (!isspace(c))
> - {
> - term = p;
> - termlen = 1;
> - }
> - }
> - ++p;
> - }
> - *f = p;
> -
> - if (term)
> - {
> - char *ret;
> - ASSERT(termlen > 0);
> - ret = malloc(termlen + 1);
> - check_malloc_return(ret);
> - memcpy(ret, term, termlen);
> - ret[termlen] = '\0';
> - return ret;
> - }
> - else
> - {
> - return NULL;
> - }
> -}
> -
> const char *
> argv_str(const struct argv *a, struct gc_arena *gc, const unsigned int flags)
> {
> @@ -218,119 +160,151 @@ argv_msg_prefix(const int msglev, const struct argv
> *a, const char *prefix)
> gc_free(&gc);
> }
>
> -static void
> +
> +/*
> + * argv_prep_format - prepare argv format string for further processing
> + *
> + * Individual argument must be separated by space. Ignores leading and
> trailing spaces.
> + * Consecutive spaces count as one. Returns prepared format string, with
> space replaced
> + * by delim and adds the number of arguments to the count parameter.
> + */
> +static char *
> +argv_prep_format(const char *format, const char delim, size_t *count, struct
> gc_arena *gc)
> +{
Lines longer than 80 chars (90).
> + int i, j;
> + char *f;
All could be made inline with C99
> + bool in_token = false;
> +
> + if (format == NULL)
> + {
> + return NULL;
> + }
> +
> + f = gc_malloc(strlen(format) + 1, true, gc);
> + for (i = 0, j = 0; i < strlen(format); i++)
> + {
int i=0, j=0;
> + if (format[i] == ' ')
> + {
> + in_token = false;
> + continue;
> + }
> +
> + if (!in_token)
> + {
> + ++*count;
I am entirely sure about our style here but I would prefer brackets
around (*count). And since you use i++ and j++, also should use (*count)++;
> +
> + /*
> + * We don't add any delimiter to the resulting
> + * format string before something has been added there.
This sentence is hard to understand. "before something has been added
there" => if it is empty?
> + * The resulting format string will never start with a delimiter.
Okay. But why? I think this is done to ignore, leading spaces?
> + */
> + if (j > 0)
> + {
> + f[j++] = delim;
> + }
[...]
> +/*
> + * argv_printf_arglist - create a struct argv from a format string
> + *
> + * Instead of parsing the format string ourselves place delimiters via
> argv_prep_format()
> + * before we let libc's printf() do the parsing. Then split the resulting
> string at the
> + * injected delimiters.
> + */
> +static bool
> argv_printf_arglist(struct argv *a, const char *format, va_list arglist)
> {
> - char *term;
> - const char *f = format;
> + struct gc_arena gc = gc_new();
> + const char delim = 0x1D; /* ASCII Group Separator (GS) */
> + char *f, *buf, *end;
> + size_t argc, size;
> + bool res = false;
> + va_list tmplist;
> + int len;
Making some of these inline or move their defintion closer where they
are actually would make the code easier to understand.
> argv_extend(a, 1); /* ensure trailing NULL */
This depends on the side effect of the allocating nulling things before.
Not sure if that is okay or not.
> - while ((term = argv_term(&f)) != NULL)
> + argc = a->argc;
> + f = argv_prep_format(format, delim, &argc, &gc);
> + if (f == NULL)
> {
> - if (term[0] == '%')
> - {
> - if (!strcmp(term, "%s"))
> - {
> - char *s = va_arg(arglist, char *);
> - if (!s)
> - {
> - s = "";
> - }
> - argv_append(a, string_alloc(s, NULL));
> - }
> - else if (!strcmp(term, "%d"))
> - {
> - char numstr[64];
> - openvpn_snprintf(numstr, sizeof(numstr), "%d",
> va_arg(arglist, int));
> - argv_append(a, string_alloc(numstr, NULL));
> - }
> - else if (!strcmp(term, "%u"))
> - {
> - char numstr[64];
> - openvpn_snprintf(numstr, sizeof(numstr), "%u",
> va_arg(arglist, unsigned int));
> - argv_append(a, string_alloc(numstr, NULL));
> - }
> - else if (!strcmp(term, "%lu"))
> - {
> - char numstr[64];
> - openvpn_snprintf(numstr, sizeof(numstr), "%lu",
> - va_arg(arglist, unsigned long));
> - argv_append(a, string_alloc(numstr, NULL));
> - }
> - else if (!strcmp(term, "%s/%d"))
> - {
> - char numstr[64];
> - char *s = va_arg(arglist, char *);
> -
> - if (!s)
> - {
> - s = "";
> - }
> + goto out;
> + }
>
> - openvpn_snprintf(numstr, sizeof(numstr), "%d",
> va_arg(arglist, int));
> + /* determine minimum buffer size */
> + va_copy(tmplist, arglist);
> + len = vsnprintf(NULL, 0, f, tmplist);
> + va_end(tmplist);
> + if (len < 0)
> + {
> + goto out;
> + }
>
> - {
> - const size_t len = strlen(s) + strlen(numstr) + 2;
> - char *combined = (char *) malloc(len);
> - check_malloc_return(combined);
> + size = adjust_power_of_2(len + 1);
I am a bit sure why we need to increase the size to the next power of 2
and why len+1 shouldn't be enough
> + buf = gc_malloc(size, false, &gc);
> + len = vsnprintf(buf, size, f, arglist);
> + if (len < 0 || len >= size)
> + {
> + goto out;
> + }
This vnsprintf should need the same space as the previous vsnprintf that
was used to determine len.
>
> - strcpy(combined, s);
> - strcat(combined, "/");
> - strcat(combined, numstr);
> - argv_append(a, combined);
> - }
> - }
> - else if (!strcmp(term, "%s%sc"))
> - {
> - char *s1 = va_arg(arglist, char *);
> - char *s2 = va_arg(arglist, char *);
> - char *combined;
> + /* split the string at the delimiters */
> + end = strchr(buf, delim);
> + while (end)
> + {
> + *end = '\0';
> + argv_append(a, string_alloc(buf, NULL));
> + buf = end + 1;
> + end = strchr(buf, delim);
> + }
> + argv_append(a, string_alloc(buf, NULL));
>
> - if (!s1)
> - {
> - s1 = "";
> - }
> - if (!s2)
> - {
> - s2 = "";
> - }
> - combined = (char *) malloc(strlen(s1) + strlen(s2) + 1);
> - check_malloc_return(combined);
> - strcpy(combined, s1);
> - strcat(combined, s2);
> - argv_append(a, combined);
> - }
> - else
> - {
> - ASSERT(0);
> - }
> - free(term);
> - }
> - else
> - {
> - argv_append(a, term);
> - }
> + if (a->argc != argc)
> + {
> + /* Someone snuck in a GS (0x1D), fail gracefully */
> + argv_reset(a);
> + argv_extend(a, 1); /* ensure trailing NULL */
> + goto out;
> }
That is a bit weird way to check that but okay.
> -void
> +
> +
> +bool
> argv_printf(struct argv *a, const char *format, ...)
> {
> + bool res;
> va_list arglist;
> - argv_reset(a);
> +
> va_start(arglist, format);
> - argv_printf_arglist(a, format, arglist);
> + argv_reset(a);
> + res = argv_printf_arglist(a, format, arglist);
> va_end(arglist);
> + return res;
> }
The bool definition could be made inline (bool res=).
> -void
> +bool
> argv_printf_cat(struct argv *a, const char *format, ...)
> {
> + bool res;
> va_list arglist;
> va_start(arglist, format);
> - argv_printf_arglist(a, format, arglist);
> + res = argv_printf_arglist(a, format, arglist);
> va_end(arglist);
> + return res;
> }
Same here.
Rest looks good.
Arne
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
