[Openvpn-devel,M] Change in openvpn[master]: Allow having an extra function that is called when an env is freed

Message ID 238dcd1f13f134079d8b0bc6c5c4dc1cc696818b-HTML@gerrit.openvpn.net
State New
Headers show
Series [Openvpn-devel,M] Change in openvpn[master]: Allow having an extra function that is called when an env is freed | expand

Commit Message

its_Giaan (Code Review) Jan. 2, 2024, 1:46 p.m. UTC
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 <arne@rfc2549.org>
---
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

Patch

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 <stdbool.h>
 
 #include <setjmp.h>
 #include <cmocka.h>
 
 #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);