Am 19.10.18 um 17:56 schrieb David Sommerseth: > From: Heiko Hund <[email protected]> > > 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 <[email protected]> > Signed-off-by: David Sommerseth <[email protected]> > --- > 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 [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
