libmapper: fix logic bug
sarraylen returns the number of strings and does not include the
trailing null pointer. This results in sarraydup creating arrays of
strings where the array is not null pointer terminated (the strings
themselves _are_ \0 terminated), which in turn causes random data to be
passed to free.
The bug was found using static analysis and inspection would indicate
exposure only exists on error and shutdown paths, which might be a hint
as to how this has been lurking for as long as it has.
Change-Id: Ie5e5b6cdfb7832c84037ff039b41082fc7d20b61
Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com>
diff --git a/Makefile.am b/Makefile.am
index f9a608d..5c31219 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -45,5 +45,6 @@
CODE_COVERAGE_IGNORE_PATTERN = '/include/*' '/usr/include/*' '$(includedir)/*'
include src/test/Makefile.am.include
+include libmapper/test/Makefile.am.include
TESTS = $(check_PROGRAMS)
diff --git a/libmapper/internal.h b/libmapper/internal.h
new file mode 100644
index 0000000..e6f9a07
--- /dev/null
+++ b/libmapper/internal.h
@@ -0,0 +1,10 @@
+#pragma once
+#ifdef __cplusplus
+extern "C" {
+#endif
+int sarraylen(char* array[]);
+void sarrayfree(char* array[]);
+char** sarraydup(char* array[]);
+#ifdef __cplusplus
+}
+#endif
diff --git a/libmapper/mapper.c b/libmapper/mapper.c
index fb07940..4a1cd3b 100644
--- a/libmapper/mapper.c
+++ b/libmapper/mapper.c
@@ -27,6 +27,8 @@
#include <systemd/sd-event.h>
#include <unistd.h>
+#include "internal.h"
+
#define _public_ __attribute__((__visibility__("default")))
static const char* async_wait_introspection_match =
@@ -99,7 +101,7 @@
static int async_subtree_getpaths_callback(sd_bus_message*, void*,
sd_bus_error*);
-static int sarraylen(char* array[])
+int sarraylen(char* array[])
{
int count = 0;
char** p = array;
@@ -113,7 +115,7 @@
return count;
}
-static void sarrayfree(char* array[])
+void sarrayfree(char* array[])
{
char** p = array;
while (*p != NULL)
@@ -124,13 +126,13 @@
free(array);
}
-static char** sarraydup(char* array[])
+char** sarraydup(char* array[])
{
int count = sarraylen(array);
int i;
char** ret = NULL;
- ret = malloc(sizeof(*ret) * count);
+ ret = calloc(count + 1, sizeof(*ret));
if (!ret)
return NULL;
diff --git a/libmapper/test/Makefile.am.include b/libmapper/test/Makefile.am.include
new file mode 100644
index 0000000..fdd4849
--- /dev/null
+++ b/libmapper/test/Makefile.am.include
@@ -0,0 +1,5 @@
+libmapper_test_mapper_SOURCES = %reldir%/mapper.cpp \
+ libmapper/mapper.c libmapper/test/utils.c
+
+check_PROGRAMS += \
+ %reldir%/mapper
diff --git a/libmapper/test/mapper.cpp b/libmapper/test/mapper.cpp
new file mode 100644
index 0000000..8831b62
--- /dev/null
+++ b/libmapper/test/mapper.cpp
@@ -0,0 +1,22 @@
+#include "../internal.h"
+#include "utils.h"
+
+#include <gtest/gtest.h>
+
+TEST(TestSarray, Dup)
+{
+ auto a = generate_test_sarray(3);
+
+ auto size = sarraylen(a);
+ EXPECT_EQ(size, 3);
+
+ auto b = sarraydup(a);
+ size_t i;
+
+ for (i = 0; i < 4; i++)
+ {
+ EXPECT_STREQ(a[i], b[i]);
+ }
+ sarrayfree(a);
+ sarrayfree(b);
+}
diff --git a/libmapper/test/utils.c b/libmapper/test/utils.c
new file mode 100644
index 0000000..b3c7ae1
--- /dev/null
+++ b/libmapper/test/utils.c
@@ -0,0 +1,31 @@
+#include "utils.h"
+
+#include <stdlib.h>
+#include <string.h>
+
+char** generate_test_sarray(size_t len)
+{
+ static const char testString[] = "test";
+ size_t i;
+ char** ret = calloc(len + 1, sizeof(*ret));
+ if (!ret)
+ return NULL;
+
+ for (i = 0; i < len; ++i)
+ {
+ ret[i] = strdup(testString);
+ if (!ret[i])
+ goto error;
+ }
+
+ return ret;
+
+error:
+ for (i = 0; i < len; ++i)
+ {
+ free(ret[i]);
+ }
+ free(ret);
+
+ return NULL;
+}
diff --git a/libmapper/test/utils.h b/libmapper/test/utils.h
new file mode 100644
index 0000000..73162cb
--- /dev/null
+++ b/libmapper/test/utils.h
@@ -0,0 +1,9 @@
+#pragma once
+#include <stddef.h>
+#ifdef __cplusplus
+extern "C" {
+#endif
+char** generate_test_sarray(size_t);
+#ifdef __cplusplus
+}
+#endif