From patchwork Sat Nov 11 21:24:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Heiko Hund X-Patchwork-Id: 71 X-Patchwork-Delegate: davids@openvpn.net Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director1.mail.ord1d.rsapps.net ([172.27.255.1]) by backend31.mail.ord1d.rsapps.net (Dovecot) with LMTP id a1VlAJoFCFqpRgAAgoeIoA for ; Sun, 12 Nov 2017 03:26:02 -0500 Received: from proxy3.mail.iad3a.rsapps.net ([172.27.255.1]) by director1.mail.ord1d.rsapps.net (Dovecot) with LMTP id lthVAJkFCFqcSgAANGzteQ ; Sun, 12 Nov 2017 03:26:02 -0500 Received: from smtp33.gate.iad3a ([172.27.255.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by proxy3.mail.iad3a.rsapps.net (Dovecot) with LMTP id GkGEMZkFCFqaOgAAYaqY3Q ; Sun, 12 Nov 2017 03:26:01 -0500 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO X-Virus-Scanned: OK X-Orig-To: openvpnslackdevel@openvpn.net X-Originating-Ip: [216.34.181.88] Authentication-Results: smtp33.gate.iad3a.rsapps.net; iprev=pass policy.iprev="216.34.181.88"; spf=pass smtp.mailfrom="openvpn-devel-bounces@lists.sourceforge.net" smtp.helo="lists.sourceforge.net"; dkim=fail (signature verification failed) header.d=sourceforge.net; dkim=fail (signature verification failed) header.d=sf.net; dkim=fail (signature verification failed) header.d=sophos.com; dmarc=fail (p=none; dis=none) header.from=sophos.com X-Classification-ID: 1dc3d0ec-c783-11e7-beb8-bc305bf670cc-1-1 Received: from [216.34.181.88] ([216.34.181.88:10004] helo=lists.sourceforge.net) by smtp33.gate.iad3a.rsapps.net (envelope-from ) (ecelerity 4.2.1.56364 r(Core:4.2.1.14)) with ESMTPS (cipher=DHE-RSA-AES256-GCM-SHA384) id D7/30-02044-995080A5; Sun, 12 Nov 2017 03:26:01 -0500 Received: from localhost ([127.0.0.1] helo=sfs-ml-2.v29.ch3.sourceforge.com) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.89) (envelope-from ) id 1eDnZe-0000WH-Sx; Sun, 12 Nov 2017 08:25:02 +0000 Received: from sfi-mx-2.v28.ch3.sourceforge.com ([172.29.28.192] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from ) id 1eDnZe-0000W5-EM for openvpn-devel@lists.sourceforge.net; Sun, 12 Nov 2017 08:25:02 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Type:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=htR7suqoA7DowIUnWGfkvRK3HrD3FQzPvf7kemVoe8g=; b=QqkF5qJCFvdkv7/9xDwVIgWG1u 6g50XT8kJFhsp+Xe7X0KjZztapMG4yzCIIK6SLX+bpY7kSCR26ON5mWCX/73RtCTGaldBvEqpV4nO L10pdOovRJWVYliJjBCk0Rvo1h4hVQJJK3QfqEr5GWNcv9ALZ+zK82dTfHIYfhvKma6g=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Date:Subject: To:From:Sender:Reply-To:Cc:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=htR7suqoA7DowIUnWGfkvRK3HrD3FQzPvf7kemVoe8g=; b=YCt0bymarA4wXlkmO1ihCEVXGx nwVdRStlrXfxw57/liRZzg8Hhs6PTVzsxWWWHlyZIsT3tSzcmAu1Z5KE8NC4bjEogChhjYM8muxG0 NczDYsraJnStPeWhHNEr3Mnk/Uay9w5D2SVoSkM3+wRMOD17/66bOSuRLVa7bWh7nFa4=; Received: from mx2.sophos.com ([145.253.124.138]) by sfi-mx-2.v28.ch3.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) id 1eDnZc-0005iL-7R for openvpn-devel@lists.sourceforge.net; Sun, 12 Nov 2017 08:25:02 +0000 Received: from mx2.sophos.com (localhost.localdomain [127.0.0.1]) by localhost (Postfix) with SMTP id 984A48A800E for ; Sun, 12 Nov 2017 08:24:53 +0000 (GMT) Received: from de-wie-exch3a.green.sophos (unknown [10.60.70.61]) by mx2.sophos.com (Postfix) with ESMTPS id 51BEA8A800D for ; Sun, 12 Nov 2017 08:24:52 +0000 (GMT) Received: from localhost.localdomain (10.128.128.78) by de-wie-exch3a.green.sophos (10.60.70.61) with Microsoft SMTP Server (TLS) id 14.3.169.1; Sun, 12 Nov 2017 09:24:43 +0100 From: Heiko Hund To: Date: Sun, 12 Nov 2017 09:24:35 +0100 Message-ID: <20171112082435.15447-1-heiko.hund@sophos.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <1477672963-5724-6-git-send-email-heiko.hund@sophos.com> References: <1477672963-5724-6-git-send-email-heiko.hund@sophos.com> MIME-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sophos.com; h=from:to:subject:date:message-id:in-reply-to:references:mime-version:content-type; s=global; bh=htR7suqoA7DowIUnWGfkvRK3HrD3FQzPvf7kemVoe8g=; b=MpDecI+FiVTIuzMjvC61XSgH9L5UlyxvY3o65x6hCuaST3zIXQrZvkOpj2eebiUhrLTrvQiM5GE8yhdrXHGnP42ZOCCcGFIzykLjUmYXEVGdvLJotpv662n0YY7SaoVDz87ldtbIo1zVPloLHmVAIEEZ42ZeY8aKbQ/WykHZiMg= X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -0.0 SPF_PASS SPF: sender matches SPF record -0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1eDnZc-0005iL-7R Subject: [Openvpn-devel] [PATCHv2 5/7] re-implement argv_printf_*() X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox 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 | 253 ++++++++++++++++------------------- 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, 184 insertions(+), 163 deletions(-) diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c index 95bdfeac..afe8efff 100644 --- a/src/openvpn/argv.c +++ b/src/openvpn/argv.c @@ -130,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) { @@ -217,112 +159,141 @@ argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) gc_free(&gc); } -static void -argv_printf_arglist(struct argv *a, const char *format, va_list arglist) +/* + * 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) { - 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(&f)) != 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; - - 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); + in_token = false; + continue; } - else + + if (!in_token) { - argv_append(a, term); + ++*count; + if (f[0]) /* not after leading spaces */ + { + f[j++] = delim; + } } + + f[j++] = format[i]; + in_token = true; } + + return f; } -void +/* + * 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) +{ + struct gc_arena gc = gc_new(); + const char *delim = 035; + char *f, *buf, *end; + size_t argc, size; + bool res = false; + 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) + { + goto out; + } + + /* determine minimum buffer size */ + va_copy(tmplist, arglist); + len = vsnprintf(NULL, 0, f, tmplist); + va_end(tmplist); + if (len < 0) + { + goto out; + } + + 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; + } + + /* 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 (a->argc != argc) + { + /* Someone snuck in a \035, fail gracefully */ + argv_reset(a); + argv_extend(a, 1); /* ensure trailing NULL */ + goto out; + } + + res = true; + +out: + gc_free(&gc); + return res; +} + +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 diff --git a/src/openvpn/argv.h b/src/openvpn/argv.h index 7d0754c9..a24ba98f 100644 --- a/src/openvpn/argv.h +++ b/src/openvpn/argv.h @@ -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))) diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 8c71e6ec..02f7299f 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -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, diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 25831ce3..61d6b9eb 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -1565,7 +1565,7 @@ do_ifconfig(struct tuntap *tt, char iface[64]; openvpn_snprintf(iface, sizeof(iface), "interface=%lu", tt->adapter_index ); argv_printf(&argv, - "%s%sc interface ipv6 set address %s %s store=active", + "%s%s interface ipv6 set address %s %s store=active", get_win_sys_path(), NETSH_PATH_SUFFIX, iface, @@ -5023,14 +5023,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); @@ -5144,8 +5144,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)); @@ -5192,7 +5192,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, @@ -5209,8 +5209,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(), @@ -5286,7 +5286,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, @@ -5335,7 +5335,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); @@ -6197,7 +6197,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, @@ -6209,7 +6209,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); diff --git a/tests/unit_tests/openvpn/test_argv.c b/tests/unit_tests/openvpn/test_argv.c index 4a3ba559..a09e92fb 100644 --- a/tests/unit_tests/openvpn/test_argv.c +++ b/tests/unit_tests/openvpn/test_argv.c @@ -9,6 +9,7 @@ #include #include #include +#include #include "argv.h" #include "buffer.h" @@ -54,23 +55,69 @@ argv_printf_cat__multiple_spaces_in_format__parsed_as_one(void **state) } static void +argv_printf__embedded_format_directive__replaced_in_output(void **state) +{ + struct argv a = argv_new(); + + argv_printf(&a, "", PATH1); + assert_int_equal(a.argc, 1); + assert_string_equal(a.argv[0], ""); + + argv_reset(&a); +} + +static void +argv_printf__group_sep_in_arg__fail_no_ouput(void **state) +{ + struct argv a = argv_new(); + + assert_false(argv_printf(&a, "tool --do %s", "this\035--harmful")); + assert_int_equal(a.argc, 0); + + argv_reset(&a); +} + +static void argv_printf__combined_path_with_spaces__argc_correct(void **state) { struct argv a = argv_new(); - argv_printf(&a, "%s%sc", PATH1, PATH2); + argv_printf(&a, "%s%s", PATH1, PATH2); assert_int_equal(a.argc, 1); - argv_printf(&a, "%s%sc %d", PATH1, PATH2, 42); + argv_printf(&a, "%s%s %d", PATH1, PATH2, 42); assert_int_equal(a.argc, 2); - argv_printf(&a, "foo %s%sc %s x y", PATH2, PATH1, "foo"); + argv_printf(&a, "foo %s%s %s x y", PATH2, PATH1, "foo"); assert_int_equal(a.argc, 5); argv_reset(&a); } static void +argv_printf__empty_parameter__argc_correct(void **state) +{ + struct argv a = argv_new(); + + argv_printf(&a, "%s", ""); + assert_int_equal(a.argc, 1); + + argv_printf(&a, "%s %s", PATH1, ""); + assert_int_equal(a.argc, 2); + + argv_printf(&a, "%s %s %s", PATH1, "", PARAM1); + assert_int_equal(a.argc, 3); + + argv_printf(&a, "%s %s %s %s", PATH1, "", "", PARAM1); + assert_int_equal(a.argc, 4); + + argv_printf(&a, "%s %s", "", PARAM1); + assert_int_equal(a.argc, 2); + + argv_free(&a); +} + +static void argv_parse_cmd__command_string__argc_correct(void **state) { struct argv a = argv_new(); @@ -113,7 +160,7 @@ argv_str__multiple_argv__correct_output(void **state) struct gc_arena gc = gc_new(); const char *output; - argv_printf(&a, "%s%sc", PATH1, PATH2); + argv_printf(&a, "%s%s", PATH1, PATH2); argv_printf_cat(&a, "%s", PARAM1); argv_printf_cat(&a, "%s", PARAM2); output = argv_str(&a, &gc, PA_BRACKET); @@ -168,7 +215,10 @@ main(void) const struct CMUnitTest tests[] = { cmocka_unit_test(argv_printf__multiple_spaces_in_format__parsed_as_one), cmocka_unit_test(argv_printf_cat__multiple_spaces_in_format__parsed_as_one), + cmocka_unit_test(argv_printf__embedded_format_directive__replaced_in_output), + 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_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), From patchwork Sat Nov 11 21:27:36 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Heiko Hund X-Patchwork-Id: 72 X-Patchwork-Delegate: davids@openvpn.net Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director3.mail.ord1d.rsapps.net ([172.28.255.1]) by backend31.mail.ord1d.rsapps.net (Dovecot) with LMTP id a5BWLj4GCFqrRgAAgoeIoA for ; Sun, 12 Nov 2017 03:28:46 -0500 Received: from director5.mail.ord1c.rsapps.net ([172.28.255.1]) by director3.mail.ord1d.rsapps.net (Dovecot) with LMTP id s6D+LT4GCFq3NwAAkXNnRw ; Sun, 12 Nov 2017 03:28:46 -0500 Received: from smtp52.gate.ord1c ([172.28.255.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by director5.mail.ord1c.rsapps.net (Dovecot) with LMTP id 0j+5LD4GCFpQMwAAH8LYwg ; Sun, 12 Nov 2017 03:28:46 -0500 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO X-Virus-Scanned: OK X-Orig-To: openvpnslackdevel@openvpn.net X-Originating-Ip: [216.34.181.88] Authentication-Results: smtp52.gate.ord1c.rsapps.net; iprev=pass policy.iprev="216.34.181.88"; spf=pass smtp.mailfrom="openvpn-devel-bounces@lists.sourceforge.net" smtp.helo="lists.sourceforge.net"; dkim=fail (signature verification failed) header.d=sourceforge.net; dkim=fail (signature verification failed) header.d=sf.net; dkim=fail (signature verification failed) header.d=sophos.com; dmarc=fail (p=none; dis=none) header.from=sophos.com X-Classification-ID: 80402086-c783-11e7-8180-b8ca3a5c9d28-1-1 Received: from [216.34.181.88] ([216.34.181.88:43802] helo=lists.sourceforge.net) by smtp52.gate.ord1c.rsapps.net (envelope-from ) (ecelerity 4.2.1.56364 r(Core:4.2.1.14)) with ESMTPS (cipher=DHE-RSA-AES256-GCM-SHA384) id 59/B3-08471-E36080A5; Sun, 12 Nov 2017 03:28:46 -0500 Received: from localhost ([127.0.0.1] helo=sfs-ml-2.v29.ch3.sourceforge.com) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.89) (envelope-from ) id 1eDnca-0000h1-CA; Sun, 12 Nov 2017 08:28:04 +0000 Received: from sfi-mx-1.v28.ch3.sourceforge.com ([172.29.28.191] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from ) id 1eDncZ-0000gs-Kp for openvpn-devel@lists.sourceforge.net; Sun, 12 Nov 2017 08:28:03 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Type:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=gtAgYSxR+pJs+1MNDVMqClg6ul4syBnif5NylwcL8Ew=; b=mp2xEF0ZAo93mp5hndKRup5hnN mCNPoWm6Zi2bMgEif2T0lfHyI7ac3Ouxh3Wod/m/2ZZi9zyvvKzr4V6+a40Y9Mg8UkxDYtHz4R7pN s1OOm5e0CVIacL32xpVOWCUnyJt07UfWwy+E6vnGo5Ll85/jS3i8ZdwGippkblOzR6nE=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Date:Subject: To:From:Sender:Reply-To:Cc:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=gtAgYSxR+pJs+1MNDVMqClg6ul4syBnif5NylwcL8Ew=; b=Kh/rk3+W/NBDS9eaZr+MWnysDQ dGePJO0sW+n77pot6MjPvLX9qg2G594OVtJGU25KhwfUKvfN+vcIm0xpIPLoMjqxDUnciEqhzMR3I 41TIQ38qVbi/CP33+dCljXsfiA4qbOSu4WSBuZJ0g5B4K9c/S7MbtH6ddOzuXKLYzUZg=; Received: from mx1.sophos.com ([145.253.124.137]) by sfi-mx-1.v28.ch3.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) id 1eDncX-000623-34 for openvpn-devel@lists.sourceforge.net; Sun, 12 Nov 2017 08:28:03 +0000 Received: from mx1.sophos.com (localhost.localdomain [127.0.0.1]) by localhost (Postfix) with SMTP id A8D48888853 for ; Sun, 12 Nov 2017 08:27:54 +0000 (GMT) Received: from de-wie-exch3a.green.sophos (unknown [10.60.70.61]) by mx1.sophos.com (Postfix) with ESMTPS id 95C02888823 for ; Sun, 12 Nov 2017 08:27:54 +0000 (GMT) Received: from localhost.localdomain (10.128.128.78) by de-wie-exch3a.green.sophos (10.60.70.61) with Microsoft SMTP Server (TLS) id 14.3.169.1; Sun, 12 Nov 2017 09:27:42 +0100 From: Heiko Hund To: Date: Sun, 12 Nov 2017 09:27:36 +0100 Message-ID: <20171112082736.15578-1-heiko.hund@sophos.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <1477672963-5724-7-git-send-email-heiko.hund@sophos.com> References: <1477672963-5724-7-git-send-email-heiko.hund@sophos.com> MIME-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sophos.com; h=from:to:subject:date:message-id:in-reply-to:references:mime-version:content-type; s=global; bh=gtAgYSxR+pJs+1MNDVMqClg6ul4syBnif5NylwcL8Ew=; b=J3CANd9evQFRg4rFw59EzXoEf4HHbGybwEMCMz57/ZfLlo0/7KikobNWs7bD3EkrjRHpcBxTGP6ORZkww+1MhxF/lubyp/xEfsSq69MGqOGTRZORZOQ8Fc77B3/Dp0O0FOwFU+sxKaPFY2i58J/sg73PS4T3VOcL7iXB0n5e3Mc= X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -0.0 SPF_PASS SPF: sender matches SPF record -0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1eDncX-000623-34 Subject: [Openvpn-devel] [PATCHv2 6/7] argv: do fewer memory re-allocations X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox 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 --- src/openvpn/argv.c | 53 ++++++++++++++++++------------------ 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 | 32 ++++++++++++---------- tests/unit_tests/openvpn/test_argv.c | 41 +++++++++++++++++++--------- 13 files changed, 94 insertions(+), 85 deletions(-) diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c index afe8efff..419b1dc6 100644 --- a/src/openvpn/argv.c +++ b/src/openvpn/argv.c @@ -40,11 +40,30 @@ #include "options.h" static void +argv_extend(struct argv *a, const size_t newcap) +{ + if (newcap > a->capacity) + { + char **newargv; + size_t i; + ALLOC_ARRAY_CLEAR(newargv, char *, newcap); + for (i = 0; i < a->argc; ++i) + { + newargv[i] = a->argv[i]; + } + free(a->argv); + a->argv = newargv; + a->capacity = newcap; + } +} + +static void argv_init(struct argv *a) { a->capacity = 0; a->argc = 0; a->argv = NULL; + argv_extend(a, 8); } struct argv @@ -56,7 +75,7 @@ argv_new(void) } void -argv_reset(struct argv *a) +argv_free(struct argv *a) { size_t i; for (i = 0; i < a->argc; ++i) @@ -64,25 +83,18 @@ argv_reset(struct argv *a) free(a->argv[i]); } free(a->argv); - argv_init(a); } static void -argv_extend(struct argv *a, const size_t newcap) +argv_reset(struct argv *a) { - if (newcap > a->capacity) + size_t i; + for (i = 0; i < a->argc; ++i) { - char **newargv; - size_t i; - ALLOC_ARRAY_CLEAR(newargv, char *, newcap); - for (i = 0; i < a->argc; ++i) - { - newargv[i] = a->argv[i]; - } - free(a->argv); - a->argv = newargv; - a->capacity = newcap; + free(a->argv[i]); + a->argv[i] = NULL; } + a->argc = 0; } static void @@ -133,14 +145,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 @@ -221,8 +226,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) @@ -262,7 +265,6 @@ argv_printf_arglist(struct argv *a, const char *format, va_list arglist) { /* Someone snuck in a \035, fail gracefully */ argv_reset(a); - argv_extend(a, 1); /* ensure trailing NULL */ goto out; } @@ -304,7 +306,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) diff --git a/src/openvpn/argv.h b/src/openvpn/argv.h index a24ba98f..2a1945e3 100644 --- a/src/openvpn/argv.h +++ b/src/openvpn/argv.h @@ -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); diff --git a/src/openvpn/console_systemd.c b/src/openvpn/console_systemd.c index e7a72ae3..5b09dee4 100644 --- a/src/openvpn/console_systemd.c +++ b/src/openvpn/console_systemd.c @@ -84,7 +84,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; } diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 1ed2c55e..25962958 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -162,7 +162,7 @@ run_up_down(const char *command, msg(M_FATAL, "ERROR: up/down plugin call failed"); } - argv_reset(&argv); + argv_free(&argv); } if (command) @@ -175,7 +175,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); @@ -824,15 +824,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(); @@ -1627,7 +1618,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 diff --git a/src/openvpn/lladdr.c b/src/openvpn/lladdr.c index ff71e48c..4df4151a 100644 --- a/src/openvpn/lladdr.c +++ b/src/openvpn/lladdr.c @@ -67,6 +67,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; } diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 4545bce1..df0531e0 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -136,7 +136,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) @@ -153,7 +153,7 @@ learn_address_script(const struct multi_context *m, { ret = false; } - argv_reset(&argv); + argv_free(&argv); } gc_free(&gc); @@ -594,7 +594,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 */ @@ -1970,7 +1970,7 @@ script_depr_failed: } script_failed: - argv_reset(&argv); + argv_free(&argv); } /* diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 7aa311aa..edab9066 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -3245,7 +3245,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; } diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c index 557b6bc7..65b113af 100644 --- a/src/openvpn/plugin.c +++ b/src/openvpn/plugin.c @@ -591,7 +591,7 @@ plugin_call_item(const struct plugin *p, p->so_pathname); } - argv_reset(&a); + argv_free(&a); gc_free(&gc); } return status; diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 02f7299f..0e4237d6 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -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, unsigned msg(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); } diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 0fc91f21..2f373116 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -2334,7 +2334,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 */ @@ -2344,7 +2344,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); diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 9cd36d7a..65342860 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -524,7 +524,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) { @@ -610,7 +610,7 @@ verify_cert_call_command(const char *verify_command, struct env_set *es, } gc_free(&gc); - argv_reset(&argv); + argv_free(&argv); if (ret) { @@ -1153,7 +1153,7 @@ done: platform_unlink(tmp_file); } - argv_reset(&argv); + argv_free(&argv); gc_free(&gc); return ret; } diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 61d6b9eb..d0e56bf1 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -1584,7 +1584,7 @@ do_ifconfig(struct tuntap *tt, #else /* if defined(TARGET_LINUX) */ msg(M_FATAL, "Sorry, but I don't know how to do 'ifconfig' commands on this operating system. You should ifconfig your TUN/TAP device manually or use an --up script."); #endif /* if defined(TARGET_LINUX) */ - argv_reset(&argv); + argv_free(&argv); } gc_free(&gc); } @@ -2166,7 +2166,7 @@ close_tun(struct tuntap *tt) #endif } - argv_reset(&argv); + argv_free(&argv); gc_free(&gc); } close_tun_generic(tt); @@ -2439,7 +2439,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) @@ -2526,7 +2526,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 @@ -2598,7 +2598,6 @@ close_tun(struct tuntap *tt) } else if (tt) { - struct gc_arena gc = gc_new(); struct argv argv = argv_new(); /* setup command, close tun dev (clears tt->actual_name!), run command @@ -2612,6 +2611,7 @@ close_tun(struct tuntap *tt) argv_msg(M_INFO, &argv); openvpn_execve_check(&argv, NULL, 0, "OpenBSD 'destroy tun interface' failed (non-critical)"); + argv_free(&argv); free(tt); } } @@ -2683,7 +2683,6 @@ close_tun(struct tuntap *tt) } else if (tt) { - struct gc_arena gc = gc_new(); struct argv argv = argv_new(); /* setup command, close tun dev (clears tt->actual_name!), run command @@ -2697,6 +2696,7 @@ close_tun(struct tuntap *tt) argv_msg(M_INFO, &argv); openvpn_execve_check(&argv, NULL, 0, "NetBSD 'destroy tun interface' failed (non-critical)"); + argv_free(&argv); free(tt); } } @@ -2834,6 +2834,7 @@ close_tun(struct tuntap *tt) argv_msg(M_INFO, &argv); openvpn_execve_check(&argv, NULL, 0, "FreeBSD 'destroy tun interface' failed (non-critical)"); + argv_free(&argv); free(tt); } } @@ -3201,7 +3202,7 @@ close_tun(struct tuntap *tt) close_tun_generic(tt); free(tt); - argv_reset(&argv); + argv_free(&argv); gc_free(&gc); } } @@ -3306,6 +3307,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun env_set_add( es, "ODMDIR=/etc/objrepos" ); openvpn_execve_check(&argv, es, S_FATAL, "AIX 'create tun interface' failed"); env_set_destroy(es); + argv_free(&argv); } else { @@ -3331,7 +3333,6 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun void close_tun(struct tuntap *tt) { - struct gc_arena gc = gc_new(); struct argv argv = argv_new(); struct env_set *es = env_set_create(NULL); @@ -3360,6 +3361,7 @@ close_tun(struct tuntap *tt) free(tt); env_set_destroy(es); + argv_free(&argv); } int @@ -5028,14 +5030,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..."); @@ -5160,7 +5162,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); } @@ -5232,7 +5234,7 @@ netsh_ifconfig_options(const char *type, } } - argv_reset(&argv); + argv_free(&argv); gc_free(&gc); } @@ -5323,7 +5325,7 @@ netsh_ifconfig(const struct tuntap_options *to, BOOL_CAST(flags & NI_TEST_FIRST)); } - argv_reset(&argv); + argv_free(&argv); gc_free(&gc); } @@ -5342,7 +5344,7 @@ netsh_enable_dhcp(const struct tuntap_options *to, netsh_command(&argv, 4, M_FATAL); - argv_reset(&argv); + argv_free(&argv); } /* @@ -6215,7 +6217,7 @@ close_tun(struct tuntap *tt) tt->actual_name); netsh_command(&argv, 1, M_WARN); } - argv_reset(&argv); + argv_free(&argv); } } #if 1 diff --git a/tests/unit_tests/openvpn/test_argv.c b/tests/unit_tests/openvpn/test_argv.c index a09e92fb..e15e2fe5 100644 --- a/tests/unit_tests/openvpn/test_argv.c +++ b/tests/unit_tests/openvpn/test_argv.c @@ -39,7 +39,7 @@ argv_printf__multiple_spaces_in_format__parsed_as_one(void **state) argv_printf(&a, " %s %s %d ", PATH1, PATH2, 42); assert_int_equal(a.argc, 3); - argv_reset(&a); + argv_free(&a); } static void @@ -51,7 +51,7 @@ argv_printf_cat__multiple_spaces_in_format__parsed_as_one(void **state) argv_printf_cat(&a, " %s %s", PATH2, PARAM1); assert_int_equal(a.argc, 3); - argv_reset(&a); + argv_free(&a); } static void @@ -63,7 +63,7 @@ argv_printf__embedded_format_directive__replaced_in_output(void **state) assert_int_equal(a.argc, 1); assert_string_equal(a.argv[0], ""); - argv_reset(&a); + argv_free(&a); } static void @@ -74,7 +74,7 @@ argv_printf__group_sep_in_arg__fail_no_ouput(void **state) assert_false(argv_printf(&a, "tool --do %s", "this\035--harmful")); assert_int_equal(a.argc, 0); - argv_reset(&a); + argv_free(&a); } static void @@ -91,7 +91,7 @@ argv_printf__combined_path_with_spaces__argc_correct(void **state) argv_printf(&a, "foo %s%s %s x y", PATH2, PATH1, "foo"); assert_int_equal(a.argc, 5); - argv_reset(&a); + argv_free(&a); } static void @@ -125,7 +125,7 @@ argv_parse_cmd__command_string__argc_correct(void **state) argv_parse_cmd(&a, SCRIPT_CMD); assert_int_equal(a.argc, 3); - argv_reset(&a); + argv_free(&a); } static void @@ -137,7 +137,7 @@ argv_parse_cmd__command_and_extra_options__argc_correct(void **state) argv_printf_cat(&a, "bar baz %d %s", 42, PATH1); assert_int_equal(a.argc, 7); - argv_reset(&a); + argv_free(&a); } static void @@ -150,7 +150,21 @@ argv_printf_cat__used_twice__argc_correct(void **state) argv_printf_cat(&a, "foo"); assert_int_equal(a.argc, 5); - argv_reset(&a); + argv_free(&a); +} + +static void +argv_str__empty_argv__empty_output(void **state) +{ + struct argv a = argv_new(); + struct gc_arena gc = gc_new(); + const char *output; + + output = argv_str(&a, &gc, PA_BRACKET); + assert_string_equal(output, ""); + + argv_free(&a); + gc_free(&gc); } static void @@ -166,7 +180,7 @@ argv_str__multiple_argv__correct_output(void **state) output = argv_str(&a, &gc, PA_BRACKET); assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]"); - argv_reset(&a); + argv_free(&a); gc_free(&gc); } @@ -179,9 +193,9 @@ argv_insert_head__empty_argv__head_only(void **state) b = argv_insert_head(&a, PATH1); assert_int_equal(b.argc, 1); assert_string_equal(b.argv[0], PATH1); - argv_reset(&b); + argv_free(&b); - argv_reset(&a); + argv_free(&a); } static void @@ -204,9 +218,9 @@ argv_insert_head__non_empty_argv__head_added(void **state) assert_string_equal(b.argv[i], a.argv[i - 1]); } } - argv_reset(&b); + argv_free(&b); - argv_reset(&a); + argv_free(&a); } int @@ -222,6 +236,7 @@ main(void) 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), + cmocka_unit_test(argv_str__empty_argv__empty_output), cmocka_unit_test(argv_str__multiple_argv__correct_output), cmocka_unit_test(argv_insert_head__non_empty_argv__head_added), }; From patchwork Sat Nov 11 21:29:15 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Heiko Hund X-Patchwork-Id: 73 X-Patchwork-Delegate: davids@openvpn.net Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director3.mail.ord1d.rsapps.net ([172.30.191.6]) by backend31.mail.ord1d.rsapps.net (Dovecot) with LMTP id KyVMFp4GCFqrRgAAgoeIoA for ; Sun, 12 Nov 2017 03:30:22 -0500 Received: from proxy1.mail.ord1d.rsapps.net ([172.30.191.6]) by director3.mail.ord1d.rsapps.net (Dovecot) with LMTP id 0dPoAp4GCFo1OQAAkXNnRw ; Sun, 12 Nov 2017 03:30:22 -0500 Received: from smtp26.gate.ord1c ([172.30.191.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by proxy1.mail.ord1d.rsapps.net (Dovecot) with LMTP id MvcZFJ4GCFqODQAAasrz9Q ; Sun, 12 Nov 2017 03:30:22 -0500 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO X-Virus-Scanned: OK X-Orig-To: openvpnslackdevel@openvpn.net X-Originating-Ip: [216.34.181.88] Authentication-Results: smtp26.gate.ord1c.rsapps.net; iprev=pass policy.iprev="216.34.181.88"; spf=pass smtp.mailfrom="openvpn-devel-bounces@lists.sourceforge.net" smtp.helo="lists.sourceforge.net"; dkim=fail (signature verification failed) header.d=sourceforge.net; dkim=fail (signature verification failed) header.d=sf.net; dkim=fail (signature verification failed) header.d=sophos.com; dmarc=fail (p=none; dis=none) header.from=sophos.com X-Classification-ID: b93327f8-c783-11e7-a60f-a4badb0c5489-1-1 Received: from [216.34.181.88] ([216.34.181.88:19571] helo=lists.sourceforge.net) by smtp26.gate.ord1c.rsapps.net (envelope-from ) (ecelerity 4.2.1.56364 r(Core:4.2.1.14)) with ESMTPS (cipher=DHE-RSA-AES256-GCM-SHA384) id A9/6E-06059-E96080A5; Sun, 12 Nov 2017 03:30:22 -0500 Received: from localhost ([127.0.0.1] helo=sfs-ml-4.v29.ch3.sourceforge.com) by sfs-ml-4.v29.ch3.sourceforge.com with esmtp (Exim 4.89) (envelope-from ) id 1eDneF-0003Mf-7Q; Sun, 12 Nov 2017 08:29:47 +0000 Received: from sfi-mx-3.v28.ch3.sourceforge.com ([172.29.28.193] helo=mx.sourceforge.net) by sfs-ml-4.v29.ch3.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from ) id 1eDneD-0003MY-Tu for openvpn-devel@lists.sourceforge.net; Sun, 12 Nov 2017 08:29:45 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Message-ID:Content-Type:MIME-Version:References: In-Reply-To:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Transfer-Encoding :Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=AniQmNFOG1ojYDkltd+ooYvuVZO0dkYKdpwzI/s3/E8=; b=Hz2eF+7Gc+eNfdnGdOBPPkx768 NZlbRu8wuJC6ZFC+BmF6B5ncc7KBKd3af58Jxgiq2fye8v+A1wysXMlhUvjJqbYTy2Ww6TSjkGmta 0aDqCPYJZY4UaJ6JjYMws1a+rwJOd/P+Y8prDO8rrVRW4icyUEyy4GqCWjNpC07XuDM4=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Message-ID:Content-Type:MIME-Version:References:In-Reply-To:Date:Subject: To:From:Sender:Reply-To:Cc:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=AniQmNFOG1ojYDkltd+ooYvuVZO0dkYKdpwzI/s3/E8=; b=AHgfNQifyTfvGanlNEpN7S6azn 9Zf8ww1UWrmklEPiAEeT5YilJsd/+KUkBH+VqOxyFM8jYR7VktzxTBy8HPZ49kFYB0PxMn0uHu+GW r6INfvwa6I94Z78r0IMckg2H38k11Y5nLmHh3Kl7DQuNXYNPVOZ7Gl3IvTnALT8pCe04=; Received: from mx1.sophos.com ([145.253.124.137]) by sfi-mx-3.v28.ch3.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) id 1eDneB-0005wP-Ii for openvpn-devel@lists.sourceforge.net; Sun, 12 Nov 2017 08:29:45 +0000 Received: from mx1.sophos.com (localhost.localdomain [127.0.0.1]) by localhost (Postfix) with SMTP id 228C1888853 for ; Sun, 12 Nov 2017 08:29:37 +0000 (GMT) Received: from de-wie-exch3a.green.sophos (unknown [10.60.70.61]) by mx1.sophos.com (Postfix) with ESMTPS id 148F7888823 for ; Sun, 12 Nov 2017 08:29:37 +0000 (GMT) Received: from localhost.localdomain (10.128.128.78) by de-wie-exch3a.green.sophos (10.60.70.61) with Microsoft SMTP Server (TLS) id 14.3.169.1; Sun, 12 Nov 2017 09:29:21 +0100 From: Heiko Hund To: <1477672963-5724-7-git-send-email-heiko.hund@sophos.com>, Date: Sun, 12 Nov 2017 09:29:15 +0100 X-Mailer: git-send-email 2.11.0 In-Reply-To: <1477672963-5724-8-git-send-email-heiko.hund@sophos.com> References: <1477672963-5724-8-git-send-email-heiko.hund@sophos.com> MIME-Version: 1.0 Message-ID: <7c0332b7-f19c-4290-bbbb-ae8cf136f116@DE-WIE-EXCH3A.green.sophos> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sophos.com; h=from:to:subject:date:in-reply-to:references:mime-version:content-type:message-id; s=global; bh=AniQmNFOG1ojYDkltd+ooYvuVZO0dkYKdpwzI/s3/E8=; b=vgrVl4m2M6IwP9LJ7RRB886Xkx2cTVDe8iH+d6MVPumL2DXxYM+JYYdQpNwY2TJ30u0xejXxuFTczgCcHfKVvWFHyzdHbBTlIqpufOIoP3/P+Jp+yOmQ2DbiMJ7Ug8yCnxeP+c2t+B8kWQGV+Nh1NwXtLq6ig1WkMMRjsktXDAk= X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -0.0 SPF_PASS SPF: sender matches SPF record -0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.0 AWL AWL: Adjusted score from AWL reputation of From: address X-Headers-End: 1eDneB-0005wP-Ii Subject: [Openvpn-devel] [PATCHv2 7/7] Add gc_arena to struct argv to save allocations X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox 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 --- 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(-) diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c index 419b1dc6..51dd1b22 100644 --- a/src/openvpn/argv.c +++ b/src/openvpn/argv.c @@ -46,12 +46,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; } @@ -63,6 +62,7 @@ argv_init(struct argv *a) a->capacity = 0; a->argc = 0; a->argv = NULL; + a->gc = gc_new(); argv_extend(a, 8); } @@ -77,24 +77,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 @@ -106,7 +103,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; @@ -127,7 +124,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; @@ -138,7 +135,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; } @@ -243,7 +240,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) { @@ -255,11 +252,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) { @@ -303,23 +300,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); } diff --git a/src/openvpn/argv.h b/src/openvpn/argv.h index 2a1945e3..a1aa7ebb 100644 --- a/src/openvpn/argv.h +++ b/src/openvpn/argv.h @@ -33,6 +33,7 @@ #include "buffer.h" struct argv { + struct gc_arena gc; size_t capacity; size_t argc; char **argv; diff --git a/tests/unit_tests/openvpn/test_argv.c b/tests/unit_tests/openvpn/test_argv.c index e15e2fe5..5bfef8af 100644 --- a/tests/unit_tests/openvpn/test_argv.c +++ b/tests/unit_tests/openvpn/test_argv.c @@ -118,6 +118,28 @@ argv_printf__empty_parameter__argc_correct(void **state) } 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) { struct argv a = argv_new(); @@ -233,6 +255,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),