[PATCH v2 00/10] multipath-tools: valgrind tests & fixes

[PATCH v2 00/10] multipath-tools: valgrind tests & fixes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe and Ben,

this series  adds a make target to run unit tests under valgrind, and fixes
the issues I found using this test target.

It contains the patches I sent yesterday ([PATCH 1/5] multipath-tools tests:
fix memory leak in alias test), but in a different order / numbering,
which seems more appropriate now.

Regards,
Martin

Martin Wilck (10):
  multipath-tools: Makefile.inc: fix compilation with gcc 4.x
  multipath-tools: Makefile: add "valgrind-test" target
  multipath-tools tests: fix memory leak in alias test
  multipath-tools tests: fix memory leak in hwtable test
  multipath-tools tests: fix memory leak in vpd test
  libmultipath: fix memory leak in ble handlers
  libmultipath: fix enable_foreign memory leak
  libmultipath: fix invalid memory access in is_token()
  libmultipath: alloc_strvec: NULL-initialize strvec elements
  libmultipath: validate_config_strvec(): avoid out-of-bounds access

 Makefile              |  3 +++
 Makefile.inc          |  2 +-
 libmultipath/config.c |  2 ++
 libmultipath/dict.c   | 12 +++++++++---
 libmultipath/parser.c | 17 +++++++++++------
 multipath/main.c      |  2 +-
 tests/Makefile        | 13 +++++++++++--
 tests/README.md       |  8 ++++++++
 tests/alias.c         |  2 ++
 tests/hwtable.c       |  4 ++++
 tests/vpd.c           | 18 ++++++++----------
 11 files changed, 60 insertions(+), 23 deletions(-)

-- 
2.28.0

[PATCH v2 01/10] multipath-tools: Makefile.inc: fix compilation with gcc 4.x

[mwilck]

From: Martin Wilck <mwilck@suse.com>

GCC 4.x doesn't enable the "gnu99" standard by default, which is
required by the multipath-tools code. Tested with gcc 4, 7, 8,
9, 10 and clang 3.9, 7, 8, 9, 10.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.inc b/Makefile.inc
index 8ea3352..e05f3a9 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -98,7 +98,7 @@ WARNFLAGS	:= -Werror -Wall -Wextra -Wformat=2 -Werror=implicit-int \
 		  -Werror=implicit-function-declaration -Werror=format-security \
 		  $(WNOCLOBBERED) -Werror=cast-qual $(ERROR_DISCARDED_QUALIFIERS)
 CPPFLAGS	:= -Wp,-D_FORTIFY_SOURCE=2 
-CFLAGS		:= $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe \
+CFLAGS		:= --std=gnu99 $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe \
 		   -DBIN_DIR=\"$(bindir)\" -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" \
 		   -MMD -MP
 BIN_CFLAGS	= -fPIE -DPIE
-- 
2.28.0

[PATCH v2 02/10] multipath-tools: Makefile: add "valgrind-test" target

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The new target "valgrind-test" (or "valgrind" in the tests/
subdirectory) allows running the unit tests under valgrind.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile        |  3 +++
 tests/Makefile  | 13 +++++++++++--
 tests/README.md |  8 ++++++++
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 8bcaba6..4a3491d 100644
--- a/Makefile
+++ b/Makefile
@@ -52,6 +52,9 @@ uninstall: $(BUILDDIRS:=.uninstall)
 test:	all
 	$(MAKE) -C tests
 
+valgrind-test:	all
+	$(MAKE) -C tests valgrind
+
 .PHONY:	TAGS
 TAGS:
 	etags -a libmultipath/*.c
diff --git a/tests/Makefile b/tests/Makefile
index 5f00a3a..502377f 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -19,6 +19,7 @@ TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy \
 .PRECIOUS: $(TESTS:%=%-test)
 
 all:	$(TESTS:%=%.out)
+valgrind:	$(TESTS:%=%.vgr)
 
 # test-specific compiler flags
 # XYZ-test_FLAGS: Additional compiler flags for this test
@@ -68,12 +69,20 @@ lib/libchecktur.so:
 	@echo == running $< ==
 	@LD_LIBRARY_PATH=$(multipathdir):$(mpathcmddir) ./$< >$@
 
+%.vgr:  %-test lib/libchecktur.so
+	@echo == running valgrind for $< ==
+	@LD_LIBRARY_PATH=$(multipathdir):$(mpathcmddir) \
+		valgrind --leak-check=full --error-exitcode=128 ./$< >$@ 2>&1
+
 OBJS = $(TESTS:%=%.o) test-lib.o
 
 test_clean:
-	$(RM) $(TESTS:%=%.out)
+	$(RM) $(TESTS:%=%.out) $(TESTS:%=%.vgr)
+
+valgrind_clean:
+	$(RM) $(TESTS:%=%.vgr)
 
-clean: test_clean dep_clean
+clean: test_clean valgrind_clean dep_clean
 	$(RM) $(TESTS:%=%-test) $(OBJS) *.o.wrap
 	$(RM) -rf lib
 
diff --git a/tests/README.md b/tests/README.md
index 6438a82..6e7ad40 100644
--- a/tests/README.md
+++ b/tests/README.md
@@ -5,6 +5,14 @@ or simply `make` in the `tests` subdirectory. The test output is saved as
 `<testname>.out`. The test programs are called `<testname>-test`, and can
 be run standalone e.g. for debugging purposes.
 
+## Running tests under valgrind
+
+The unit tests can be run under the valgrind debugger with `make valgrind`
+in the `tests` directory, or `make valgrind-test` in the top directory.
+If valgrind detects a bad memory access or leak, the test will fail. The
+output of the test run, including valgrind output, is stored as
+`<testname>.vgr`.
+
 ## Notes on individual tests
 
 ### Tests that require root permissions
-- 
2.28.0

[PATCH v2 03/10] multipath-tools tests: fix memory leak in alias test

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/alias.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/alias.c b/tests/alias.c
index 22ffd55..5624138 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -652,6 +652,7 @@ static void al_a(void **state)
 	alias = allocate_binding(0, "WWIDa", 1, "MPATH");
 	assert_ptr_not_equal(alias, NULL);
 	assert_string_equal(alias, "MPATHa");
+	free(alias);
 }
 
 static void al_zz(void **state)
@@ -668,6 +669,7 @@ static void al_zz(void **state)
 	alias = allocate_binding(0, "WWIDzz", 26*26 + 26, "MPATH");
 	assert_ptr_not_equal(alias, NULL);
 	assert_string_equal(alias, "MPATHzz");
+	free(alias);
 }
 
 static void al_0(void **state)
-- 
2.28.0

[PATCH v2 04/10] multipath-tools tests: fix memory leak in hwtable test

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/hwtable.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/hwtable.c b/tests/hwtable.c
index 84d2b35..12660da 100644
--- a/tests/hwtable.c
+++ b/tests/hwtable.c
@@ -25,6 +25,7 @@
 #include "test-lib.h"
 #include "print.h"
 #include "util.h"
+#include "foreign.h"
 
 #define N_CONF_FILES 2
 
@@ -187,6 +188,9 @@ static int teardown(void **state)
 
 	free_hwt(*state);
 	*state = NULL;
+	cleanup_prio();
+	cleanup_checkers();
+	cleanup_foreign();
 
 	return 0;
 }
-- 
2.28.0

[PATCH v2 05/10] multipath-tools tests: fix memory leak in vpd test

[mwilck]

From: Martin Wilck <mwilck@suse.com>

regfree() wasn't called on the re used in subst_spaces().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/vpd.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/tests/vpd.c b/tests/vpd.c
index 3cbad81..e2ec65e 100644
--- a/tests/vpd.c
+++ b/tests/vpd.c
@@ -28,13 +28,17 @@ struct vpdtest {
 	char wwid[WWID_SIZE];
 };
 
+static regex_t space_re;
 static int setup(void **state)
 {
 	struct vpdtest *vt = malloc(sizeof(*vt));
+	int rc;
 
 	if (vt == NULL)
 		return -1;
 	*state = vt;
+	rc = regcomp(&space_re, " +", REG_EXTENDED);
+	assert_int_equal(rc, 0);
 	return 0;
 }
 
@@ -44,6 +48,7 @@ static int teardown(void **state)
 
 	free(vt);
 	*state = NULL;
+	regfree(&space_re);
 	return 0;
 }
 
@@ -360,21 +365,14 @@ static char *subst_spaces(const char *src)
 {
 	char *dst = calloc(1, strlen(src) + 1);
 	char *p;
-	static regex_t *re;
 	regmatch_t match;
-	int rc;
+	int rc = 0;
 
 	assert_non_null(dst);
-	if (re == NULL) {
-		re = calloc(1, sizeof(*re));
-		assert_non_null(re);
-		rc = regcomp(re, " +", REG_EXTENDED);
-		assert_int_equal(rc, 0);
-	}
 
-	for (rc = regexec(re, src, 1, &match, 0), p = dst;
+	for (rc = regexec(&space_re, src, 1, &match, 0), p = dst;
 	    rc == 0;
-	    src += match.rm_eo, rc = regexec(re, src, 1, &match, 0)) {
+	    src += match.rm_eo, rc = regexec(&space_re, src, 1, &match, 0)) {
 		memcpy(p, src, match.rm_so);
 		p += match.rm_so;
 		*p = '_';
-- 
2.28.0

[PATCH v2 06/10] libmultipath: fix memory leak in ble handlers

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Since patch "libmultipath fix a memory leak in set_ble_device",
strings are strdup'd in set_ble_device() and store_ble(). The
passed string must therefore be freed in the handlers in dict.c.

Fixes: ("libmultipath fix a memory leak in set_ble_device")
Cc: lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/dict.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index be3029c..feabae5 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -1499,7 +1499,8 @@ blacklist_exceptions_handler(struct config *conf, vector strvec)
 static int								\
 ble_ ## option ## _handler (struct config *conf, vector strvec)		\
 {									\
-	char * buff;							\
+	char *buff;							\
+	int rc;								\
 									\
 	if (!conf->option)						\
 		return 1;						\
@@ -1508,7 +1509,9 @@ ble_ ## option ## _handler (struct config *conf, vector strvec)		\
 	if (!buff)							\
 		return 1;						\
 									\
-	return store_ble(conf->option, buff, ORIGIN_CONFIG);		\
+	rc = store_ble(conf->option, buff, ORIGIN_CONFIG);		\
+	free(buff);							\
+	return rc;							\
 }
 
 #define declare_ble_device_handler(name, option, vend, prod)		\
@@ -1516,6 +1519,7 @@ static int								\
 ble_ ## option ## _ ## name ## _handler (struct config *conf, vector strvec) \
 {									\
 	char * buff;							\
+	int rc;								\
 									\
 	if (!conf->option)						\
 		return 1;						\
@@ -1524,7 +1528,9 @@ ble_ ## option ## _ ## name ## _handler (struct config *conf, vector strvec) \
 	if (!buff)							\
 		return 1;						\
 									\
-	return set_ble_device(conf->option, vend, prod, ORIGIN_CONFIG);	\
+	rc = set_ble_device(conf->option, vend, prod, ORIGIN_CONFIG);	\
+	free(buff);							\
+	return rc;							\
 }
 
 declare_ble_handler(blist_devnode)
-- 
2.28.0

[PATCH v2 07/10] libmultipath: fix enable_foreign memory leak

[mwilck]

From: Martin Wilck <mwilck@suse.com>

enable_foreign wasn't freed in free_config(). Do it, and make
sure it's always a malloc'd string.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c | 2 ++
 multipath/main.c      | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 86b6733..b9bdbdb 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -631,6 +631,8 @@ free_config (struct config * conf)
 
 	if (conf->config_dir)
 		FREE(conf->config_dir);
+	if (conf->enable_foreign)
+		FREE(conf->enable_foreign);
 
 	free_blacklist(conf->blist_devnode);
 	free_blacklist(conf->blist_wwid);
diff --git a/multipath/main.c b/multipath/main.c
index 80bc4b5..004bce5 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -997,7 +997,7 @@ main (int argc, char *argv[])
 	}
 
 	if ((cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) && enable_foreign)
-		conf->enable_foreign = "";
+		conf->enable_foreign = strdup("");
 
 	/* Failing here is non-fatal */
 	init_foreign(conf->multipath_dir, conf->enable_foreign);
-- 
2.28.0

[PATCH v2 08/10] libmultipath: fix invalid memory access in is_token()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

memcmp() must always be passed memory areas that are valid for the
full length given by the size argument.

See e.g. https://trust-in-soft.com/blog/2015/12/21/memcmp-requires-pointers-to-fully-valid-buffers/

Fixes: 7d95fb6 ("libmultipath: config parser: fix corner case for double quotes")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/parser.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 4f65ba1..3875174 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -194,7 +194,9 @@ snprint_keyword(char *buff, int len, char *fmt, struct keyword *kw,
 static const char quote_marker[] = { '\0', '"', '\0' };
 bool is_quote(const char* token)
 {
-	return !memcmp(token, quote_marker, sizeof(quote_marker));
+	return token[0] == quote_marker[0] &&
+		token[1] == quote_marker[1] &&
+		token[2] == quote_marker[2];
 }
 
 vector
-- 
2.28.0

[PATCH v2 09/10] libmultipath: alloc_strvec: NULL-initialize strvec elements

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The statement assigning a real token to a strvec element may
never be reached. Thus set the element to NULL beforehand to
make sure later code can recognized the non-properly initialized
element.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/parser.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 3875174..e6753c4 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -237,6 +237,7 @@ alloc_strvec(char *string)
 		if (!vector_alloc_slot(strvec))
 			goto out;
 
+		vector_set_slot(strvec, NULL);
 		start = cp;
 		if (*cp == '"' && !(in_string && *(cp + 1) == '"')) {
 			cp++;
-- 
2.28.0

[PATCH v2 10/10] libmultipath: validate_config_strvec(): avoid out-of-bounds access

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Always check the length of strvec before accessing elements.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/parser.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index e6753c4..ed6d5d6 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -436,14 +436,16 @@ is_sublevel_keyword(char *str)
 int
 validate_config_strvec(vector strvec, char *file)
 {
-	char *str;
+	char *str = NULL;
 	int i;
 
-	str = VECTOR_SLOT(strvec, 0);
+	if (strvec && VECTOR_SIZE(strvec) > 0)
+		str = VECTOR_SLOT(strvec, 0);
+
 	if (str == NULL) {
 		condlog(0, "can't parse option on line %d of %s",
 			line_nr, file);
-	return -1;
+		return -1;
 	}
 	if (*str == '}') {
 		if (VECTOR_SIZE(strvec) > 1)
@@ -456,7 +458,7 @@ validate_config_strvec(vector strvec, char *file)
 		return -1;
 	}
 	if (is_sublevel_keyword(str)) {
-		str = VECTOR_SLOT(strvec, 1);
+		str = VECTOR_SIZE(strvec) > 1 ? VECTOR_SLOT(strvec, 1) : NULL;
 		if (str == NULL)
 			condlog(0, "missing '{' on line %d of %s",
 				line_nr, file);
@@ -467,7 +469,7 @@ validate_config_strvec(vector strvec, char *file)
 			condlog(0, "ignoring extra data starting with '%s' on line %d of %s", (char *)VECTOR_SLOT(strvec, 2), line_nr, file);
 		return 0;
 	}
-	str = VECTOR_SLOT(strvec, 1);
+	str = VECTOR_SIZE(strvec) > 1 ? VECTOR_SLOT(strvec, 1) : NULL;
 	if (str == NULL) {
 		condlog(0, "missing value for option '%s' on line %d of %s",
 			(char *)VECTOR_SLOT(strvec, 0), line_nr, file);
-- 
2.28.0

Re: [PATCH v2 00/10] multipath-tools: valgrind tests & fixes

[Benjamin Marzinski]

On Wed, Aug 26, 2020 at 11:58:57AM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hi Christophe and Ben,
> 
> this series  adds a make target to run unit tests under valgrind, and fixes
> the issues I found using this test target.
> 
> It contains the patches I sent yesterday ([PATCH 1/5] multipath-tools tests:
> fix memory leak in alias test), but in a different order / numbering,
> which seems more appropriate now.
> 

For the set
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Regards,
> Martin
> 
> Martin Wilck (10):
>   multipath-tools: Makefile.inc: fix compilation with gcc 4.x
>   multipath-tools: Makefile: add "valgrind-test" target
>   multipath-tools tests: fix memory leak in alias test
>   multipath-tools tests: fix memory leak in hwtable test
>   multipath-tools tests: fix memory leak in vpd test
>   libmultipath: fix memory leak in ble handlers
>   libmultipath: fix enable_foreign memory leak
>   libmultipath: fix invalid memory access in is_token()
>   libmultipath: alloc_strvec: NULL-initialize strvec elements
>   libmultipath: validate_config_strvec(): avoid out-of-bounds access
> 
>  Makefile              |  3 +++
>  Makefile.inc          |  2 +-
>  libmultipath/config.c |  2 ++
>  libmultipath/dict.c   | 12 +++++++++---
>  libmultipath/parser.c | 17 +++++++++++------
>  multipath/main.c      |  2 +-
>  tests/Makefile        | 13 +++++++++++--
>  tests/README.md       |  8 ++++++++
>  tests/alias.c         |  2 ++
>  tests/hwtable.c       |  4 ++++
>  tests/vpd.c           | 18 ++++++++----------
>  11 files changed, 60 insertions(+), 23 deletions(-)
> 
> -- 
> 2.28.0