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 <a...@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
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);
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/486?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I6899c1af65fbd670d434cdb17f81c82b900bfbd4
Gerrit-Change-Number: 486
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arne-open...@rfc2549.org>
Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: flichtenheld <fr...@lichtenheld.com>
Gerrit-MessageType: newchange
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
