Re: [Openvpn-devel] [PATCH 5/7] re-implement argv_printf_*()

[Heiko Hund]

Hi David,

On Wednesday, November 9, 2016 9:41:21 PM CET David Sommerseth wrote:
> In the new argv_prep_format() function:
> 
> +  if (!in_token)
> +{
> +  ++*count;
> +  if (f[0])
> +f[j++] = delim;
> +}
> 
> What is the purpose of the f[0] check?  At first glance it looks like a
> NOOP, as you'd expect f[0] to always have a value > 0.  But that is just
> until you've parsed something which have been put into f, so I presume
> it is some kind of "lock" to not write a delimiter as the first byte in
> a string.

That check is there to not add a delimiter after leading spaces, I've added a 
comment in v2 of the patch.
 
> Can this whole function be made a bit simpler to understand, as I'm
> struggling to fully follow what happens and why?  If I spend enough time
> digging into this, I will understand it ... but I prefer code which is
> easier to understand at first glance, unless the task can't be resolved
> with less complexity.

Added comments to explain what's happening in argv_prep_format() and 
argv_printf_arglist(). Especially the first one does more than just copying the 
format string buffer, so its complexity is justified.

> So to argv_printf_arglist():
> 
> +  va_copy (tmplist, arglist);
> +  len = vsnprintf (NULL, 0, f, tmplist);
> +  va_end (tmplist);
> +  if (len < 0)
> +goto out;
> 
> This is reasonably okay, but would benefit from a comment simply
> explaining what you do here.  I presume it is a kind of estimated
> strlen() operation of the final formatted string.

Done.

> Wed Nov  9 21:37:26 2016
> Wed Nov  9 21:37:26 2016 openvpn_execve: called with empty argv
> Wed Nov  9 21:37:26 2016 Exiting due to fatal error
> 
> Reverting this patch makes t_cltsrv.sh succeed, so pretty sure this
> patch is the one to blame for this error.

Nice catch! This was triggered by the use of strtok(), which only returns 
nonempty tokens. During shutdown openvpn passes empty strings to the down 
script, which triggered this. Not using strtok() anymore and also added a unit 
test for empty string parameters.

Cheers
Heiko

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 5/7] re-implement argv_printf_*()

[David Sommerseth]

On 28/10/16 18:42, Heiko Hund wrote:
> 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 '\035' 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).
> 
> Signed-off-by: Heiko Hund 
> ---
>  src/openvpn/argv.c   | 203 
> ++-
>  src/openvpn/argv.h   |   4 +-
>  src/openvpn/route.c  |   8 +-
>  src/openvpn/tun.c|  22 ++--
>  tests/unit_tests/openvpn/test_argv.c |  34 +-
>  5 files changed, 130 insertions(+), 141 deletions(-)

This one was harder to review.  Had to use 'git diff --histogram' to
make it easier to read and parse.  And there I spotted a few odd things.

In the new argv_prep_format() function:

+  if (!in_token)
+{
+  ++*count;
+  if (f[0])
+f[j++] = delim;
+}

What is the purpose of the f[0] check?  At first glance it looks like a
NOOP, as you'd expect f[0] to always have a value > 0.  But that is just
until you've parsed something which have been put into f, so I presume
it is some kind of "lock" to not write a delimiter as the first byte in
a string.

Can this whole function be made a bit simpler to understand, as I'm
struggling to fully follow what happens and why?  If I spend enough time
digging into this, I will understand it ... but I prefer code which is
easier to understand at first glance, unless the task can't be resolved
with less complexity.

Suggestion: Why not just do a memmove()/memcpy() to the destination
buffer first, then loop through the destination buffer and flip the
in_token flag and replace spaces with delim where needed, but directly
on the destination buffer?  With this method, you could even skip the
first byte easily before the loop.


So to argv_printf_arglist():

+  va_copy (tmplist, arglist);
+  len = vsnprintf (NULL, 0, f, tmplist);
+  va_end (tmplist);
+  if (len < 0)
+goto out;

This is reasonably okay, but would benefit from a comment simply
explaining what you do here.  I presume it is a kind of estimated
strlen() operation of the final formatted string.

The vsnprintf(NULL, 0, ...) should also have a little note, as this
requires C99 mode to work - otherwise it will return something
completely unexpected.


Otherwise it looks good.  Valgrind tests reports no leaks, t_client.sh
and t_lpback.sh tests are good.  BUT t_cltsrv.sh fails, so that is also
something which needs to be fixed.  The error is:

Wed Nov  9 21:37:26 2016
Wed Nov  9 21:37:26 2016 openvpn_execve: called with empty argv
Wed Nov  9 21:37:26 2016 Exiting due to fatal error

Reverting this patch makes t_cltsrv.sh succeed, so pretty sure this
patch is the one to blame for this error.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 5/7] re-implement argv_printf_*()

[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 '\035' 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).

Signed-off-by: Heiko Hund 
---
 src/openvpn/argv.c   | 203 ++-
 src/openvpn/argv.h   |   4 +-
 src/openvpn/route.c  |   8 +-
 src/openvpn/tun.c|  22 ++--
 tests/unit_tests/openvpn/test_argv.c |  34 +-
 5 files changed, 130 insertions(+), 141 deletions(-)

diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
index f8287b7..dcfbd76 100644
--- a/src/openvpn/argv.c
+++ b/src/openvpn/argv.c
@@ -6,6 +6,7 @@
  * packet compression.
  *
  *  Copyright (C) 2002-2010 OpenVPN Technologies, Inc. 
+ *2016  Heiko Hund 
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2
@@ -122,54 +123,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)
 {
@@ -195,101 +148,111 @@ argv_msg_prefix (const int msglev, const struct argv 
*a, const char *prefix)
   gc_free ();
 }
 
-static void
-argv_printf_arglist (struct argv *a, const char *format, va_list arglist)
+static char *
+argv_prep_format (const char *format, const char delim, size_t *count, struct 
gc_arena *gc)
 {
-  char *term;
-  const char *f = format;
+  int i, j;
+  char *f;
+  bool in_token = false;
 
-  argv_extend (a, 1); /* ensure trailing NULL */
+  if (format == NULL)
+return NULL;
 
-  while ((term = argv_term ()) != NULL)
+  f = gc_malloc (strlen (format) + 1, true, gc);
+  for (i = 0, j = 0; i < strlen (format); i++)
 {
-  if (term[0] == '%')
+  if (format[i] == ' ')
 {
-  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, "%s/%d"))
-{
-  char numstr[64];
-  char *s = va_arg (arglist, char *);
-
-  if (!s)
-s = "";
-
-  openvpn_snprintf (numstr, sizeof (numstr), "%d", va_arg 
(arglist, int));
-
-  {
-const size_t len = strlen(s) + strlen(numstr) + 2;
-char *combined = (char *) malloc (len);
-check_malloc_return (combined);
-
-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;
-  char *cmd_name;
-
-  if (!s1) s1 = "";
-  if (!s2) s2 = "";
-  combined = (char *) malloc