From patchwork Tue Jan 2 13:46:39 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "d12fk (Code Review)" X-Patchwork-Id: 3545 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7301:2791:b0:100:d2e5:60d with SMTP id hm17csp2004921dyb; Tue, 2 Jan 2024 05:47:18 -0800 (PST) X-Google-Smtp-Source: AGHT+IGlkqvoBpEe5NqtpPh9Us3WspTLw1mrBwIpBW+8RaveqoPmvDp9+8tYJr9+B6uhfyInOOo1 X-Received: by 2002:a05:6a20:a41b:b0:18f:cc5f:ffe with SMTP id z27-20020a056a20a41b00b0018fcc5f0ffemr29809796pzk.4.1704203238726; Tue, 02 Jan 2024 05:47:18 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704203238; cv=none; d=google.com; s=arc-20160816; b=A6JwzlLhbQpJ2ATvKTJZ3zIbRK9lYfgj+KE8Wh3y3nkSsMoBf5euLG19lCMg+STcHK ukJ79f/9n/VxFNmXx/2bMI7SwAdsMAYWW5OM4APD1/XPf1g+ypspLdUFEFkwoW9zhYzI RKjuDUpB/2DCR3m8AXjlDkga8wFwUy+ryzpqZBECpTHXSvWQ3BuMp6G41z8F+5bm0IR7 8KxzOYteuySqShsxFjFP8uOUOf2TzSMjKiU6eOf8HwEyNIjPwURlEkIoDbvBW9oFbela DK2HnTXW4axiu8aYA86fItXBU4M8Qe0iWsnWxLFrErCzdVqy11Y+vJzopf2h1l3gwWb1 H1GA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=errors-to:cc:reply-to:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence:subject:user-agent :mime-version:message-id:references:auto-submitted:to:date:from :dkim-signature:dkim-signature:dkim-signature; bh=FiQMAT+2JTRj5lyg7G90hY63D4PsA2H/g0BunU5g71M=; fh=lm0MLPW7DntlrDqRECIiC9JlE1uPxhepE0URYHIf+eE=; b=X7SpMtMw6eJePRiptkTb1KJJj29d0cZZd134bipnBPQr7MU5ZQTUjZ1C89JoNRfuxR mbbjw6anHiU1vWrjQbf9WGMLj542dqKlhYZ7L7+li1w0b5T5eotcuJcZyMX2X8zeeeZc ivA2P4HvhhXhnAtfJ3Zj53gRPM7DhPh6UhvGVUrHm1nbTdmJDbumUqciZbHxjuTnu1Rz sQ+6PXMd9LlpH4fn4jCkpiiENgMTgHRQyPTot8AUNJIFIMxu1eViDwier9OaVSYWn0Ij TB15P/FCwB2Jb8KW4L9As4oUAwXhcQ0MhNJ6UvU81CTnEXTR8fLg/uEou8RGvKsedD5P IrTg== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=M4nWcq6d; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=GaCNEwGF; dkim=neutral (body hash did not verify) header.i=@openvpn.net header.s=google header.b=FqSa4pcT; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=openvpn.net Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id d4-20020a056a00198400b006d0e1cc26a8si19783999pfl.197.2024.01.02.05.47.18 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Jan 2024 05:47:18 -0800 (PST) Received-SPF: pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) client-ip=216.105.38.7; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=M4nWcq6d; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=GaCNEwGF; dkim=neutral (body hash did not verify) header.i=@openvpn.net header.s=google header.b=FqSa4pcT; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=openvpn.net Received: from [127.0.0.1] (helo=sfs-ml-2.v29.lw.sourceforge.com) by sfs-ml-2.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1rKf6c-0000Ly-Ne; Tue, 02 Jan 2024 13:46:55 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-2.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1rKf6a-0000Ls-W0 for openvpn-devel@lists.sourceforge.net; Tue, 02 Jan 2024 13:46:53 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Type:Content-Transfer-Encoding:MIME-Version :Message-ID:Reply-To:References:Subject:List-Unsubscribe:List-Id:Cc:To:Date: From:Sender:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:List-Help: List-Subscribe:List-Post:List-Owner:List-Archive; bh=ONVpOrZLbEvEveMVpeB6+q+LLuLjjmTYQXUSkAxHHs4=; b=M4nWcq6dewlEf+xXXjzBYcNdgE KN2QUT3bc+1NdyplH8SU4e9BSSUq1X2q3d2kppI8dlRuAw7VDuRc6fDZP77EUEG1OCAGmEs+tjiNq mgWDgI3823oMP//jSEo+HUQ6e5nW6oCmi+0dIynyzcsx/PqDadk9Sq0bScjAhHsHGioo=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Type:Content-Transfer-Encoding:MIME-Version:Message-ID:Reply-To: References:Subject:List-Unsubscribe:List-Id:Cc:To:Date:From:Sender:Content-ID :Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To: Resent-Cc:Resent-Message-ID:In-Reply-To:List-Help:List-Subscribe:List-Post: List-Owner:List-Archive; bh=ONVpOrZLbEvEveMVpeB6+q+LLuLjjmTYQXUSkAxHHs4=; b=G aCNEwGFtF8uS5J/1opIXvfpJjJtn6mxaV6Rkuy0L+PuBweckI5Xk+bzpC2t9ZkIb5HZZqvRZkgLKl bAYgc3jAt8xlZEzM+NizQGycs3oFDLytxHrB0U6RbVk+RfzU2LPzAprYfioLmMKVH6cTbJDb43aaj nmsME1+M6BQPV4Ic=; Received: from mail-wr1-f41.google.com ([209.85.221.41]) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.95) id 1rKf6a-0001la-5p for openvpn-devel@lists.sourceforge.net; Tue, 02 Jan 2024 13:46:53 +0000 Received: by mail-wr1-f41.google.com with SMTP id ffacd0b85a97d-3374117c79eso1308394f8f.0 for ; Tue, 02 Jan 2024 05:46:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=openvpn.net; s=google; t=1704203200; x=1704808000; darn=lists.sourceforge.net; h=user-agent:content-disposition:content-transfer-encoding :mime-version:message-id:reply-to:references:subject :list-unsubscribe:list-id:auto-submitted:cc:to:date:from:from:to:cc :subject:date:message-id:reply-to; bh=ONVpOrZLbEvEveMVpeB6+q+LLuLjjmTYQXUSkAxHHs4=; b=FqSa4pcTbYLPfISDV886DJnG1Byx06E1uok5rDu74VMKXI7Ydyg6lsQ3WDdFZs8AKl BxJCDWKM/eg7W6sLlTXwwcwLuVPxp2RBDUFX8aCA8Q81P7jviDubygsjvhkyUqQrBw22 0tG0Wz2igx9E7P1yRFYHcrSsfU1e41ZWQVwR1oAy8r+XweRKU18JRiG0nC9QT+/1BHg8 618RIFVnX6kMXxFi6J09Y2IFNFh12HYVEHvkSjoZMf602nCccq5Vx8utepL/Kd0hzajk AbI/dHYkRA8YM9nS+wIGNMYEM0AHa4hRyHPmUuHBfv84HJRTXl3FHBR5WydgzG40MxH9 aGxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704203200; x=1704808000; h=user-agent:content-disposition:content-transfer-encoding :mime-version:message-id:reply-to:references:subject :list-unsubscribe:list-id:auto-submitted:cc:to:date:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ONVpOrZLbEvEveMVpeB6+q+LLuLjjmTYQXUSkAxHHs4=; b=BURxxRspxOn7suSMCBumfNbmQDqNliGbfnraWNCjgOZDoRVB+mrQ4oTGhElrosVIls V3Trf2JAZrcu9C8hom7RDHKwvuvpLC8I7rfYP8t1RAxh0i3Dg8yyap7zXRSlrAb9Rw/E S2829nyHapIBVt2HNYh7nd05SMkekLC494XCWWR13qgTftIS2nhN3/0lapyDq02qHlrs mMa/5uuzYx6+K+fZXUwncmISNkFe2GNsbIp1FafhjFEH6k/Umq4ABDam+VITnjCydlEI 3UyD4FqR0TOqMblj9YHi3JzgwC6otB7ws1VUNnhRC/u53vEhX3VaTUzNxs85gy/yf8gY UPNg== X-Gm-Message-State: AOJu0YzNhjWg9dE80ZQ1vCyKQpb3C/GZLU0LqPAn1drplMq0+IajvPIK PjihHjga1Tpj8YBbQThFVUSzXmd2UN5f1U5VabAos61r6JE= X-Received: by 2002:a05:6000:1c1:b0:334:ada5:3835 with SMTP id t1-20020a05600001c100b00334ada53835mr9555368wrx.4.1704203200442; Tue, 02 Jan 2024 05:46:40 -0800 (PST) Received: from gerrit.openvpn.in (ec2-18-159-0-78.eu-central-1.compute.amazonaws.com. [18.159.0.78]) by smtp.gmail.com with ESMTPSA id c9-20020a056000104900b003368849129dsm27757911wrx.15.2024.01.02.05.46.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jan 2024 05:46:39 -0800 (PST) From: "plaisthos (Code Review)" X-Google-Original-From: "plaisthos (Code Review)" X-Gerrit-PatchSet: 1 Date: Tue, 2 Jan 2024 13:46:39 +0000 To: flichtenheld Auto-Submitted: auto-generated X-Gerrit-MessageType: newchange X-Gerrit-Change-Id: I6899c1af65fbd670d434cdb17f81c82b900bfbd4 X-Gerrit-Change-Number: 486 X-Gerrit-Project: openvpn X-Gerrit-ChangeURL: X-Gerrit-Commit: 765ac46c4f5702822aeb3a23e9b623325abafa4e References: Message-ID: <238dcd1f13f134079d8b0bc6c5c4dc1cc696818b-HTML@gerrit.openvpn.net> MIME-Version: 1.0 User-Agent: Gerrit/3.8.2 X-Spam-Score: -0.2 (/) X-Spam-Report: Spam detection software, running on the system "util-spamd-1.v13.lw.sourceforge.com", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: Attention is currently required from: flichtenheld. Hello flichtenheld, I'd like you to do a code review. Please visit Content analysis details: (-0.2 points, 6.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [209.85.221.41 listed in wl.mailspike.net] -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [209.85.221.41 listed in list.dnswl.org] 0.0 WEIRD_PORT URI: Uses non-standard port number for HTTP 0.0 HTML_MESSAGE BODY: HTML included in message -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_EF Message has a valid DKIM or DK signature from envelope-from domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.0 T_SCC_BODY_TEXT_LINE No description available. 0.0 T_KAM_HTML_FONT_INVALID Test for Invalidly Named or Formatted Colors in HTML X-Headers-End: 1rKf6a-0001la-5p Subject: [Openvpn-devel] [M] Change in openvpn[master]: Allow having an extra function that is called when an env is freed 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: , Reply-To: arne-openvpn@rfc2549.org, openvpn-devel@lists.sourceforge.net, frank@lichtenheld.com Cc: openvpn-devel Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox X-GMAIL-THRID: =?utf-8?q?1786986615048500606?= X-GMAIL-MSGID: =?utf-8?q?1786986615048500606?= X-getmail-filter-classifier: gerrit message type newchange Attention is currently required from: flichtenheld. Hello flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/486?usp=email to review the following change. Change subject: Allow having an extra function that is called when an env is freed ...................................................................... Allow having an extra function that is called when an env is freed Change-Id: I6899c1af65fbd670d434cdb17f81c82b900bfbd4 Signed-off-by: Arne Schwabe --- M CMakeLists.txt M src/openvpn/env_set.c M src/openvpn/env_set.h M tests/unit_tests/openvpn/Makefile.am M tests/unit_tests/openvpn/test_buffer.c 5 files changed, 106 insertions(+), 7 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/86/486/1 diff --git a/CMakeLists.txt b/CMakeLists.txt index 96328a1..04e9e34 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -685,6 +685,7 @@ target_sources(test_buffer PRIVATE tests/unit_tests/openvpn/mock_get_random.c + src/openvpn/env_set.c ) target_sources(test_crypto PRIVATE diff --git a/src/openvpn/env_set.c b/src/openvpn/env_set.c index 97b011f..fb7cd98 100644 --- a/src/openvpn/env_set.c +++ b/src/openvpn/env_set.c @@ -112,6 +112,10 @@ } if (do_free) { + if (current->free_function) + { + current->free_function(current); + } secure_memzero(current->string, strlen(current->string)); free(current->string); free(current); @@ -124,15 +128,37 @@ } static void -add_env_item(char *str, const bool do_alloc, struct env_item **list, struct gc_arena *gc) +env_gc_freefunction_wrapper(void *arg) +{ + struct env_item *ei = arg; + ei->free_function(ei); + free(ei->string); + free(ei); +} + +static void +add_env_item(char *str, void (*free_function)(struct env_item *), + struct env_item **list, struct gc_arena *gc) { struct env_item *item; ASSERT(str); ASSERT(list); - ALLOC_OBJ_GC(item, struct env_item, gc); - item->string = do_alloc ? string_alloc(str, gc) : str; + if (gc && free_function) + { + /* alloc items manually without gc to allow the gc_wrap_free + * function to free them all in the right order */ + ALLOC_OBJ(item, struct env_item); + item->string = string_alloc(str, NULL); + gc_addspecial(item, env_gc_freefunction_wrapper, gc); + } + else + { + ALLOC_OBJ_GC(item, struct env_item, gc); + item->string = string_alloc(str, gc); + } + item->free_function = free_function; item->next = *list; *list = item; } @@ -146,10 +172,11 @@ } static void -env_set_add_nolock(struct env_set *es, const char *str) +env_set_add_nolock(struct env_set *es, const char *str, + void (*free_function)(struct env_item *)) { remove_env_item(str, es->gc == NULL, &es->list); - add_env_item((char *)str, true, &es->list, es->gc); + add_env_item((char *)str, free_function, &es->list, es->gc); } struct env_set * @@ -171,6 +198,10 @@ while (e) { struct env_item *next = e->next; + if (e->free_function) + { + e->free_function(e); + } free(e->string); free(e); e = next; @@ -194,7 +225,16 @@ { ASSERT(es); ASSERT(str); - env_set_add_nolock(es, str); + env_set_add_nolock(es, str, NULL); +} + +void +env_set_add_specialfree(struct env_set *es, const char *str, + void (*free_function)(struct env_item *)) +{ + ASSERT(es); + ASSERT(str); + env_set_add_nolock(es, str, free_function); } const char * @@ -246,7 +286,7 @@ e = src->list; while (e) { - env_set_add_nolock(es, e->string); + env_set_add_nolock(es, e->string, e->free_function); e = e->next; } } diff --git a/src/openvpn/env_set.h b/src/openvpn/env_set.h index 4599977..27bc4dd 100644 --- a/src/openvpn/env_set.h +++ b/src/openvpn/env_set.h @@ -36,6 +36,7 @@ struct env_item { char *string; + void (*free_function)(struct env_item *); struct env_item *next; }; @@ -87,6 +88,11 @@ void env_set_add(struct env_set *es, const char *str); +void +env_set_add_specialfree(struct env_set *es, const char *str, + void (*free_function)(struct env_item *)); + + const char *env_set_get(const struct env_set *es, const char *name); void env_set_print(int msglevel, const struct env_set *es); diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index 6667626..c5098a9 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -52,6 +52,7 @@ buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(top_srcdir)/src/openvpn -Wl,--wrap=parse_line buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \ mock_get_random.c \ + $(top_srcdir)/src/openvpn/env_set.c \ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/platform.c diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c index ee84c1f..e355a43 100644 --- a/tests/unit_tests/openvpn/test_buffer.c +++ b/tests/unit_tests/openvpn/test_buffer.c @@ -26,13 +26,22 @@ #endif #include "syshead.h" +#include #include #include #include "buffer.h" +#include "env_set.h" #include "buffer.c" +/* mock this function as it is required by env.c */ +int +script_security(void) +{ + return 0; +} + static void test_buffer_strprefix(void **state) { @@ -353,6 +362,47 @@ assert_string_equal(buf, "There is a .'nice.' \"1234\" [.] year old .tree!"); } +static bool unit_test_free_called = false; + +static void +unit_test_special_free(struct env_item *ei) +{ + assert_string_equal(ei->string, "unit=test"); + unit_test_free_called = true; +} + +static void +test_env_special_free(void **state) +{ + /* Test called via env_set_destroy */ + struct env_set *es = env_set_create(NULL); + env_set_add_specialfree(es, "unit=test", &unit_test_special_free); + env_set_destroy(es); + assert_true(unit_test_free_called); + unit_test_free_called = false; + + /* Test called via env_remove */ + es = env_set_create(NULL); + env_set_add_specialfree(es, "unit=test", &unit_test_special_free); + env_set_del(es, "unit"); + assert_true(unit_test_free_called); + unit_test_free_called = false; + env_set_destroy(es); + + /* Test for env with GC */ + struct gc_arena gc = gc_new(); + es = env_set_create(&gc); + env_set_add_specialfree(es, "unit=test", &unit_test_special_free); + env_set_destroy(es); + + /* The free should be done as part of gc free and not as part of + * the env set destroy */ + assert_false(unit_test_free_called); + + gc_free(&gc); + assert_true(unit_test_free_called); +} + int main(void) { @@ -385,6 +435,7 @@ cmocka_unit_test(test_buffer_free_gc_two), cmocka_unit_test(test_buffer_gc_realloc), cmocka_unit_test(test_character_class), + cmocka_unit_test(test_env_special_free) }; return cmocka_run_group_tests_name("buffer", tests, NULL, NULL);