Re: [Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()

[Arne Schwabe]

Am 06.02.20 um 14:21 schrieb David Sommerseth:
> From: Heiko Hund 
> 
> 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 Arne
> Schwabe and his own feedback on the mailing list.
> 

Acked-By: Arne Schwabe 



signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()

[David Sommerseth]

From: Heiko Hund 

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 Arne
Schwabe and his own feedback on the mailing list.

Signed-off-by: Heiko Hund 
Signed-off-by: David Sommerseth 

---
v2 - Improved comments, to make it even clearer what is going on
   - Switched to C99 variable declaration, closer to where used
   - Swapped out adjust_power_of_2() length calculation in
 argv_printf_arglist() to len+1, which should be good enough.
---
 src/openvpn/argv.c   | 289 +--
 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, 206 insertions(+), 177 deletions(-)

diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
index 9100a196..fcf61ec5 100644
--- a/src/openvpn/argv.c
+++ b/src/openvpn/argv.c
@@ -105,16 +105,15 @@ static struct argv
 argv_clone(const struct argv *a, const size_t headroom)
 {
 struct argv r;
-size_t i;
-
 argv_init();
-for (i = 0; i < headroom; ++i)
+
+for (size_t i = 0; i < headroom; ++i)
 {
 argv_append(, NULL);
 }
 if (a)
 {
-for (i = 0; i < a->argc; ++i)
+for (size_t i = 0; i < a->argc; ++i)
 {
 argv_append(, string_alloc(a->argv[i], NULL));
 }
@@ -131,64 +130,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,132 +159,170 @@ argv_msg_prefix(const int msglev, const struct argv *a, 
const char *prefix)
 gc_free();
 }
 
-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)
+{
+if (format == NULL)
+{
+return NULL;
+}
+
+bool in_token = false;
+char *f = gc_malloc(strlen(format) + 1, true, gc);
+for (int i = 0, j = 0; i < strlen(format); i++)
+{
+if (format[i] == ' ')
+{
+in_token = false;
+continue;
+}
+
+if (!in_token)
+{
+(*count)++;
+
+/*
+ * We don't add any delimiter to the output string if
+ * the string is empty; the resulting format string
+ * will never start with a delimiter.
+ */
+if (j > 0)  /* Has anything been written to the output string? */
+{
+f[j++] = delim;
+}
+}
+
+f[j++] = format[i];
+in_token = true;
+}
+
+return f;
+}
+
+/*
+ * 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)
 {

Re: [Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()

[Gert Doering]

Hi,

On Sun, Jan 19, 2020 at 04:51:08PM +0100, Gert Doering wrote:
> David, shall I just amend the patch, or do you want to resend a v3?
> 
> (The other 3 of the set should be fine, as they do not touch tun.c)

Not exactly correct, 2/4 renames all argv_reset() calls to argv_free(),
so that *might* need adjustment for tun.c as well.  But these are 
similarily trivial, and if not done completely, will result in link
failures (so, easy enough, provided I compile on all platforms).

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()

[Gert Doering]

Hi,

On Fri, Oct 04, 2019 at 07:12:43PM +0200, David Sommerseth wrote:
> From: Heiko Hund 
> 
> 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).

Events have overtaken this one.

Namely, tun.c has been changed in many places due to the windows
refactoring / wintun work by Lev and Simon, so this patch does not
apply "as is" anymore.

The changes to tun.c are trivial ("%s%sc" --> "%s%s"), but according to
custom, I do not do code changes while applying - at least not without
asking :-)

David, shall I just amend the patch, or do you want to resend a v3?

(The other 3 of the set should be fine, as they do not touch tun.c)

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()

[David Sommerseth]

From: Heiko Hund 

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 Arne
Schwabe and his own feedback on the mailing list.

Signed-off-by: Heiko Hund 
Signed-off-by: David Sommerseth 

---
v2 - Improved comments, to make it even clearer what is going on
   - Switched to C99 variable declaration, closer to where used
   - Swapped out adjust_power_of_2() length calculation in
 argv_printf_arglist() to len+1, which should be good enough.
---
 src/openvpn/argv.c   | 289 +--
 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, 206 insertions(+), 177 deletions(-)

diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
index 9100a196..fcf61ec5 100644
--- a/src/openvpn/argv.c
+++ b/src/openvpn/argv.c
@@ -105,16 +105,15 @@ static struct argv
 argv_clone(const struct argv *a, const size_t headroom)
 {
 struct argv r;
-size_t i;
-
 argv_init();
-for (i = 0; i < headroom; ++i)
+
+for (size_t i = 0; i < headroom; ++i)
 {
 argv_append(, NULL);
 }
 if (a)
 {
-for (i = 0; i < a->argc; ++i)
+for (size_t i = 0; i < a->argc; ++i)
 {
 argv_append(, string_alloc(a->argv[i], NULL));
 }
@@ -131,64 +130,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,132 +159,170 @@ argv_msg_prefix(const int msglev, const struct argv *a, 
const char *prefix)
 gc_free();
 }
 
-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)
+{
+if (format == NULL)
+{
+return NULL;
+}
+
+bool in_token = false;
+char *f = gc_malloc(strlen(format) + 1, true, gc);
+for (int i = 0, j = 0; i < strlen(format); i++)
+{
+if (format[i] == ' ')
+{
+in_token = false;
+continue;
+}
+
+if (!in_token)
+{
+(*count)++;
+
+/*
+ * We don't add any delimiter to the output string if
+ * the string is empty; the resulting format string
+ * will never start with a delimiter.
+ */
+if (j > 0)  /* Has anything been written to the output string? */
+{
+f[j++] = delim;
+}
+}
+
+f[j++] = format[i];
+in_token = true;
+}
+
+return f;
+}
+
+/*
+ * 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)
 {

Re: [Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()

[David Sommerseth]

On 19/10/18 17:56, David Sommerseth wrote:
> From: Heiko Hund 
> 
> 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 
> Signed-off-by: David Sommerseth 
> ---
>  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(-)

This is a rebase of [0,1] on top of git master 2b15c11716e0d.  I've added my
review suggestions directly to this commit, as can be found here [2].  The
doxygen suggestion is coming as the last patch in this patch-set.

[0] 
[1]

[2]



-- 
kind regards,

David Sommerseth
OpenVPN Inc




signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()

[David Sommerseth]

From: Heiko Hund 

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 
Signed-off-by: David Sommerseth 
---
 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();
 }
 
-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)
+{
+int i, j;
+char *f;
+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++)
+{
+if (format[i] == ' ')
+{
+in_token = false;
+continue;
+}
+
+if (!in_token)
+{
+++*count;
+
+/*
+ * We don't add any delimiter to the resulting
+ * format string before something has been added there.
+ * The resulting format string will never start with a delimiter.
+ */
+if (j > 0)
+{
+f[j++] = delim;
+}
+}
+
+f[j++] = format[i];
+in_token = true;
+}
+
+return f;
+}
+
+/*
+ * 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;
 
 argv_extend(a, 1); /* ensure trailing NULL */
 
-while ((term = argv_term()) != NULL)
+argc = a->argc;
+f = argv_prep_format(format, delim, , );
+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,