struct argv overhaul - round 2+x
cover struct argv overhaul - round 2+x
Hi,
Since the hackathon a couple of weeks ago, I've picked up the work from Heiko Hund which does a big overhaul on the struct argv patches [0].
This patchset is rebased on the latest master and with my review comments embedded into Heikos patch. This was only relevant for the big 're-implement argv_printf_*()' patch.
In addition, I've added a separate documentation patch which adds doxygen comments to basically all functions in argv.c, trying to explain better what each of these functions does. To make the comment flow better, I renamed a few variables and did minor refactoring changes on the coding style.
--
kind regards,
David Sommerseth
OpenVPN Inc
David Sommerseth (1):
Documented all the argv related code with minor refactoring
Heiko Hund (3):
re-implement argv_printf_*()
argv: do fewer memory re-allocations
Add gc_arena to struct argv to save allocations
src/openvpn/argv.c | 525 +++++++++++++++++---------- src/openvpn/argv.h | 7 +- src/openvpn/console_systemd.c | 2 +- src/openvpn/init.c | 15 +- src/openvpn/lladdr.c | 2 +- src/openvpn/multi.c | 10 +- src/openvpn/options.c | 2 +- src/openvpn/plugin.c | 2 +- src/openvpn/route.c | 16 +- src/openvpn/socket.c | 4 +- src/openvpn/ssl_verify.c | 6 +- src/openvpn/tun.c | 62 ++-- tests/unit_tests/openvpn/test_argv.c | 118 +++++- 13 files changed, 496 insertions(+), 275 deletions(-)
Signature · 1 line
2.17.1
PATCH 1/4 re-implement argv_printf_*()
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(-)
src/openvpn/argv.c +120 -146
@@ -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) +{ +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 (&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 ); + buf =gc_malloc (size, false, &gc); + len =vsnprintf (buf, size, f, arglist); +if (len <0 || len >= size) + { + goto out; + } -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; } + + res = true; + +out: +gc_free (&gc); +return res; } -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; } -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; }void
src/openvpn/argv.h +2 -2
@@ -52,7 +52,7 @@ void argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix)void argv_parse_cmd (struct argv *a,const char *s); -void argv_printf(struct argv *a, const char *format, ...) +bool argv_printf(struct argv *a, const char *format, ...)#ifdef __GNUC__#if __USE_MINGW_ANSI_STDIO __attribute__ ((format(gnu_printf, 2, 3)))
@@ -62,7 +62,7 @@ __attribute__ ((format(__printf__, 2, 3)))#endif ; -void argv_printf_cat(struct argv *a, const char *format, ...) +bool argv_printf_cat(struct argv *a, const char *format, ...)#ifdef __GNUC__#if __USE_MINGW_ANSI_STDIO __attribute__ ((format(gnu_printf, 2, 3)))
src/openvpn/route.c +4 -4
@@ -1614,7 +1614,7 @@ add_route(struct route_ipv4 *r,#elif defined (_WIN32) {DWORD ai =TUN_ADAPTER_INDEX_INVALID ; - argv_printf(&argv, "%s%sc ADD %s MASK %s %s", + argv_printf(&argv, "%s%s ADD %s MASK %s %s", get_win_sys_path(), WIN_ROUTE_PATH_SUFFIX, network,
@@ -1979,7 +1979,7 @@ add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt, unsigned int flag device =buf_bptr (&out);/* netsh interface ipv6 add route 2001:db8::/32 MyTunDevice */ - argv_printf(&argv, "%s%sc interface ipv6 add route %s/%d %s", + argv_printf(&argv, "%s%s interface ipv6 add route %s/%d %s", get_win_sys_path(), NETSH_PATH_SUFFIX, network,
@@ -2186,7 +2186,7 @@ delete_route(struct route_ipv4 *r,#elif defined (_WIN32) - argv_printf(&argv, "%s%sc DELETE %s MASK %s %s", + argv_printf(&argv, "%s%s DELETE %s MASK %s %s", get_win_sys_path(), WIN_ROUTE_PATH_SUFFIX, network,
@@ -2426,7 +2426,7 @@ delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt, unsigned device =buf_bptr (&out);/* netsh interface ipv6 delete route 2001:db8::/32 MyTunDevice */ - argv_printf(&argv, "%s%sc interface ipv6 delete route %s/%d %s", + argv_printf(&argv, "%s%s interface ipv6 delete route %s/%d %s", get_win_sys_path(), NETSH_PATH_SUFFIX, network,
src/openvpn/tun.c +12 -12
@@ -994,7 +994,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu,openvpn_snprintf (iface,sizeof (iface),"interface=%lu" , tt->adapter_index); -argv_printf (&argv,"%s%sc interface ipv6 set address %s %s store=active" , +argv_printf (&argv,"%s%s interface ipv6 set address %s %s store=active" ,get_win_sys_path (),NETSH_PATH_SUFFIX , iface, ifconfig_ipv6_local);netsh_command (&argv,4 ,M_FATAL );
@@ -4873,14 +4873,14 @@ ipconfig_register_dns(const struct env_set *es)msg (D_TUNTAP_INFO ,"Start ipconfig commands for register-dns..." );netcmd_semaphore_lock (); -argv_printf (&argv,"%s%sc /flushdns" , +argv_printf (&argv,"%s%s /flushdns" ,get_win_sys_path (),WIN_IPCONFIG_PATH_SUFFIX );argv_msg (D_TUNTAP_INFO , &argv); status =openvpn_execve_check (&argv, es,0 , err);argv_reset (&argv); -argv_printf (&argv,"%s%sc /registerdns" , +argv_printf (&argv,"%s%s /registerdns" ,get_win_sys_path (),WIN_IPCONFIG_PATH_SUFFIX );argv_msg (D_TUNTAP_INFO , &argv);
@@ -4994,8 +4994,8 @@ netsh_set_dns6_servers(const struct in6_addr *addr_list,for (int i =0 ; i < addr_len; ++i) {const char *fmt = (i ==0 ) ? -"%s%sc interface ipv6 set dns %s static %s" - :"%s%sc interface ipv6 add dns %s %s" ; +"%s%s interface ipv6 set dns %s static %s" + :"%s%s interface ipv6 add dns %s %s" ;argv_printf (&argv, fmt,get_win_sys_path (),NETSH_PATH_SUFFIX , flex_name,print_in6_addr (addr_list[i],0 , &gc));
@@ -5042,7 +5042,7 @@ netsh_ifconfig_options(const char *type,/* delete existing DNS/WINS settings from TAP interface */ if (delete_first) { - argv_printf(&argv, "%s%sc interface ip delete %s %s all", + argv_printf(&argv, "%s%s interface ip delete %s %s all", get_win_sys_path(), NETSH_PATH_SUFFIX, type,
@@ -5059,8 +5059,8 @@ netsh_ifconfig_options(const char *type,if (delete_first || !test_first || !ip_addr_member_of (addr_list[i], current)) {const char *fmt = count ? -"%s%sc interface ip add %s %s %s" - :"%s%sc interface ip set %s %s static %s" ; +"%s%s interface ip add %s %s %s" + :"%s%s interface ip set %s %s static %s" ; argv_printf(&argv, fmt, get_win_sys_path(),
@@ -5136,7 +5136,7 @@ netsh_ifconfig(const struct tuntap_options *to,else {/* example: netsh interface ip set address my-tap static 10.3.0.1 255.255.255.0 */ - argv_printf(&argv, "%s%sc interface ip set address %s static %s %s", + argv_printf(&argv, "%s%s interface ip set address %s static %s %s", get_win_sys_path(), NETSH_PATH_SUFFIX, flex_name,
@@ -5185,7 +5185,7 @@ netsh_enable_dhcp(const struct tuntap_options *to,/* example: netsh interface ip set address my-tap dhcp */ argv_printf (&argv, -"%s%sc interface ip set address %s dhcp" , +"%s%s interface ip set address %s dhcp" ,get_win_sys_path (),NETSH_PATH_SUFFIX , actual_name);
@@ -6100,7 +6100,7 @@ close_tun(struct tuntap *tt)/* netsh interface ipv6 delete address \"%s\" %s */ ifconfig_ipv6_local =print_in6_addr (tt->local_ipv6,0 , &gc); argv_printf(&argv, - "%s%sc interface ipv6 delete address %s %s store=active", + "%s%s interface ipv6 delete address %s %s store=active", get_win_sys_path(), NETSH_PATH_SUFFIX, tt->actual_name,
@@ -6112,7 +6112,7 @@ close_tun(struct tuntap *tt)if (tt->options.dns6_len >0 ) {argv_printf (&argv, -"%s%sc interface ipv6 delete dns %s all" , +"%s%s interface ipv6 delete dns %s all" ,get_win_sys_path (),NETSH_PATH_SUFFIX , tt->actual_name);
tests/unit_tests/openvpn/test_argv.c +54 -4
97 lines not on this page · raw message
Signature · 1 line
2.17.1
PATCH 2/4 argv: do fewer memory re-allocations
In reply to David SommersethFrom: Heiko Hund <[email protected]...>
Prevent the re-allocations of memory when the internal argv grows beyond 2 and 4 arguments by initially allocating argv to hold up to 7 (+ trailing NULL) pointers.
While at it rename argv_reset to argv_free to actually express what's going on. Redo the argv_reset functionality so that it can be used to actually reset the argv without re-allocation.
Signed-off-by Heiko Hund <[email protected]...>
--- src/openvpn/argv.c | 81 ++++++++++++++-------------- src/openvpn/argv.h | 2 +- src/openvpn/console_systemd.c | 2 +- src/openvpn/init.c | 15 ++---- src/openvpn/lladdr.c | 2 +- src/openvpn/multi.c | 10 ++-- src/openvpn/options.c | 2 +- src/openvpn/plugin.c | 2 +- src/openvpn/route.c | 8 +-- src/openvpn/socket.c | 4 +- src/openvpn/ssl_verify.c | 6 +-- src/openvpn/tun.c | 38 ++++++------- tests/unit_tests/openvpn/test_argv.c | 41 +++++++++----- 13 files changed, 110 insertions(+), 103 deletions(-)
src/openvpn/argv.c +41 -40
@@ -40,34 +40,6 @@#include "env_set.h" #include "options.h" -static void -argv_init (struct argv *a) -{ - a->capacity =0 ; - a->argc =0 ; - a->argv =NULL ; -} - -struct argv -argv_new (void ) -{ -struct argv ret; -argv_init (&ret); -return ret; -} - -void -argv_reset (struct argv *a) -{ -size_t i; -for (i =0 ; i < a->argc; ++i) - { -free (a->argv[i]); - } -free (a->argv); -argv_init (a); -} -static void argv_extend (struct argv *a,const size_t newcap) {
@@ -86,6 +58,46 @@ argv_extend(struct argv *a, const size_t newcap) } } +static void +argv_init (struct argv *a) +{ + a->capacity =0 ; + a->argc =0 ; + a->argv =NULL ; +argv_extend (a,8 ); +} + +struct argv +argv_new (void ) +{ +struct argv ret; +argv_init (&ret); +return ret; +} + +void +argv_free (struct argv *a) +{ +size_t i; +for (i =0 ; i < a->argc; ++i) + { +free (a->argv[i]); + } +free (a->argv); +} + +static void +argv_reset (struct argv *a) +{ +size_t i; +for (i =0 ; i < a->argc; ++i) + { +free (a->argv[i]); + a->argv[i] =NULL ; + } + a->argc =0 ; +} +static void argv_grow (struct argv *a,const size_t add) {
@@ -134,14 +146,7 @@ argv_insert_head(const struct argv *a, const char *head)const char *argv_str (const struct argv *a,struct gc_arena *gc,const unsigned int flags) { -if (a->argv) - { -return print_argv ((const char **)a->argv, gc, flags); - } -else - { -return "" ; - } +return print_argv ((const char **)a->argv, gc, flags); }void
@@ -229,8 +234,6 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist)va_list tmplist;int len; -argv_extend (a,1 );/* ensure trailing NULL */ - argc = a->argc; f =argv_prep_format (format, delim, &argc, &gc);if (f ==NULL )
@@ -270,7 +273,6 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist) {/* Someone snuck in a GS (0x1D), fail gracefully */ argv_reset (a); -argv_extend (a,1 );/* ensure trailing NULL */ goto out; }
@@ -315,7 +317,6 @@ argv_parse_cmd(struct argv *a, const char *s)struct gc_arena gc =gc_new ();argv_reset (a); -argv_extend (a,1 );/* ensure trailing NULL */ nparms =parse_line (s, parms,MAX_PARMS ,"SCRIPT-ARGV" ,0 ,D_ARGV_PARSE_CMD , &gc);if (nparms)
src/openvpn/argv.h +1 -1
@@ -40,7 +40,7 @@ struct argv {struct argv argv_new (void ); -void argv_reset (struct argv *a); +void argv_free (struct argv *a);const char *argv_str (const struct argv *a,struct gc_arena *gc,const unsigned int flags);
src/openvpn/console_systemd.c +1 -1
@@ -85,7 +85,7 @@ get_console_input_systemd(const char *prompt, const bool echo, char *input, cons }close (std_out); -argv_reset (&argv); +argv_free (&argv);return ret; }
src/openvpn/init.c +3 -12
@@ -163,7 +163,7 @@ run_up_down(const char *command,msg (M_FATAL ,"ERROR: up/down plugin call failed" ); } -argv_reset (&argv); +argv_free (&argv); }if (command)
@@ -176,7 +176,7 @@ run_up_down(const char *command, ifconfig_local, ifconfig_remote, context);argv_msg (M_INFO , &argv);openvpn_run_script (&argv, es,S_FATAL ,"--up/--down" ); -argv_reset (&argv); +argv_free (&argv); }gc_free (&gc);
@@ -844,15 +844,6 @@ init_static(void)return false;#endif -#ifdef ARGV_TEST - { -void argv_test (void ); - -argv_test (); -return false; - } -#endif -#ifdef PRNG_TEST {struct gc_arena gc =gc_new ();
@@ -1639,7 +1630,7 @@ do_route(const struct options *options,setenv_str (es,"script_type" ,"route-up" );argv_parse_cmd (&argv, options->route_script);openvpn_run_script (&argv, es,0 ,"--route-up" ); -argv_reset (&argv); +argv_free (&argv); }#ifdef _WIN32
src/openvpn/lladdr.c +1 -1
@@ -69,6 +69,6 @@ set_lladdr(const char *ifname, const char *lladdr,msg (M_INFO ,"TUN/TAP link layer address set to %s" , lladdr); } -argv_reset (&argv); +argv_free (&argv);return r; }
src/openvpn/multi.c +5 -5
@@ -138,7 +138,7 @@ learn_address_script(const struct multi_context *m,msg (M_WARN ,"WARNING: learn-address plugin call failed" ); ret = false; } -argv_reset (&argv); +argv_free (&argv); }if (m->top.options.learn_address_script)
@@ -155,7 +155,7 @@ learn_address_script(const struct multi_context *m, { ret = false; } -argv_reset (&argv); +argv_free (&argv); }gc_free (&gc);
@@ -593,7 +593,7 @@ multi_client_disconnect_script(struct multi_context *m,setenv_str (mi->context.c2.es,"script_type" ,"client-disconnect" );argv_parse_cmd (&argv, mi->context.options.client_disconnect_script);openvpn_run_script (&argv, mi->context.c2.es,0 ,"--client-disconnect" ); -argv_reset (&argv); +argv_free (&argv); }#ifdef MANAGEMENT_DEF_AUTH if (management)
@@ -1908,7 +1908,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi) } script_depr_failed: -argv_reset (&argv); +argv_free (&argv); }/* V2 callback, use a plugin_return struct for passing back return info */
@@ -1971,7 +1971,7 @@ script_depr_failed: } script_failed: -argv_reset (&argv); +argv_free (&argv); }/*
src/openvpn/options.c +1 -1
@@ -3228,7 +3228,7 @@ check_cmd_access(const char *command, const char *opt, const char *chroot) return_code = true; } -argv_reset (&argv); +argv_free (&argv);return return_code; }
src/openvpn/plugin.c +1 -1
@@ -587,7 +587,7 @@ plugin_call_item(const struct plugin *p, p->so_pathname); } -argv_reset (&a); +argv_free (&a);gc_free (&gc); }return status;
src/openvpn/route.c +4 -4
@@ -1815,7 +1815,7 @@ done: { r->flags &= ~RT_ADDED ; } -argv_reset (&argv); +argv_free (&argv);gc_free (&gc); }
@@ -2122,7 +2122,7 @@ add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt, unsigned int flag { r6->flags &= ~RT_ADDED ; } -argv_reset (&argv); +argv_free (&argv);gc_free (&gc); }
@@ -2314,7 +2314,7 @@ delete_route(struct route_ipv4 *r, done: r->flags &= ~RT_ADDED ; -argv_reset (&argv); +argv_free (&argv);gc_free (&gc); }
@@ -2548,7 +2548,7 @@ delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt, unsignedmsg (M_FATAL ,"Sorry, but I don't know how to do 'route ipv6' commands on this operating system. Try putting your routes in a --route-down script" );#endif /* if defined(TARGET_LINUX) */ -argv_reset (&argv); +argv_free (&argv);gc_free (&gc); }
src/openvpn/socket.c +2 -2
@@ -2467,7 +2467,7 @@ link_socket_connection_initiated(const struct buffer *buf, {msg (M_WARN ,"WARNING: ipchange plugin call failed" ); } -argv_reset (&argv); +argv_free (&argv); }/* Process --ipchange option */
@@ -2477,7 +2477,7 @@ link_socket_connection_initiated(const struct buffer *buf,setenv_str (es,"script_type" ,"ipchange" );ipchange_fmt (true, &argv, info, &gc);openvpn_run_script (&argv, es,0 ,"--ipchange" ); -argv_reset (&argv); +argv_free (&argv); }gc_free (&gc);
src/openvpn/ssl_verify.c +3 -3
@@ -522,7 +522,7 @@ verify_cert_call_plugin(const struct plugin_list *plugins, struct env_set *es, ret =plugin_call_ssl (plugins,OPENVPN_PLUGIN_TLS_VERIFY , &argv,NULL , es, cert_depth, cert); -argv_reset (&argv); +argv_free (&argv);if (ret ==OPENVPN_PLUGIN_FUNC_SUCCESS ) {
@@ -615,7 +615,7 @@ verify_cert_call_command(const char *verify_command, struct env_set *es, cleanup:gc_free (&gc); -argv_reset (&argv); +argv_free (&argv);if (ret) {
@@ -1159,7 +1159,7 @@ done:platform_unlink (tmp_file); } -argv_reset (&argv); +argv_free (&argv);gc_free (&gc);return ret; }
src/openvpn/tun.c +19 -19
@@ -1012,7 +1012,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu,#endif /* outer "if defined(TARGET_xxx)" conditional */ gc_free (&gc); -argv_reset (&argv); +argv_free (&argv); }/**
@@ -1400,7 +1400,7 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,#endif /* if defined(TARGET_LINUX) */ gc_free (&gc); -argv_reset (&argv); +argv_free (&argv); }/* execute the ifconfig command through the shell */
@@ -1968,7 +1968,7 @@ undo_ifconfig_ipv4(struct tuntap *tt, struct gc_arena *gc)argv_msg (M_INFO , &argv);openvpn_execve_check (&argv,NULL ,0 ,"Linux ip addr del failed" ); -argv_reset (&argv); +argv_free (&argv); }void
@@ -1988,7 +1988,7 @@ undo_ifconfig_ipv6(struct tuntap *tt, struct gc_arena *gc)argv_msg (M_INFO , &argv);openvpn_execve_check (&argv,NULL ,0 ,"Linux ip -6 addr del failed" ); -argv_reset (&argv); +argv_free (&argv); }void
@@ -2280,7 +2280,7 @@ solaris_close_tun(struct tuntap *tt) IFCONFIG_PATH, tt->actual_name );argv_msg (M_INFO , &argv);openvpn_execve_check (&argv,NULL ,0 ,"Solaris ifconfig inet6 unplumb failed" ); -argv_reset (&argv); +argv_free (&argv); }if (tt->ip_fd >=0 )
@@ -2365,7 +2365,7 @@ solaris_error_close(struct tuntap *tt, const struct env_set *es,openvpn_execve_check (&argv, es,0 ,"Solaris ifconfig unplumb failed" );close_tun (tt);msg (M_FATAL ,"Solaris ifconfig failed" ); -argv_reset (&argv); +argv_free (&argv); }int
@@ -2453,7 +2453,7 @@ close_tun(struct tuntap *tt)openvpn_execve_check (&argv,NULL ,0 ,"OpenBSD 'destroy tun interface' failed (non-critical)" );free (tt); -argv_reset (&argv); +argv_free (&argv); }int
@@ -2539,7 +2539,7 @@ close_tun(struct tuntap *tt)openvpn_execve_check (&argv,NULL ,0 ,"NetBSD 'destroy tun interface' failed (non-critical)" );free (tt); -argv_reset (&argv); +argv_free (&argv); } static inline int
@@ -2680,7 +2680,7 @@ close_tun(struct tuntap *tt) "FreeBSD 'destroy tun interface' failed (non-critical)");free (tt); -argv_reset (&argv); +argv_free (&argv); }int
@@ -3045,7 +3045,7 @@ close_tun(struct tuntap *tt)close_tun_generic (tt);free (tt); -argv_reset (&argv); +argv_free (&argv);gc_free (&gc); }
@@ -3149,7 +3149,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tunenv_set_add ( es,"ODMDIR=/etc/objrepos" );openvpn_execve_check (&argv, es,S_FATAL ,"AIX 'create tun interface' failed" );env_set_destroy (es); -argv_reset (&argv); +argv_free (&argv); }else {
@@ -3200,7 +3200,7 @@ close_tun(struct tuntap *tt)free (tt);env_set_destroy (es); -argv_reset (&argv); +argv_free (&argv); }int
@@ -4878,14 +4878,14 @@ ipconfig_register_dns(const struct env_set *es) WIN_IPCONFIG_PATH_SUFFIX);argv_msg (D_TUNTAP_INFO , &argv); status =openvpn_execve_check (&argv, es,0 , err); -argv_reset (&argv); +argv_free (&argv);argv_printf (&argv,"%s%s /registerdns" ,get_win_sys_path (),WIN_IPCONFIG_PATH_SUFFIX );argv_msg (D_TUNTAP_INFO , &argv); status =openvpn_execve_check (&argv, es,0 , err); -argv_reset (&argv); +argv_free (&argv);netcmd_semaphore_release ();msg (D_TUNTAP_INFO ,"End ipconfig commands for register-dns..." );
@@ -5010,7 +5010,7 @@ netsh_set_dns6_servers(const struct in6_addr *addr_list,netsh_command (&argv,1 , (i==0 ) ?M_FATAL :M_NONFATAL ); } -argv_reset (&argv); +argv_free (&argv);gc_free (&gc); }
@@ -5082,7 +5082,7 @@ netsh_ifconfig_options(const char *type, } } -argv_reset (&argv); +argv_free (&argv);gc_free (&gc); }
@@ -5173,7 +5173,7 @@ netsh_ifconfig(const struct tuntap_options *to, BOOL_CAST(flags & NI_TEST_FIRST)); } -argv_reset (&argv); +argv_free (&argv);gc_free (&gc); }
@@ -5192,7 +5192,7 @@ netsh_enable_dhcp(const struct tuntap_options *to,netsh_command (&argv,4 ,M_FATAL ); -argv_reset (&argv); +argv_free (&argv); }/* Enable dhcp on tap adapter using iservice */
@@ -6118,7 +6118,7 @@ close_tun(struct tuntap *tt) tt->actual_name);netsh_command (&argv,1 ,M_WARN ); } -argv_reset (&argv); +argv_free (&argv); } }#if 1
tests/unit_tests/openvpn/test_argv.c +28 -13
115 lines not on this page · raw message
Signature · 1 line
2.17.1
PATCH 3/4 Add gc_arena to struct argv to save allocations
In reply to David SommersethFrom: Heiko Hund <[email protected]...>
With the private gc_arena we do not have to allocate the strings found during parsing again, since we know the arena they are allocated in is valid as long as the argv vector is.
Signed-off-by Heiko Hund <[email protected]...>
--- src/openvpn/argv.c | 44 ++++++++++++---------------- src/openvpn/argv.h | 1 + tests/unit_tests/openvpn/test_argv.c | 23 +++++++++++++++ 3 files changed, 43 insertions(+), 25 deletions(-)
src/openvpn/argv.c +19 -25
@@ -47,12 +47,11 @@ argv_extend(struct argv *a, const size_t newcap) {char **newargv;size_t i; - ALLOC_ARRAY_CLEAR(newargv, char *, newcap); + ALLOC_ARRAY_CLEAR_GC(newargv, char *, newcap, &a->gc);for (i =0 ; i < a->argc; ++i) { newargv[i] = a->argv[i]; } -free (a->argv); a->argv = newargv; a->capacity = newcap; }
@@ -64,6 +63,7 @@ argv_init(struct argv *a) a->capacity =0 ; a->argc =0 ; a->argv =NULL ; + a->gc =gc_new ();argv_extend (a,8 ); }
@@ -78,24 +78,21 @@ argv_new(void)void argv_free (struct argv *a) { -size_t i; -for (i =0 ; i < a->argc; ++i) - { -free (a->argv[i]); - } -free (a->argv); +gc_free (&a->gc); }static void argv_reset (struct argv *a) { -size_t i; -for (i =0 ; i < a->argc; ++i) +if (a->argc) { -free (a->argv[i]); - a->argv[i] =NULL ; +size_t i; +for (i =0 ; i < a->argc; ++i) + { + a->argv[i] =NULL ; + } + a->argc =0 ; } - a->argc =0 ; } static void
@@ -107,7 +104,7 @@ argv_grow(struct argv *a, const size_t add) }static void -argv_append (struct argv *a,char *str)/* str must have been malloced or be NULL */ +argv_append (struct argv *a,char *str)/* str must have been gc_malloced or be NULL */ {argv_grow (a,1 ); a->argv[a->argc++] = str;
@@ -128,7 +125,7 @@ argv_clone(const struct argv *a, const size_t headroom) {for (i =0 ; i < a->argc; ++i) { -argv_append (&r,string_alloc (a->argv[i],NULL )); +argv_append (&r,string_alloc (a->argv[i], &r.gc)); } }return r;
@@ -139,7 +136,7 @@ argv_insert_head(const struct argv *a, const char *head) {struct argv r; r =argv_clone (a,1 ); - r.argv[0 ] =string_alloc (head,NULL ); + r.argv[0 ] =string_alloc (head, &r.gc);return r; }
@@ -251,7 +248,7 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist) } size =adjust_power_of_2 (len +1 ); - buf =gc_malloc (size, false, &gc); + buf =gc_malloc (size, false, &a->gc); len =vsnprintf (buf, size, f, arglist);if (len <0 || len >= size) {
@@ -263,11 +260,11 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist)while (end) { *end ='\0' ; -argv_append (a,string_alloc (buf,NULL )); +argv_append (a, buf); buf = end +1 ; end =strchr (buf, delim); } -argv_append (a,string_alloc (buf,NULL )); +argv_append (a, buf);if (a->argc != argc) {
@@ -314,23 +311,20 @@ argv_parse_cmd(struct argv *a, const char *s) {int nparms;char *parms[MAX_PARMS +1 ]; -struct gc_arena gc =gc_new ();argv_reset (a); - nparms =parse_line (s, parms,MAX_PARMS ,"SCRIPT-ARGV" ,0 ,D_ARGV_PARSE_CMD , &gc); + nparms =parse_line (s, parms,MAX_PARMS ,"SCRIPT-ARGV" ,0 ,D_ARGV_PARSE_CMD , &a->gc);if (nparms) {int i;for (i =0 ; i < nparms; ++i) { -argv_append (a,string_alloc (parms[i],NULL )); +argv_append (a, parms[i]); } }else { -argv_append (a,string_alloc (s,NULL )); +argv_append (a,string_alloc (s, &a->gc)); } - -gc_free (&gc); }
src/openvpn/argv.h +1 -0
@@ -33,6 +33,7 @@#include "buffer.h" struct argv { +struct gc_arena gc;size_t capacity;size_t argc;char **argv;
tests/unit_tests/openvpn/test_argv.c +23 -0
@@ -117,6 +117,28 @@ argv_printf__empty_parameter__argc_correct(void **state)argv_free (&a); } +static void +argv_printf__long_args__data_correct (void **state) +{ +int i; +struct argv a =argv_new (); +const char *args[] = { +"good_tools_have_good_names_even_though_it_might_impair_typing" , +"--long-opt=looooooooooooooooooooooooooooooooooooooooooooooooong" , +"--long-cat=loooooooooooooooooooooooooooooooooooooooooooooooooooonger" , +"file_with_very_descriptive_filename_that_leaves_no_questions_open.jpg.exe" + }; + +argv_printf (&a,"%s %s %s %s" , args[0 ], args[1 ], args[2 ], args[3 ]); +assert_int_equal (a.argc,4 ); +for (i =0 ; i < a.argc; i++) + { +assert_string_equal (a.argv[i], args[i]); + } + +argv_free (&a); +} +static void argv_parse_cmd__command_string__argc_correct (void **state) {
@@ -237,6 +259,7 @@ main(void)cmocka_unit_test (argv_printf__group_sep_in_arg__fail_no_ouput),cmocka_unit_test (argv_printf__combined_path_with_spaces__argc_correct),cmocka_unit_test (argv_printf__empty_parameter__argc_correct), +cmocka_unit_test (argv_printf__long_args__data_correct),cmocka_unit_test (argv_parse_cmd__command_string__argc_correct),cmocka_unit_test (argv_parse_cmd__command_and_extra_options__argc_correct), cmocka_unit_test(argv_printf_cat__used_twice__argc_correct),
Signature · 1 line
2.17.1
PATCH 4/4 Documented all the argv related code with minor refactoring
In reply to David SommersethAdded doxygen comments for all the functions in argv.c.
There are some slight refactoring, renaming a few variables to make their use case more obvious and ensure lines do not break our 80-chars per line coding style limit.
Signed-off-by David Sommerseth <[email protected]...>
--- src/openvpn/argv.c | 246 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 209 insertions(+), 37 deletions(-)
src/openvpn/argv.c +209 -37
@@ -40,6 +40,13 @@#include "env_set.h" #include "options.h" +/** +* Resizes the list of arguments struct argv can carry. This resize +* operation will only increase the size, never decrease the size. +* +* @param *a Valid pointer to a struct argv to resize +* @param newcap size_t with the new size of the argument list. +*/ static void argv_extend (struct argv *a,const size_t newcap) {
@@ -57,6 +64,12 @@ argv_extend(struct argv *a, const size_t newcap) } } +/** +* Initialise an already allocated struct argv. +* It is expected that the input argument is a valid pointer. +* +* @param *a Pointer to a struct argv to initialise +*/ static void argv_init (struct argv *a) {
@@ -67,6 +80,12 @@ argv_init(struct argv *a)argv_extend (a,8 ); } +/** +* Allocates a new struct argv and ensures it is initialised. +* Note that it does not return a pointer, but a struct argv directly. +* +* @returns Returns an initialised and empty struct argv. +*/ struct argv argv_new (void ) {
@@ -75,12 +94,24 @@ argv_new(void)return ret; } +/** +* Frees all memory allocations allocated by the struct argv +* related functions. +* +* @param *a Valid pointer to a struct argv to release memory from +*/ void argv_free (struct argv *a) {gc_free (&a->gc); } +/** +* Resets the struct argv to an initial state. No memory buffers +* will be released by this call. +* +* @param *a Valid pointer to a struct argv to resize +*/ static void argv_reset (struct argv *a) {
@@ -95,6 +126,19 @@ argv_reset(struct argv *a) } } +/** +* Extends an existing struct argv to carry minimum 'add' number +* of new arguments. This builds on argv_extend(), which ensures the +* new size will only be higher than the current capacity. +* +* The new size is also calculated based on the result of adjust_power_of_2(). +* This approach ensures that the list does grow bulks and only when the +* current limit is reached. +* +* @param *a Valid pointer to the struct argv to extend +* @param add size_t with the number of elements to add. +* +*/ static void argv_grow (struct argv *a,const size_t add) {
@@ -103,15 +147,39 @@ argv_grow(struct argv *a, const size_t add)argv_extend (a,adjust_power_of_2 (newargc)); } +/** +* Appends a string to to the list of arguments stored in a struct argv +* This will ensure the list size in struct argv has the needed capacity to +* store the value. +* +* @param *a struct argv where to append the new string value +* @param *str Pointer to string to append. The provided string *MUST* have +* been malloc()ed or NULL. +*/ static void -argv_append (struct argv *a,char *str)/* str must have been gc_malloced or be NULL */ +argv_append (struct argv *a,char *str) {argv_grow (a,1 ); a->argv[a->argc++] = str; } +/** +* Clones a struct argv with all the contents to a new allocated struct argv. +* If 'headroom' is larger than 0, it will create a head-room in front of the +* values being copied from the source input. +* +* +* @param *source Valid pointer to the source struct argv to clone. It may +* be NULL. +* @param headroom Number of slots to leave empty in front of the slots +* copied from the source. +* +* @returns Returns a new struct argv containing a copy of the source +* struct argv, with the given headroom in front of the copy. +* +*/ static struct argv -argv_clone (const struct argv *a,const size_t headroom) +argv_clone (const struct argv *source,const size_t headroom) {struct argv r;size_t i;
@@ -121,16 +189,24 @@ argv_clone(const struct argv *a, const size_t headroom) {argv_append (&r,NULL ); } -if (a) +if (source) { -for (i =0 ; i < a->argc; ++i) +for (i =0 ; i < source->argc; ++i) { -argv_append (&r,string_alloc (a->argv[i], &r.gc)); +argv_append (&r,string_alloc (source->argv[i], &r.gc)); } }return r; } +/** +* Inserts an argument string in front of all other argument slots. +* +* @param *a Valid pointer to the struct argv to insert the argument into +* @param *head Pointer to the char * string with the argument to insert +* +* @returns Returns a new struct argv with the inserted argument in front +*/ struct argv argv_insert_head (const struct argv *a,const char *head) {
@@ -140,12 +216,32 @@ argv_insert_head(const struct argv *a, const char *head)return r; } +/** +* Generate a single string with all the arguments in a struct argv +* concatenated. +* +* @param *a Valid pointer to the struct argv with the arguments to list +* @param *gc Pointer to a struct gc_arena managed buffer +* @param flags Flags passed to the print_argv() function. +* +* @returns Returns a string generated by print_argv() with all the arguments +* concatenated. If the argument count is 0, it will return an empty +* string. The return string is allocated in the gc_arena managed +* buffer. If the gc_arena pointer is NULL, the returned string +* must be free()d explicitly to avoid memory leaks. +*/ const char *argv_str (const struct argv *a,struct gc_arena *gc,const unsigned int flags) {return print_argv ((const char **)a->argv, gc, flags); } +/** +* Write the arguments stored in a struct argv via the msg() command. +* +* @param msglev Integer with the message level used by msg(). +* @param *a Valid pointer to the struct argv with the arguments to write. +*/ void argv_msg (const int msglev,const struct argv *a) {
@@ -154,6 +250,15 @@ argv_msg(const int msglev, const struct argv *a)gc_free (&gc); } +/** +* Similar to argv_msg() but prefixes the messages being written with a +* given string. +* +* @param msglev Integer with the message level used by msg(). +* @param *a Valid pointer to the struct argv with the arguments to write +* @param *prefix Valid char * pointer to the prefix string +* +*/ void argv_msg_prefix (const int msglev,const struct argv *a,const char *prefix) {
@@ -162,16 +267,29 @@ argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix)gc_free (&gc); } - -/* -* argv_prep_format - prepare argv format string for further processing +/** +* Prepares 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. +* 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. +* +* @param *format Pointer to a the format string to process +* @param delim Char with the delimiter to use +* @param *count size_t pointer used to return the number of +* tokens (argument slots) found in the format string. +* @param *gc Pointer to a gc_arena managed buffer. +* +* @returns Returns a parsed format string (char *), together with the +* number of tokens parts found (via *count). The result string +* is allocated within the gc_arena managed buffer. If the +* gc_arena pointer is NULL, the returned string must be explicitly +* free()d to avoid memory leaks. */ static char * -argv_prep_format (const char *format,const char delim,size_t *count,struct gc_arena *gc) +argv_prep_format (const char *format,const char delim,size_t *count, +struct gc_arena *gc) {int i, j;char *f;
@@ -213,15 +331,28 @@ argv_prep_format(const char *format, const char delim, size_t *count, struct gc_return f; } -/* -* argv_printf_arglist - create a struct argv from a format string +/** +* Create a struct argv based on 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. +* 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. +* +* @param *argres Valid pointer to a struct argv where the resulting parsed +* arguments, based on the format string. +* @param *format Char* string with a printf() compliant format string +* @param arglist A va_list with the arguments to be consumed by the format +* string +* +* @returns Returns true if the parsing and processing was successfully. If +* the resulting number of arguments does not match the expected +* number of arguments (based on the format string), it is +* considered a failure, which returns false. This can happen if +* the ASCII Group Separator (GS - 0x1D) is put into the arguments +* list or format string. */ static bool -argv_printf_arglist (struct argv *a,const char *format,va_list arglist) +argv_printf_arglist (struct argv *argres,const char *format,va_list arglist) {struct gc_arena gc =gc_new ();const char delim =0x1D ;/* ASCII Group Separator (GS) */
@@ -231,14 +362,19 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist)va_list tmplist;int len; - argc = a->argc; + argc = argres->argc; f =argv_prep_format (format, delim, &argc, &gc);if (f ==NULL ) { goto out; } -/* determine minimum buffer size */ +/* +* Determine minimum buffer size. +* +* With C99, vsnprintf(NULL, 0, ...) will return the number of bytes +* it would have written, had the buffer been large enough. +*/ va_copy (tmplist, arglist); len =vsnprintf (NULL ,0 , f, tmplist);va_end (tmplist);
@@ -248,7 +384,7 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist) } size =adjust_power_of_2 (len +1 ); - buf =gc_malloc (size, false, &a->gc); + buf =gc_malloc (size, false, &argres->gc); len =vsnprintf (buf, size, f, arglist);if (len <0 || len >= size) {
@@ -260,16 +396,16 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist)while (end) { *end ='\0' ; -argv_append (a, buf); +argv_append (argres, buf); buf = end +1 ; end =strchr (buf, delim); } -argv_append (a, buf); +argv_append (argres, buf); -if (a->argc != argc) +if (argres->argc != argc) {/* Someone snuck in a GS (0x1D), fail gracefully */ -argv_reset (a); +argv_reset (argres); goto out; }
@@ -280,51 +416,87 @@ out:return res; } - - +/** +* printf() variant which populates a struct argv. It processes the +* format string with the provided arguments. For each space separator found +* in the format string, a new argument will be added to the resulting +* struct argv. +* +* This will always reset and ensure the result is based on a pristine +* struct argv. +* +* @param *argres Valid pointer to a struct argv where the result will be put. +* @param *format printf() compliant (char *) format string. +* +* @returns Returns true if the parsing was successful. See +* argv_printf_arglist() for more details. The parsed result will +* be put into argres. +*/ bool -argv_printf (struct argv *a,const char *format, ...) +argv_printf (struct argv *argres,const char *format, ...) {bool res;va_list arglist;va_start (arglist, format); -argv_reset (a); - res =argv_printf_arglist (a, format, arglist); +argv_reset (argres); + res =argv_printf_arglist (argres, format, arglist);va_end (arglist);return res; } +/** +* printf() inspired argv concatenation. Adds arguments to an existing +* struct argv and populets the argument slots based on the printf() based +* format string. +* +* @param *argres Valid pointer to a struct argv where the result will be put. +* @param *format printf() compliant (char *) format string. +* +* @returns Returns true if the parsing was successful. See +* argv_printf_arglist() for more details. The parsed result will +* be put into argres. +*/ bool -argv_printf_cat (struct argv *a,const char *format, ...) +argv_printf_cat (struct argv *argres,const char *format, ...) {bool res;va_list arglist;va_start (arglist, format); - res =argv_printf_arglist (a, format, arglist); + res =argv_printf_arglist (argres, format, arglist);va_end (arglist);return res; } +/** +* Parses a command string, tokenizes it and puts each element into a separate +* struct argv argument slot. +* +* @params *argres Valid pointer to a struct argv where the parsed result +* will be found. +* @params *cmdstr Char * based string to parse +* +*/ void -argv_parse_cmd (struct argv *a,const char *s) +argv_parse_cmd (struct argv *argres,const char *cmdstr) {int nparms;char *parms[MAX_PARMS +1 ]; -argv_reset (a); +argv_reset (argres); - nparms =parse_line (s, parms,MAX_PARMS ,"SCRIPT-ARGV" ,0 ,D_ARGV_PARSE_CMD , &a->gc); + nparms =parse_line (cmdstr, parms,MAX_PARMS ,"SCRIPT-ARGV" ,0 , +D_ARGV_PARSE_CMD , &argres->gc);if (nparms) {int i;for (i =0 ; i < nparms; ++i) { -argv_append (a, parms[i]); +argv_append (argres, parms[i]); } }else { -argv_append (a,string_alloc (s, &a->gc)); +argv_append (argres,string_alloc (cmdstr, &argres->gc)); } }
Signature · 1 line
2.17.1
Re: [Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()
In reply to David SommersethQuoted 22 lines · 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
0x1Das 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]...>
Quoted 7 lines
--- 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] <https://patchwork.openvpn.net/patch/71/>
[1]
<https://www.mail-archive.com/[email protected]/msg15834.html>
[2]
<https://www.mail-archive.com/[email protected]/msg15868.html>
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Inc
Re: [Openvpn-devel] [PATCH 2/4] argv: do fewer memory re-allocations
In reply to David SommersethQuoted 11 lines · David Sommerseth
From: Heiko Hund <[email protected]...>
Prevent the re-allocations of memory when the internal argv grows beyond 2 and 4 arguments by initially allocating argv to hold up to 7 (+ trailing NULL) pointers.
While at it rename
argv_resettoargv_freeto actually express what's going on. Redo theargv_resetfunctionality so that it can be used to actually reset the argv without re-allocation.Signed-off-by Heiko Hund <[email protected]...>
Quoted 15 lines
--- src/openvpn/argv.c | 81 ++++++++++++++-------------- src/openvpn/argv.h | 2 +- src/openvpn/console_systemd.c | 2 +- src/openvpn/init.c | 15 ++---- src/openvpn/lladdr.c | 2 +- src/openvpn/multi.c | 10 ++-- src/openvpn/options.c | 2 +- src/openvpn/plugin.c | 2 +- src/openvpn/route.c | 8 +-- src/openvpn/socket.c | 4 +- src/openvpn/ssl_verify.c | 6 +-- src/openvpn/tun.c | 38 ++++++------- tests/unit_tests/openvpn/test_argv.c | 41 +++++++++----- 13 files changed, 110 insertions(+), 103 deletions(-)
This is a pure rebase of [0, 1]. In my initial review, I questioned the need to keep argv_str(). I decided to keep the argv_str() as it makes the usage of struct argv based processing simpler; no need for the caller to know inner details of struct argv.
[0] <https://patchwork.openvpn.net/patch/72/>
[1] <http://mid.mail-archive.com/[email protected].....>
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Inc
Re: [Openvpn-devel] [PATCH 3/4] Add gc_arena to struct argv to save allocations
In reply to David SommersethQuoted 6 lines · David Sommerseth
From: Heiko Hund <[email protected]...>
With the private
gc_arenawe do not have to allocate the strings found during parsing again, since we know the arena they are allocated in is valid as long as the argv vector is.
Quoted 6 lines
Signed-off-by Heiko Hund <[email protected]...>--- src/openvpn/argv.c | 44 ++++++++++++---------------- src/openvpn/argv.h | 1 + tests/unit_tests/openvpn/test_argv.c | 23 +++++++++++++++ 3 files changed, 43 insertions(+), 25 deletions(-)
This is a simple rebase of [0, 1] on top of the previous two patches. Nothing more exciting to say about this. My last review didn't have any further remarks than "will test it"; which I've done :)
[0] <https://patchwork.openvpn.net/patch/74/>
[1]
<https://www.mail-archive.com/[email protected]/msg15837.html>
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Inc
Re: [Openvpn-devel] [PATCH 1/4] re-implement argv_printf_*()
In reply to David SommersethAm 19.10.18 um 17:56 schrieb David Sommerseth:
Quoted 105 lines
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
0x1Das 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(-)src/openvpn/argv.c +1
-59@@ -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 +
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 173
Quoted 10 lines
src/openvpn/argv.c +10
-0+/* +* 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).
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 175
Quoted 2 lines
src/openvpn/argv.c +2
-0+int i, j; +char *f;
All could be made inline with C99
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 185
Quoted 10 lines
src/openvpn/argv.c +10
-0+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;
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 194
Quoted 9 lines
src/openvpn/argv.c +9
-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)++;
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 198
Quoted 4 lines
src/openvpn/argv.c +4
-0+ +/* +* 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?
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 199
Quoted 1 line
src/openvpn/argv.c +1
-0+* The resulting format string will never start with a delimiter.
Okay. But why? I think this is done to ignore, leading spaces?
Quoted 5 lines
src/openvpn/argv.c +5
-0+*/ +if (j >0 ) + { + f[j++] = delim; + }
[...]
Quoted 9 lines
src/openvpn/argv.c +8
-0+/* +* 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)
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 230
Quoted 10 lines
src/openvpn/argv.c +7
-2{ -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.
Quoted 1 line
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.
Quoted 52 lines
src/openvpn/argv.c +9
-41-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);
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 250
Quoted 10 lines
src/openvpn/argv.c +5
-4+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
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 256
Quoted 6 lines
src/openvpn/argv.c +6
-0+ 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.
Quoted 44 lines
src/openvpn/argv.c +10
-32-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
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 275
Quoted 10 lines
src/openvpn/argv.c +6
-3- { -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.
Quoted 7 lines
src/openvpn/argv.c +4
-1-void + + +bool argv_printf (struct argv *a,const char *format, ...) { +bool res;
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 297
Quoted 10 lines
src/openvpn/argv.c +4
-2va_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=).
src/openvpn/argv.c argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) · line 308
Quoted 12 lines
src/openvpn/argv.c +4
-2-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