All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] libsepol: do not pass NULL to memcpy
@ 2021-10-13 12:53 Christian Göttsche
  2021-10-13 12:53 ` [PATCH 2/3] libsemanage: do not sort empty records Christian Göttsche
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Christian Göttsche @ 2021-10-13 12:53 UTC (permalink / raw)
  To: selinux

For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not
use it as source of a memcpy(3), even with a size of 0.  memcpy(3) might
be annotated with the function attribute nonnull and UBSan then
complains:

    link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null

Use a realloc + memset instead of a calloc and free to increase the size
of `mod->perm_map[sclassi]`.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/link.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/libsepol/src/link.c b/libsepol/src/link.c
index 7512a4d9..75ce2b20 100644
--- a/libsepol/src/link.c
+++ b/libsepol/src/link.c
@@ -185,14 +185,12 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
 	 * may have originated from the class -or- it could be from
 	 * the class's common parent.*/
 	if (perm->s.value > mod->perm_map_len[sclassi]) {
-		uint32_t *newmap = calloc(perm->s.value, sizeof(*newmap));
+		uint32_t *newmap = realloc(mod->perm_map[sclassi], perm->s.value * sizeof(*newmap));
 		if (newmap == NULL) {
 			ERR(state->handle, "Out of memory!");
 			return -1;
 		}
-		memcpy(newmap, mod->perm_map[sclassi],
-		       mod->perm_map_len[sclassi] * sizeof(*newmap));
-		free(mod->perm_map[sclassi]);
+		memset(newmap + mod->perm_map_len[sclassi], '\0', perm->s.value - mod->perm_map_len[sclassi]);
 		mod->perm_map[sclassi] = newmap;
 		mod->perm_map_len[sclassi] = perm->s.value;
 	}
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 2/3] libsemanage: do not sort empty records
  2021-10-13 12:53 [PATCH 1/3] libsepol: do not pass NULL to memcpy Christian Göttsche
@ 2021-10-13 12:53 ` Christian Göttsche
  2021-10-16 19:34   ` Nicolas Iooss
  2021-10-13 12:53 ` [PATCH 3/3] libsemanage/tests: free memory Christian Göttsche
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Christian Göttsche @ 2021-10-13 12:53 UTC (permalink / raw)
  To: selinux

Do not sort empty records to avoid calling qsort(3) with a NULL pointer.
qsort(3) might be annotated with the function attribute nonnull and
UBSan then complains:

    database_join.c:80:2: runtime error: null pointer passed as argument 1, which is declared to never be null

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsemanage/src/database_join.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/libsemanage/src/database_join.c b/libsemanage/src/database_join.c
index b9b35a61..b0e66e53 100644
--- a/libsemanage/src/database_join.c
+++ b/libsemanage/src/database_join.c
@@ -77,10 +77,12 @@ static int dbase_join_cache(semanage_handle_t * handle, dbase_join_t * dbase)
 		goto err;
 
 	/* Sort for quicker merge later */
-	qsort(records1, rcount1, sizeof(record1_t *),
-	      (int (*)(const void *, const void *))rtable1->compare2_qsort);
-	qsort(records2, rcount2, sizeof(record2_t *),
-	      (int (*)(const void *, const void *))rtable2->compare2_qsort);
+	if (rcount1 > 0)
+		qsort(records1, rcount1, sizeof(record1_t *),
+		      (int (*)(const void *, const void *))rtable1->compare2_qsort);
+	if (rcount2 > 0)
+		qsort(records2, rcount2, sizeof(record2_t *),
+		      (int (*)(const void *, const void *))rtable2->compare2_qsort);
 
 	/* Now merge into this dbase */
 	while (i < rcount1 || j < rcount2) {
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH 3/3] libsemanage/tests: free memory
  2021-10-13 12:53 [PATCH 1/3] libsepol: do not pass NULL to memcpy Christian Göttsche
  2021-10-13 12:53 ` [PATCH 2/3] libsemanage: do not sort empty records Christian Göttsche
@ 2021-10-13 12:53 ` Christian Göttsche
  2021-10-16 20:08   ` Nicolas Iooss
  2021-10-16 19:30 ` [PATCH 1/3] libsepol: do not pass NULL to memcpy Nicolas Iooss
  2021-10-19 15:11 ` [PATCH v2 " Christian Göttsche
  3 siblings, 1 reply; 12+ messages in thread
From: Christian Göttsche @ 2021-10-13 12:53 UTC (permalink / raw)
  To: selinux

Free all memory in test cases, reported by LeakSanitizer.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsemanage/tests/test_bool.c      | 33 +++++++++++++++++++++++++++++-
 libsemanage/tests/test_fcontext.c  | 31 +++++++++++++++++++++++++++-
 libsemanage/tests/test_ibendport.c | 13 ++++++++++++
 libsemanage/tests/test_iface.c     | 24 ++++++++++++++++++++++
 libsemanage/tests/test_node.c      | 29 ++++++++++++++++++++++++++
 libsemanage/tests/test_other.c     |  6 ++++++
 libsemanage/tests/test_port.c      | 24 ++++++++++++++++++++++
 libsemanage/tests/test_user.c      | 17 +++++++++++++++
 libsemanage/tests/utilities.c      |  5 ++++-
 libsemanage/tests/utilities.h      |  2 ++
 10 files changed, 181 insertions(+), 3 deletions(-)

diff --git a/libsemanage/tests/test_bool.c b/libsemanage/tests/test_bool.c
index ae80d448..558eab78 100644
--- a/libsemanage/tests/test_bool.c
+++ b/libsemanage/tests/test_bool.c
@@ -132,6 +132,8 @@ semanage_bool_t *get_bool_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_bool_free(records[i]);
 
+	free(records);
+
 	return boolean;
 }
 
@@ -163,6 +165,8 @@ semanage_bool_key_t *get_bool_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_bool_free(boolean);
+
 	return key;
 }
 
@@ -196,6 +200,9 @@ void add_local_bool(const char *name)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(boolean);
 
 	CU_ASSERT_FATAL(semanage_bool_modify_local(sh, key, boolean) >= 0);
+
+	semanage_bool_key_free(key);
+	semanage_bool_free(boolean);
 }
 
 void delete_local_bool(const char *name)
@@ -208,6 +215,8 @@ void delete_local_bool(const char *name)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_bool_del_local(sh, key) >= 0);
+
+	semanage_bool_key_free(key);
 }
 
 /* Function bool_key_create */
@@ -447,6 +456,8 @@ void helper_bool_create(level_t level)
 	CU_ASSERT_PTR_NULL(semanage_bool_get_name(boolean));
 	CU_ASSERT(semanage_bool_get_value(boolean) == 0);
 
+	semanage_bool_free(boolean);
+
 	cleanup_handle(level);
 }
 
@@ -483,6 +494,9 @@ void helper_bool_clone(level_t level, int bool_idx)
 
 	CU_ASSERT_EQUAL(val, val_clone);
 
+	semanage_bool_free(boolean_clone);
+	semanage_bool_free(boolean);
+
 	cleanup_handle(level);
 }
 
@@ -514,6 +528,9 @@ void helper_bool_query(level_t level, const char *bool_str, int exp_res)
 		CU_ASSERT_PTR_NULL(resp);
 	}
 
+	semanage_bool_free(resp);
+	semanage_bool_key_free(key);
+
 	cleanup_handle(level);
 }
 
@@ -647,6 +664,8 @@ void helper_bool_list(level_t level)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_bool_free(records[i]);
 
+	free(records);
+
 	cleanup_handle(level);
 }
 
@@ -662,7 +681,7 @@ void helper_bool_modify_del_local(level_t level, const char *name,
 				  int old_val, int exp_res)
 {
 	semanage_bool_t *boolean;
-	semanage_bool_t *boolean_local;
+	semanage_bool_t *boolean_local = NULL;
 	semanage_bool_key_t *key = NULL;
 	int res;
 	int new_val;
@@ -703,6 +722,7 @@ void helper_bool_modify_del_local(level_t level, const char *name,
 
 	/* cleanup */
 	semanage_bool_key_free(key);
+	semanage_bool_free(boolean_local);
 	semanage_bool_free(boolean);
 
 	cleanup_handle(level);
@@ -734,15 +754,18 @@ void test_bool_query_local(void)
 
 	/* transaction */
 	setup_handle(SH_TRANS);
+	semanage_bool_key_free(key);
 	CU_ASSERT(semanage_bool_key_create(sh, BOOL1_NAME, &key) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(key);
 
 	CU_ASSERT(semanage_bool_query_local(sh, key, &resp) < 0);
 	CU_ASSERT_PTR_NULL(resp);
+	semanage_bool_free(resp);
 
 	add_local_bool(BOOL1_NAME);
 	CU_ASSERT(semanage_bool_query_local(sh, key, &resp) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(resp);
+	semanage_bool_free(resp);
 
 	semanage_bool_key_free(key);
 	CU_ASSERT(semanage_bool_key_create(sh, BOOL2_NAME, &key) >= 0);
@@ -751,8 +774,10 @@ void test_bool_query_local(void)
 	add_local_bool(BOOL2_NAME);
 	CU_ASSERT(semanage_bool_query_local(sh, key, &resp) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(resp);
+	semanage_bool_free(resp);
 
 	/* cleanup */
+	semanage_bool_key_free(key);
 	delete_local_bool(BOOL1_NAME);
 	delete_local_bool(BOOL2_NAME);
 	cleanup_handle(SH_TRANS);
@@ -784,6 +809,7 @@ void test_bool_exists_local(void)
 	CU_ASSERT(resp == 0);
 
 	/* cleanup */
+	semanage_bool_key_free(key);
 	cleanup_handle(SH_TRANS);
 }
 
@@ -918,12 +944,17 @@ void test_bool_list_local(void)
 	CU_ASSERT(semanage_bool_list_local(sh, &records, &count) >= 0);
 	CU_ASSERT(count == init_count + 1);
 	CU_ASSERT_PTR_NOT_NULL(records[0]);
+	semanage_bool_free(records[0]);
+	free(records);
 
 	add_local_bool(BOOL2_NAME);
 	CU_ASSERT(semanage_bool_list_local(sh, &records, &count) >= 0);
 	CU_ASSERT(count == init_count + 2);
 	CU_ASSERT_PTR_NOT_NULL(records[0]);
 	CU_ASSERT_PTR_NOT_NULL(records[1]);
+	semanage_bool_free(records[0]);
+	semanage_bool_free(records[1]);
+	free(records);
 
 	/* cleanup */
 	delete_local_bool(BOOL1_NAME);
diff --git a/libsemanage/tests/test_fcontext.c b/libsemanage/tests/test_fcontext.c
index 62af711f..a012c5dd 100644
--- a/libsemanage/tests/test_fcontext.c
+++ b/libsemanage/tests/test_fcontext.c
@@ -214,6 +214,8 @@ semanage_fcontext_t *get_fcontext_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_fcontext_free(records[i]);
 
+	free(records);
+
 	return fcontext;
 }
 
@@ -230,6 +232,8 @@ semanage_fcontext_key_t *get_fcontext_key_nth(int idx)
 	CU_ASSERT_FATAL(semanage_fcontext_key_extract(sh, fcontext, &key) >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_fcontext_free(fcontext);
+
 	return key;
 }
 
@@ -246,6 +250,10 @@ void add_local_fcontext(int fcontext_idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_fcontext_modify_local(sh, key, fcontext) >= 0);
+
+	/* cleanup */
+	semanage_fcontext_key_free(key);
+	semanage_fcontext_free(fcontext);
 }
 
 void delete_local_fcontext(int fcontext_idx)
@@ -257,6 +265,8 @@ void delete_local_fcontext(int fcontext_idx)
 	key = get_fcontext_key_nth(fcontext_idx);
 
 	CU_ASSERT_FATAL(semanage_fcontext_del_local(sh, key) >= 0);
+
+	semanage_fcontext_key_free(key);
 }
 
 semanage_fcontext_key_t *get_fcontext_key_from_str(const char *str, int type)
@@ -477,6 +487,7 @@ void helper_fcontext_get_set_con(level_t level, int fcontext_idx,
 	}
 
 	/* cleanup */
+	semanage_context_free(con);
 	semanage_fcontext_free(fcontext);
 	cleanup_handle(level);
 }
@@ -587,12 +598,14 @@ void helper_fcontext_query(level_t level, const char *fcontext_expr,
 		CU_ASSERT(res >= 0);
 		const char *expr = semanage_fcontext_get_expr(resp);
 		CU_ASSERT_STRING_EQUAL(expr, fcontext_expr);
+		semanage_fcontext_free(resp);
 	} else {
 		CU_ASSERT(res < 0);
 		CU_ASSERT(resp == (void *) 42);
 	}
 
 	/* cleanup */
+	semanage_fcontext_key_free(key);
 	cleanup_handle(level);
 }
 
@@ -752,6 +765,8 @@ void helper_fcontext_list(level_t level)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_fcontext_free(records[i]);
 
+	free(records);
+
 	/* cleanup */
 	cleanup_handle(level);
 }
@@ -768,7 +783,7 @@ void helper_fcontext_modify_del_local(level_t level, int fcontext_idx,
 				      const char *con_str, int exp_res)
 {
 	semanage_fcontext_t *fcontext;
-	semanage_fcontext_t *fcontext_local;
+	semanage_fcontext_t *fcontext_local = NULL;
 	semanage_fcontext_key_t *key = NULL;
 	semanage_context_t *con = NULL;
 	int res;
@@ -811,7 +826,9 @@ void helper_fcontext_modify_del_local(level_t level, int fcontext_idx,
 	}
 
 	/* cleanup */
+	semanage_context_free(con);
 	semanage_fcontext_key_free(key);
+	semanage_fcontext_free(fcontext_local);
 	semanage_fcontext_free(fcontext);
 	cleanup_handle(level);
 }
@@ -846,6 +863,7 @@ void test_fcontext_query_local(void)
 	/* transaction */
 	setup_handle(SH_TRANS);
 
+	semanage_fcontext_key_free(key);
 	key = get_fcontext_key_nth(I_FIRST);
 	CU_ASSERT(semanage_fcontext_query_local(sh, key, &resp) < 0);
 	CU_ASSERT_PTR_NULL(resp);
@@ -853,14 +871,19 @@ void test_fcontext_query_local(void)
 	add_local_fcontext(I_FIRST);
 	CU_ASSERT(semanage_fcontext_query_local(sh, key, &resp) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(resp);
+	semanage_fcontext_free(resp);
+	resp = NULL;
 
 	semanage_fcontext_key_free(key);
 	key = get_fcontext_key_nth(I_SECOND);
 	add_local_fcontext(I_SECOND);
 	CU_ASSERT(semanage_fcontext_query_local(sh, key, &resp) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(resp);
+	semanage_fcontext_free(resp);
+	resp = NULL;
 
 	/* cleanup */
+	semanage_fcontext_key_free(key);
 	delete_local_fcontext(I_FIRST);
 	delete_local_fcontext(I_SECOND);
 	cleanup_handle(SH_TRANS);
@@ -898,6 +921,7 @@ void test_fcontext_exists_local(void)
 	CU_ASSERT(resp == 0);
 
 	/* cleanup */
+	semanage_fcontext_key_free(key);
 	cleanup_handle(SH_TRANS);
 }
 
@@ -1031,12 +1055,17 @@ void test_fcontext_list_local(void)
 	CU_ASSERT(semanage_fcontext_list_local(sh, &records, &count) >= 0);
 	CU_ASSERT(count == 1);
 	CU_ASSERT_PTR_NOT_NULL(records[0]);
+	semanage_fcontext_free(records[0]);
+	free(records);
 
 	add_local_fcontext(I_SECOND);
 	CU_ASSERT(semanage_fcontext_list_local(sh, &records, &count) >= 0);
 	CU_ASSERT(count == 2);
 	CU_ASSERT_PTR_NOT_NULL(records[0]);
 	CU_ASSERT_PTR_NOT_NULL(records[1]);
+	semanage_fcontext_free(records[0]);
+	semanage_fcontext_free(records[1]);
+	free(records);
 
 	/* cleanup */
 	delete_local_fcontext(I_FIRST);
diff --git a/libsemanage/tests/test_ibendport.c b/libsemanage/tests/test_ibendport.c
index 79a8e2c8..666f8828 100644
--- a/libsemanage/tests/test_ibendport.c
+++ b/libsemanage/tests/test_ibendport.c
@@ -113,6 +113,8 @@ semanage_ibendport_t *get_ibendport_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_ibendport_free(records[i]);
 
+	free(records);
+
 	return ibendport;
 }
 
@@ -132,6 +134,8 @@ semanage_ibendport_key_t *get_ibendport_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_ibendport_free(ibendport);
+
 	return key;
 }
 
@@ -148,6 +152,9 @@ void add_local_ibendport(int idx)
 
 	CU_ASSERT_FATAL(semanage_ibendport_modify_local(sh, key,
 							ibendport) >= 0);
+
+	semanage_ibendport_key_free(key);
+	semanage_ibendport_free(ibendport);
 }
 
 void delete_local_ibendport(int idx)
@@ -155,6 +162,8 @@ void delete_local_ibendport(int idx)
 	semanage_ibendport_key_t *key = NULL;
 	key = get_ibendport_key_nth(idx);
 	CU_ASSERT_FATAL(semanage_ibendport_del_local(sh, key) >= 0);
+
+	semanage_ibendport_key_free(key);
 }
 
 /* Function semanage_ibendport_query */
@@ -195,7 +204,9 @@ void test_ibendport_query(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con_exp);
 
 	/* cleanup */
+	free(name_exp);
 	free(name);
+	semanage_ibendport_key_free(key);
 	semanage_ibendport_free(ibendport);
 	semanage_ibendport_free(ibendport_exp);
 	cleanup_handle(SH_CONNECT);
@@ -362,6 +373,8 @@ void test_ibendport_modify_del_query_local(void)
 						 &ibendport_local) < 0);
 
 	/* cleanup */
+	semanage_ibendport_key_free(key);
+	semanage_ibendport_free(ibendport_local);
 	semanage_ibendport_free(ibendport);
 	cleanup_handle(SH_TRANS);
 }
diff --git a/libsemanage/tests/test_iface.c b/libsemanage/tests/test_iface.c
index d5d530a8..19ac577c 100644
--- a/libsemanage/tests/test_iface.c
+++ b/libsemanage/tests/test_iface.c
@@ -139,6 +139,8 @@ semanage_iface_t *get_iface_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_iface_free(records[i]);
 
+	free(records);
+
 	return iface;
 }
 
@@ -157,6 +159,9 @@ semanage_iface_key_t *get_iface_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	/* cleanup */
+	semanage_iface_free(iface);
+
 	return key;
 }
 
@@ -171,6 +176,10 @@ void add_local_iface(int idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_iface_modify_local(sh, key, iface) >= 0);
+
+	/* cleanup */
+	semanage_iface_key_free(key);
+	semanage_iface_free(iface);
 }
 
 void delete_local_iface(int idx)
@@ -178,6 +187,9 @@ void delete_local_iface(int idx)
 	semanage_iface_key_t *key = NULL;
 	key = get_iface_key_nth(idx);
 	CU_ASSERT_FATAL(semanage_iface_del_local(sh, key) >= 0);
+
+	/* cleanup */
+	semanage_iface_key_free(key);
 }
 
 /* Function semanage_iface_compare */
@@ -309,6 +321,7 @@ void test_iface_get_set_ifcon(void)
 	CU_ASSERT_CONTEXT_EQUAL(con1, con2);
 
 	/* cleanup */
+	semanage_context_free(con1);
 	semanage_iface_free(iface);
 	cleanup_handle(SH_CONNECT);
 }
@@ -332,6 +345,7 @@ void test_iface_get_set_msgcon(void)
 	CU_ASSERT_CONTEXT_EQUAL(con1, con2);
 
 	/* cleanup */
+	semanage_context_free(con1);
 	semanage_iface_free(iface);
 	cleanup_handle(SH_CONNECT);
 }
@@ -357,6 +371,8 @@ void test_iface_create(void)
 	CU_ASSERT(semanage_iface_set_msgcon(sh, iface, msgcon) >= 0);
 
 	/* cleanup */
+	semanage_context_free(msgcon);
+	semanage_context_free(ifcon);
 	semanage_iface_free(iface);
 	cleanup_handle(SH_CONNECT);
 }
@@ -393,6 +409,8 @@ void test_iface_clone(void)
 	CU_ASSERT_CONTEXT_EQUAL(msgcon, msgcon2);
 
 	/* cleanup */
+	semanage_context_free(msgcon);
+	semanage_context_free(ifcon);
 	semanage_iface_free(iface);
 	semanage_iface_free(iface_clone);
 	cleanup_handle(SH_CONNECT);
@@ -426,6 +444,7 @@ void test_iface_query(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con_exp);
 
 	/* cleanup */
+	semanage_iface_key_free(key);
 	semanage_iface_free(iface);
 	semanage_iface_free(iface_exp);
 	cleanup_handle(SH_CONNECT);
@@ -513,6 +532,8 @@ void test_iface_list(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_iface_free(records[i]);
 
+	free(records);
+
 	/* cleanup */
 	cleanup_handle(SH_CONNECT);
 }
@@ -546,6 +567,8 @@ void test_iface_modify_del_query_local(void)
 	CU_ASSERT(semanage_iface_query_local(sh, key, &iface_local) < 0);
 
 	/* cleanup */
+	semanage_iface_key_free(key);
+	semanage_iface_free(iface_local);
 	semanage_iface_free(iface);
 	cleanup_handle(SH_TRANS);
 }
@@ -658,6 +681,7 @@ void test_iface_list_local(void)
 	/* cleanup */
 	for (unsigned int i = 0; i < count; i++)
 		semanage_iface_free(records[i]);
+	free(records);
 
 	delete_local_iface(I_FIRST);
 	delete_local_iface(I_SECOND);
diff --git a/libsemanage/tests/test_node.c b/libsemanage/tests/test_node.c
index 53c2eb69..d715e6a4 100644
--- a/libsemanage/tests/test_node.c
+++ b/libsemanage/tests/test_node.c
@@ -148,6 +148,8 @@ semanage_node_t *get_node_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_node_free(records[i]);
 
+	free(records);
+
 	return node;
 }
 
@@ -167,6 +169,8 @@ semanage_node_key_t *get_node_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_node_free(node);
+
 	return key;
 }
 
@@ -181,6 +185,10 @@ void add_local_node(int idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_node_modify_local(sh, key, node) >= 0);
+
+	/* cleanup */
+	semanage_node_key_free(key);
+	semanage_node_free(node);
 }
 
 void delete_local_node(int idx)
@@ -190,6 +198,9 @@ void delete_local_node(int idx)
 	key = get_node_key_nth(idx);
 
 	CU_ASSERT_FATAL(semanage_node_del_local(sh, key) >= 0);
+
+	/* cleanup */
+	semanage_node_key_free(key);
 }
 
 /* Function semanage_node_compare */
@@ -305,6 +316,7 @@ void test_node_get_set_addr(void)
 	CU_ASSERT_STRING_EQUAL(addr, "192.168.0.1");
 
 	/* cleanup */
+	free(addr);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -334,6 +346,7 @@ void test_node_get_set_addr_bytes(void)
 		CU_ASSERT(addr1[i] == addr2[i]);
 
 	/* cleanup */
+	free(addr2);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -357,6 +370,7 @@ void test_node_get_set_mask(void)
 	CU_ASSERT_STRING_EQUAL(mask, "255.255.255.0");
 
 	/* cleanup */
+	free(mask);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -386,6 +400,7 @@ void test_node_get_set_mask_bytes(void)
 		CU_ASSERT(mask1[i] == mask2[i]);
 
 	/* cleanup */
+	free(mask2);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -436,6 +451,7 @@ void test_node_get_set_con(void)
 	CU_ASSERT_CONTEXT_EQUAL(con1, con2);
 
 	/* cleanup */
+	semanage_context_free(con1);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -461,6 +477,7 @@ void test_node_create(void)
 	CU_ASSERT(semanage_node_set_con(sh, node, con) >= 0);
 
 	/* cleanup */
+	semanage_context_free(con);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -508,6 +525,9 @@ void test_node_clone(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con2);
 
 	/* cleanup */
+	free(mask2);
+	free(addr2);
+	semanage_context_free(con);
 	semanage_node_free(node);
 	semanage_node_free(node_clone);
 	cleanup_handle(SH_CONNECT);
@@ -552,6 +572,8 @@ void test_node_query(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con_exp);
 
 	/* cleanup */
+	semanage_node_key_free(key);
+	semanage_node_free(node_exp);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -638,6 +660,8 @@ void test_node_list(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_node_free(records[i]);
 
+	free(records);
+
 	/* cleanup */
 	cleanup_handle(SH_CONNECT);
 }
@@ -686,6 +710,9 @@ void test_node_modify_del_query_local(void)
 	CU_ASSERT(semanage_node_query_local(sh, key, &node_local) < 0);
 
 	/* cleanup */
+	semanage_node_key_free(key_tmp);
+	semanage_node_key_free(key);
+	semanage_node_free(node_local);
 	semanage_node_free(node);
 	semanage_node_free(node_tmp);
 	cleanup_handle(SH_TRANS);
@@ -800,6 +827,8 @@ void test_node_list_local(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_node_free(records[i]);
 
+	free(records);
+
 	delete_local_node(I_FIRST);
 	delete_local_node(I_SECOND);
 	delete_local_node(I_THIRD);
diff --git a/libsemanage/tests/test_other.c b/libsemanage/tests/test_other.c
index c4ee0ed8..0a57e247 100644
--- a/libsemanage/tests/test_other.c
+++ b/libsemanage/tests/test_other.c
@@ -81,6 +81,9 @@ void test_semanage_context(void)
 	assert(str);
 	CU_ASSERT_STRING_EQUAL(str, "user_u:role_r:type_t:s0");
 
+	semanage_context_free(con);
+	con = NULL;
+
 	CU_ASSERT(semanage_context_from_string(sh, "my_u:my_r:my_t:s0",
 					       &con) >= 0);
 	CU_ASSERT_STRING_EQUAL(semanage_context_get_user(con), "my_u");
@@ -95,6 +98,7 @@ void test_semanage_context(void)
 	CU_ASSERT_STRING_EQUAL(semanage_context_get_mls(con_clone), "s0");
 
 	/* cleanup */
+	free(str);
 	semanage_context_free(con);
 	semanage_context_free(con_clone);
 	cleanup_handle(SH_CONNECT);
@@ -115,6 +119,8 @@ void test_debug(void)
 	CU_ASSERT(semanage_module_info_set_priority(sh, modinfo, -42) < 0);
 
 	/* cleanup */
+	semanage_module_info_destroy(sh, modinfo);
+	free(modinfo);
 	CU_ASSERT(semanage_disconnect(sh) >= 0);
 	semanage_handle_destroy(sh);
 }
diff --git a/libsemanage/tests/test_port.c b/libsemanage/tests/test_port.c
index 0408be4d..3b00bb73 100644
--- a/libsemanage/tests/test_port.c
+++ b/libsemanage/tests/test_port.c
@@ -146,6 +146,8 @@ semanage_port_t *get_port_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_port_free(records[i]);
 
+	free(records);
+
 	return port;
 }
 
@@ -165,6 +167,9 @@ semanage_port_key_t *get_port_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	/* cleanup */
+	semanage_port_free(port);
+
 	return key;
 }
 
@@ -181,6 +186,10 @@ void add_local_port(int port_idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_port_modify_local(sh, key, port) >= 0);
+
+	/* cleanup */
+	semanage_port_key_free(key);
+	semanage_port_free(port);
 }
 
 void delete_local_port(int port_idx)
@@ -192,6 +201,8 @@ void delete_local_port(int port_idx)
 	key = get_port_key_nth(port_idx);
 
 	CU_ASSERT_FATAL(semanage_port_del_local(sh, key) >= 0);
+
+	semanage_port_key_free(key);
 }
 
 /* Function semanage_port_compare */
@@ -447,6 +458,7 @@ void test_port_clone(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con2);
 
 	/* cleanup */
+	semanage_context_free(con);
 	semanage_port_free(port);
 	semanage_port_free(port_clone);
 	cleanup_handle(SH_CONNECT);
@@ -480,6 +492,7 @@ void test_port_query(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con_exp);
 
 	/* cleanup */
+	semanage_port_key_free(key);
 	semanage_port_free(port);
 	semanage_port_free(port_exp);
 	cleanup_handle(SH_CONNECT);
@@ -567,6 +580,8 @@ void test_port_list(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_port_free(records[i]);
 
+	free(records);
+
 	cleanup_handle(SH_CONNECT);
 }
 
@@ -599,6 +614,9 @@ void test_port_modify_del_local(void)
 	CU_ASSERT(semanage_port_query_local(sh, key, &port_local) < 0);
 
 	/* cleanup */
+	semanage_context_free(con);
+	semanage_port_key_free(key);
+	semanage_port_free(port_local);
 	semanage_port_free(port);
 	cleanup_handle(SH_TRANS);
 }
@@ -633,6 +651,7 @@ void test_port_query_local(void)
 
 	/* cleanup */
 	delete_local_port(I_FIRST);
+	semanage_port_key_free(key);
 	semanage_port_free(port);
 	semanage_port_free(port_exp);
 	cleanup_handle(SH_TRANS);
@@ -747,6 +766,8 @@ void test_port_list_local(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_port_free(records[i]);
 
+	free(records);
+
 	delete_local_port(I_FIRST);
 	delete_local_port(I_SECOND);
 	delete_local_port(I_THIRD);
@@ -773,6 +794,7 @@ void helper_port_validate_local_noport(void)
 	helper_commit();
 
 	/* cleanup */
+	semanage_port_key_free(key);
 	helper_begin_transaction();
 	delete_local_port(I_FIRST);
 	cleanup_handle(SH_TRANS);
@@ -832,6 +854,8 @@ void helper_port_validate_local_twoports(void)
 	helper_begin_transaction();
 	CU_ASSERT(semanage_port_del_local(sh, key1) >= 0);
 	CU_ASSERT(semanage_port_del_local(sh, key2) >= 0);
+	semanage_context_free(con2);
+	semanage_context_free(con1);
 	semanage_port_key_free(key1);
 	semanage_port_key_free(key2);
 	semanage_port_free(port1);
diff --git a/libsemanage/tests/test_user.c b/libsemanage/tests/test_user.c
index cd082030..c30dca86 100644
--- a/libsemanage/tests/test_user.c
+++ b/libsemanage/tests/test_user.c
@@ -130,6 +130,8 @@ semanage_user_t *get_user_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_user_free(records[i]);
 
+	free(records);
+
 	return user;
 }
 
@@ -149,6 +151,8 @@ semanage_user_key_t *get_user_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_user_free(user);
+
 	return key;
 }
 
@@ -165,6 +169,9 @@ void add_local_user(int user_idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_user_modify_local(sh, key, user) >= 0);
+
+	semanage_user_key_free(key);
+	semanage_user_free(user);
 }
 
 void delete_local_user(int user_idx)
@@ -176,6 +183,8 @@ void delete_local_user(int user_idx)
 	key = get_user_key_nth(user_idx);
 
 	CU_ASSERT_FATAL(semanage_user_del_local(sh, key) >= 0);
+
+	semanage_user_key_free(key);
 }
 
 /* Function semanage_user_compare */
@@ -391,6 +400,7 @@ void test_user_roles(void)
 	CU_ASSERT(semanage_user_get_num_roles(user) == 0);
 
 	/* cleanup */
+	free(roles_arr);
 	semanage_user_free(user);
 	cleanup_handle(SH_CONNECT);
 }
@@ -459,6 +469,7 @@ void test_user_query(void)
 	CU_ASSERT_PTR_NOT_NULL(user);
 
 	/* cleanup */
+	semanage_user_key_free(key);
 	semanage_user_free(user);
 	cleanup_handle(SH_CONNECT);
 }
@@ -546,6 +557,8 @@ void test_user_list(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_user_free(records[i]);
 
+	free(records);
+
 	cleanup_handle(SH_CONNECT);
 }
 
@@ -577,6 +590,8 @@ void test_user_modify_del_query_local(void)
 	CU_ASSERT(semanage_user_query_local(sh, key, &user_local) < 0);
 
 	/* cleanup */
+	semanage_user_key_free(key);
+	semanage_user_free(user_local);
 	semanage_user_free(user);
 	cleanup_handle(SH_TRANS);
 }
@@ -683,6 +698,8 @@ void test_user_list_local(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_user_free(records[i]);
 
+	free(records);
+
 	delete_local_user(I_FIRST);
 	delete_local_user(I_SECOND);
 	delete_local_user(I_THIRD);
diff --git a/libsemanage/tests/utilities.c b/libsemanage/tests/utilities.c
index 18393215..b28ae155 100644
--- a/libsemanage/tests/utilities.c
+++ b/libsemanage/tests/utilities.c
@@ -99,6 +99,7 @@ int write_test_policy_from_file(const char *filename) {
 	char *buf = NULL;
 	size_t len = 0;
 	FILE *fptr = fopen(filename, "rb");
+	int rc;
 
 	if (!fptr) {
 		perror("fopen");
@@ -120,7 +121,9 @@ int write_test_policy_from_file(const char *filename) {
 	fread(buf, len, 1, fptr);
 	fclose(fptr);
 
-	return write_test_policy(buf, len);
+	rc = write_test_policy(buf, len);
+	free(buf);
+	return rc;
 }
 
 int write_test_policy_src(unsigned char *data, unsigned int data_len) {
diff --git a/libsemanage/tests/utilities.h b/libsemanage/tests/utilities.h
index db4dabf9..298b3280 100644
--- a/libsemanage/tests/utilities.h
+++ b/libsemanage/tests/utilities.h
@@ -39,6 +39,8 @@
 		CU_ASSERT(semanage_context_to_string(sh, CON1, &__str) >= 0); \
 		CU_ASSERT(semanage_context_to_string(sh, CON2, &__str2) >= 0); \
 		CU_ASSERT_STRING_EQUAL(__str, __str2); \
+		free(__str2); \
+		free(__str); \
 	} while (0)
 
 
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] libsepol: do not pass NULL to memcpy
  2021-10-13 12:53 [PATCH 1/3] libsepol: do not pass NULL to memcpy Christian Göttsche
  2021-10-13 12:53 ` [PATCH 2/3] libsemanage: do not sort empty records Christian Göttsche
  2021-10-13 12:53 ` [PATCH 3/3] libsemanage/tests: free memory Christian Göttsche
@ 2021-10-16 19:30 ` Nicolas Iooss
  2021-10-19 12:50   ` Christian Göttsche
  2021-10-19 15:11 ` [PATCH v2 " Christian Göttsche
  3 siblings, 1 reply; 12+ messages in thread
From: Nicolas Iooss @ 2021-10-16 19:30 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: SElinux list

On Wed, Oct 13, 2021 at 2:54 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not
> use it as source of a memcpy(3), even with a size of 0.  memcpy(3) might
> be annotated with the function attribute nonnull and UBSan then
> complains:
>
>     link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null
>
> Use a realloc + memset instead of a calloc and free to increase the size
> of `mod->perm_map[sclassi]`.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsepol/src/link.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/libsepol/src/link.c b/libsepol/src/link.c
> index 7512a4d9..75ce2b20 100644
> --- a/libsepol/src/link.c
> +++ b/libsepol/src/link.c
> @@ -185,14 +185,12 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
>          * may have originated from the class -or- it could be from
>          * the class's common parent.*/
>         if (perm->s.value > mod->perm_map_len[sclassi]) {
> -               uint32_t *newmap = calloc(perm->s.value, sizeof(*newmap));
> +               uint32_t *newmap = realloc(mod->perm_map[sclassi], perm->s.value * sizeof(*newmap));

Hello,
It seems sad to transform a calloc call to an explicit multiplication
which can overflow. Nowadays glibc provides reallocarray, for a
"realloc with multiplication overflow checking". Could you use it here
instead?

I saw in another patch ([RFC PATCH 09/35] libsepol: use reallocarray
wrapper to avoid overflows ;
https://lore.kernel.org/selinux/20211011162533.53404-10-cgzones@googlemail.com/T/#u)
that you introduced reallocarray calls in other places, with a
fallback implementation for systems which lack it. When I saw it, I
was wondering: which C library does not provide this function?

- glibc has it since version 2.29 (according to
https://man7.org/linux/man-pages/man3/realloc.3.html ; this version
was released on 2019-02-01)
- musl has it since version 1.2.2 (thanks to
https://git.musl-libc.org/cgit/musl/commit/?id=821083ac7b54eaa040d5a8ddc67c6206a175e0ca
; released on 2021-01-15)
- bionic has it since 2018
(https://android.googlesource.com/platform/bionic/+/b177085ce7219562eecf77f2e8de49f8f2605005%5E%21/)
- newlib has it since 2017
(https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=eb14d0cc649978c10928bf38c1847f295bf0de69)

However uclic-ng 1.0.39 (released ten days ago) does not provide
reallocarray. It is the only maintained and open-source C library
compatible with Linux that I could find which would not provide
reallocarray. Did I miss a library which is likely to be used by
libsepol users?

In my humble opinion, this means that for future releases we can
consider that systems are expected to provide reallocarray, and that
we can introduce a Makefile variable which would introduce a custom
internal reallocarray implementation in libsepol. This means that I
suggest using something like this in libsepol/src/Makefile:

USE_INTERNAL_REALLOCARRAY ?= n
ifeq (USE_INTERNAL_REALLOCARRAY, y)
  CFLAGS += -DUSE_INTERNAL_REALLOCARRAY
endif

and I suggest putting the internal reallocarray implementation from
your patch in libsepol/src/private.h (like you did), around "#ifdef
USE_INTERNAL_REALLOCARRAY" statements. This way, libsepol would work
out-of-the-box on most systems, would be compatible on systems without
reallocarray by using "make USE_INTERNAL_REALLOCARRAY=y", and would
not try to auto-detect it at build-time by some compiler magic in
Makefile (which would avoid issues similar as the one libapparmor had
in http://lists.busybox.net/pipermail/buildroot/2020-October/294691.html).
What do you think of these suggestions?

Thanks,
Nicolas

>                 if (newmap == NULL) {
>                         ERR(state->handle, "Out of memory!");
>                         return -1;
>                 }
> -               memcpy(newmap, mod->perm_map[sclassi],
> -                      mod->perm_map_len[sclassi] * sizeof(*newmap));
> -               free(mod->perm_map[sclassi]);
> +               memset(newmap + mod->perm_map_len[sclassi], '\0', perm->s.value - mod->perm_map_len[sclassi]);
>                 mod->perm_map[sclassi] = newmap;
>                 mod->perm_map_len[sclassi] = perm->s.value;
>         }
> --
> 2.33.0
>


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] libsemanage: do not sort empty records
  2021-10-13 12:53 ` [PATCH 2/3] libsemanage: do not sort empty records Christian Göttsche
@ 2021-10-16 19:34   ` Nicolas Iooss
  0 siblings, 0 replies; 12+ messages in thread
From: Nicolas Iooss @ 2021-10-16 19:34 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: SElinux list

On Wed, Oct 13, 2021 at 2:54 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Do not sort empty records to avoid calling qsort(3) with a NULL pointer.
> qsort(3) might be annotated with the function attribute nonnull and
> UBSan then complains:
>
>     database_join.c:80:2: runtime error: null pointer passed as argument 1, which is declared to never be null
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsemanage/src/database_join.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/libsemanage/src/database_join.c b/libsemanage/src/database_join.c
> index b9b35a61..b0e66e53 100644
> --- a/libsemanage/src/database_join.c
> +++ b/libsemanage/src/database_join.c
> @@ -77,10 +77,12 @@ static int dbase_join_cache(semanage_handle_t * handle, dbase_join_t * dbase)
>                 goto err;
>
>         /* Sort for quicker merge later */
> -       qsort(records1, rcount1, sizeof(record1_t *),
> -             (int (*)(const void *, const void *))rtable1->compare2_qsort);
> -       qsort(records2, rcount2, sizeof(record2_t *),
> -             (int (*)(const void *, const void *))rtable2->compare2_qsort);
> +       if (rcount1 > 0)
> +               qsort(records1, rcount1, sizeof(record1_t *),
> +                     (int (*)(const void *, const void *))rtable1->compare2_qsort);
> +       if (rcount2 > 0)
> +               qsort(records2, rcount2, sizeof(record2_t *),
> +                     (int (*)(const void *, const void *))rtable2->compare2_qsort);
>
>         /* Now merge into this dbase */
>         while (i < rcount1 || j < rcount2) {
> --
> 2.33.0
>

Hello,
This patch looks good, and the code would be more readable with braces
around the qsort calls (so it is clear with a fast glance over the
code that there is no issue of not using braces here). Could you add
braces like this?

if (rcount1 > 0) {
    qsort(records1, rcount1, sizeof(record1_t *),
        (int (*)(const void *, const void *))rtable1->compare2_qsort);
}

Thanks,
Nicolas


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] libsemanage/tests: free memory
  2021-10-13 12:53 ` [PATCH 3/3] libsemanage/tests: free memory Christian Göttsche
@ 2021-10-16 20:08   ` Nicolas Iooss
  0 siblings, 0 replies; 12+ messages in thread
From: Nicolas Iooss @ 2021-10-16 20:08 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: SElinux list

On Wed, Oct 13, 2021 at 2:54 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Free all memory in test cases, reported by LeakSanitizer.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsemanage/tests/test_bool.c      | 33 +++++++++++++++++++++++++++++-
>  libsemanage/tests/test_fcontext.c  | 31 +++++++++++++++++++++++++++-
>  libsemanage/tests/test_ibendport.c | 13 ++++++++++++
>  libsemanage/tests/test_iface.c     | 24 ++++++++++++++++++++++
>  libsemanage/tests/test_node.c      | 29 ++++++++++++++++++++++++++
>  libsemanage/tests/test_other.c     |  6 ++++++
>  libsemanage/tests/test_port.c      | 24 ++++++++++++++++++++++
>  libsemanage/tests/test_user.c      | 17 +++++++++++++++
>  libsemanage/tests/utilities.c      |  5 ++++-
>  libsemanage/tests/utilities.h      |  2 ++
>  10 files changed, 181 insertions(+), 3 deletions(-)

The diff was very large, so I posted 3 comments on GitHub:
https://github.com/SELinuxProject/selinux/pull/321/commits/322243ee7d34f1b3e23c04c75a6b5c4f597092f7#r730306247
and https://github.com/SELinuxProject/selinux/pull/321/commits/322243ee7d34f1b3e23c04c75a6b5c4f597092f7#r730306949
and https://github.com/SELinuxProject/selinux/pull/321/commits/322243ee7d34f1b3e23c04c75a6b5c4f597092f7#r730307266
, about the same pattern which causes (in my humble opinion) issues.
The pattern is in:

CU_ASSERT(semanage_ibendport_query_local(sh, key,
&ibendport_local) >= 0);
CU_ASSERT_PTR_NOT_NULL_FATAL(ibendport_local);
CU_ASSERT(semanage_ibendport_del_local(sh, key) >= 0);
CU_ASSERT(semanage_ibendport_query_local(sh, key,
&ibendport_local) < 0);

/* cleanup */
semanage_ibendport_key_free(key);
semanage_ibendport_free(ibendport_local);

The last ..._free occurs after a second call to
semanage_ibendport_query_local. It seems more appropriate to free the
memory right after CU_ASSERT_PTR_NOT_NULL_FATAL(ibendport_local),
before the second semanage_ibendport_query_local. So if an error
happens, there is no memory issue too.

This pattern is similar when testing boolean, fiface, node, port, user
and context objects.

What do you think?

Thanks,
Nicolas


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] libsepol: do not pass NULL to memcpy
  2021-10-16 19:30 ` [PATCH 1/3] libsepol: do not pass NULL to memcpy Nicolas Iooss
@ 2021-10-19 12:50   ` Christian Göttsche
  0 siblings, 0 replies; 12+ messages in thread
From: Christian Göttsche @ 2021-10-19 12:50 UTC (permalink / raw)
  To: Nicolas Iooss; +Cc: SElinux list

On Sat, 16 Oct 2021 at 21:30, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Wed, Oct 13, 2021 at 2:54 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not
> > use it as source of a memcpy(3), even with a size of 0.  memcpy(3) might
> > be annotated with the function attribute nonnull and UBSan then
> > complains:
> >
> >     link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null
> >
> > Use a realloc + memset instead of a calloc and free to increase the size
> > of `mod->perm_map[sclassi]`.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> >  libsepol/src/link.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/libsepol/src/link.c b/libsepol/src/link.c
> > index 7512a4d9..75ce2b20 100644
> > --- a/libsepol/src/link.c
> > +++ b/libsepol/src/link.c
> > @@ -185,14 +185,12 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
> >          * may have originated from the class -or- it could be from
> >          * the class's common parent.*/
> >         if (perm->s.value > mod->perm_map_len[sclassi]) {
> > -               uint32_t *newmap = calloc(perm->s.value, sizeof(*newmap));
> > +               uint32_t *newmap = realloc(mod->perm_map[sclassi], perm->s.value * sizeof(*newmap));
>
> Hello,
> It seems sad to transform a calloc call to an explicit multiplication
> which can overflow. Nowadays glibc provides reallocarray, for a
> "realloc with multiplication overflow checking". Could you use it here
> instead?
>

Yes, I thought about that, but I wanted this diff to be as small as possible.
One option would be to wait until reallocarray is available (via
fallback or requirement (see later)) or keep the calloc+memcpy, but
not invoke the memset on a size of 0.

> I saw in another patch ([RFC PATCH 09/35] libsepol: use reallocarray
> wrapper to avoid overflows ;
> https://lore.kernel.org/selinux/20211011162533.53404-10-cgzones@googlemail.com/T/#u)
> that you introduced reallocarray calls in other places, with a
> fallback implementation for systems which lack it. When I saw it, I
> was wondering: which C library does not provide this function?
>
> - glibc has it since version 2.29 (according to
> https://man7.org/linux/man-pages/man3/realloc.3.html ; this version
> was released on 2019-02-01)
> - musl has it since version 1.2.2 (thanks to
> https://git.musl-libc.org/cgit/musl/commit/?id=821083ac7b54eaa040d5a8ddc67c6206a175e0ca
> ; released on 2021-01-15)
> - bionic has it since 2018
> (https://android.googlesource.com/platform/bionic/+/b177085ce7219562eecf77f2e8de49f8f2605005%5E%21/)
> - newlib has it since 2017
> (https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=eb14d0cc649978c10928bf38c1847f295bf0de69)
>
> However uclic-ng 1.0.39 (released ten days ago) does not provide
> reallocarray. It is the only maintained and open-source C library
> compatible with Linux that I could find which would not provide
> reallocarray. Did I miss a library which is likely to be used by
> libsepol users?
>
> In my humble opinion, this means that for future releases we can
> consider that systems are expected to provide reallocarray, and that
> we can introduce a Makefile variable which would introduce a custom
> internal reallocarray implementation in libsepol. This means that I
> suggest using something like this in libsepol/src/Makefile:
>
> USE_INTERNAL_REALLOCARRAY ?= n
> ifeq (USE_INTERNAL_REALLOCARRAY, y)
>   CFLAGS += -DUSE_INTERNAL_REALLOCARRAY
> endif
>
> and I suggest putting the internal reallocarray implementation from
> your patch in libsepol/src/private.h (like you did), around "#ifdef
> USE_INTERNAL_REALLOCARRAY" statements. This way, libsepol would work
> out-of-the-box on most systems, would be compatible on systems without
> reallocarray by using "make USE_INTERNAL_REALLOCARRAY=y", and would
> not try to auto-detect it at build-time by some compiler magic in
> Makefile (which would avoid issues similar as the one libapparmor had
> in http://lists.busybox.net/pipermail/buildroot/2020-October/294691.html).
> What do you think of these suggestions?
>

Looking for example at
https://github.com/SELinuxProject/selinux/commit/4859b738136ef8889fbb71a166cfec8d313ba4e0
(supporting gcc 4.8) I am not sure what compilers and standard C
libraries should be supported for building the SELinux userland
libraries and tools. Until any statement of the maintainer team, I
prefer the Makefile hack from
https://lore.kernel.org/selinux/20211011162533.53404-10-cgzones@googlemail.com/T/#u
to support standard C libraries without reallocarray(3).


> Thanks,
> Nicolas
>
> >                 if (newmap == NULL) {
> >                         ERR(state->handle, "Out of memory!");
> >                         return -1;
> >                 }
> > -               memcpy(newmap, mod->perm_map[sclassi],
> > -                      mod->perm_map_len[sclassi] * sizeof(*newmap));
> > -               free(mod->perm_map[sclassi]);
> > +               memset(newmap + mod->perm_map_len[sclassi], '\0', perm->s.value - mod->perm_map_len[sclassi]);
> >                 mod->perm_map[sclassi] = newmap;
> >                 mod->perm_map_len[sclassi] = perm->s.value;
> >         }
> > --
> > 2.33.0
> >
>

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH v2 1/3] libsepol: do not pass NULL to memcpy
  2021-10-13 12:53 [PATCH 1/3] libsepol: do not pass NULL to memcpy Christian Göttsche
                   ` (2 preceding siblings ...)
  2021-10-16 19:30 ` [PATCH 1/3] libsepol: do not pass NULL to memcpy Nicolas Iooss
@ 2021-10-19 15:11 ` Christian Göttsche
  2021-10-19 15:11   ` [PATCH v2 2/3] libsemanage: do not sort empty records Christian Göttsche
                     ` (2 more replies)
  3 siblings, 3 replies; 12+ messages in thread
From: Christian Göttsche @ 2021-10-19 15:11 UTC (permalink / raw)
  To: selinux

For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not
use it as source of a memcpy(3), even with a size of 0.  memcpy(3) might
be annotated with the function attribute nonnull and UBSan then
complains:

    link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

---
v2:
   drop realloc rewrite, just check for 0 size
---
 libsepol/src/link.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libsepol/src/link.c b/libsepol/src/link.c
index 7512a4d9..b14240d5 100644
--- a/libsepol/src/link.c
+++ b/libsepol/src/link.c
@@ -190,8 +190,9 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
 			ERR(state->handle, "Out of memory!");
 			return -1;
 		}
-		memcpy(newmap, mod->perm_map[sclassi],
-		       mod->perm_map_len[sclassi] * sizeof(*newmap));
+		if (mod->perm_map_len[sclassi] > 0) {
+			memcpy(newmap, mod->perm_map[sclassi], mod->perm_map_len[sclassi] * sizeof(*newmap));
+		}
 		free(mod->perm_map[sclassi]);
 		mod->perm_map[sclassi] = newmap;
 		mod->perm_map_len[sclassi] = perm->s.value;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 2/3] libsemanage: do not sort empty records
  2021-10-19 15:11 ` [PATCH v2 " Christian Göttsche
@ 2021-10-19 15:11   ` Christian Göttsche
  2021-10-19 15:11   ` [PATCH v2 3/3] libsemanage/tests: free memory Christian Göttsche
  2021-11-08 21:38   ` [PATCH v2 1/3] libsepol: do not pass NULL to memcpy Nicolas Iooss
  2 siblings, 0 replies; 12+ messages in thread
From: Christian Göttsche @ 2021-10-19 15:11 UTC (permalink / raw)
  To: selinux

Do not sort empty records to avoid calling qsort(3) with a NULL pointer.
qsort(3) might be annotated with the function attribute nonnull and
UBSan then complains:

    database_join.c:80:2: runtime error: null pointer passed as argument 1, which is declared to never be null

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

---
v2:
   add brackets around if blocks
---
 libsemanage/src/database_join.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libsemanage/src/database_join.c b/libsemanage/src/database_join.c
index b9b35a61..a49a6226 100644
--- a/libsemanage/src/database_join.c
+++ b/libsemanage/src/database_join.c
@@ -77,10 +77,14 @@ static int dbase_join_cache(semanage_handle_t * handle, dbase_join_t * dbase)
 		goto err;
 
 	/* Sort for quicker merge later */
-	qsort(records1, rcount1, sizeof(record1_t *),
-	      (int (*)(const void *, const void *))rtable1->compare2_qsort);
-	qsort(records2, rcount2, sizeof(record2_t *),
-	      (int (*)(const void *, const void *))rtable2->compare2_qsort);
+	if (rcount1 > 0) {
+		qsort(records1, rcount1, sizeof(record1_t *),
+		      (int (*)(const void *, const void *))rtable1->compare2_qsort);
+	}
+	if (rcount2 > 0) {
+		qsort(records2, rcount2, sizeof(record2_t *),
+		      (int (*)(const void *, const void *))rtable2->compare2_qsort);
+	}
 
 	/* Now merge into this dbase */
 	while (i < rcount1 || j < rcount2) {
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 3/3] libsemanage/tests: free memory
  2021-10-19 15:11 ` [PATCH v2 " Christian Göttsche
  2021-10-19 15:11   ` [PATCH v2 2/3] libsemanage: do not sort empty records Christian Göttsche
@ 2021-10-19 15:11   ` Christian Göttsche
  2021-11-08 21:38   ` [PATCH v2 1/3] libsepol: do not pass NULL to memcpy Nicolas Iooss
  2 siblings, 0 replies; 12+ messages in thread
From: Christian Göttsche @ 2021-10-19 15:11 UTC (permalink / raw)
  To: selinux

Free all memory in test cases, reported by LeakSanitizer.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

---
v2:
   reorder some free calls before failure tests on the variable in
   question
---
 libsemanage/tests/test_bool.c      | 34 +++++++++++++++++++++++++++++-
 libsemanage/tests/test_fcontext.c  | 32 +++++++++++++++++++++++++++-
 libsemanage/tests/test_ibendport.c | 13 ++++++++++++
 libsemanage/tests/test_iface.c     | 24 +++++++++++++++++++++
 libsemanage/tests/test_node.c      | 29 +++++++++++++++++++++++++
 libsemanage/tests/test_other.c     |  6 ++++++
 libsemanage/tests/test_port.c      | 24 +++++++++++++++++++++
 libsemanage/tests/test_user.c      | 17 +++++++++++++++
 libsemanage/tests/utilities.c      |  5 ++++-
 libsemanage/tests/utilities.h      |  2 ++
 10 files changed, 183 insertions(+), 3 deletions(-)

diff --git a/libsemanage/tests/test_bool.c b/libsemanage/tests/test_bool.c
index ae80d448..7bf5225b 100644
--- a/libsemanage/tests/test_bool.c
+++ b/libsemanage/tests/test_bool.c
@@ -132,6 +132,8 @@ semanage_bool_t *get_bool_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_bool_free(records[i]);
 
+	free(records);
+
 	return boolean;
 }
 
@@ -163,6 +165,8 @@ semanage_bool_key_t *get_bool_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_bool_free(boolean);
+
 	return key;
 }
 
@@ -196,6 +200,9 @@ void add_local_bool(const char *name)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(boolean);
 
 	CU_ASSERT_FATAL(semanage_bool_modify_local(sh, key, boolean) >= 0);
+
+	semanage_bool_key_free(key);
+	semanage_bool_free(boolean);
 }
 
 void delete_local_bool(const char *name)
@@ -208,6 +215,8 @@ void delete_local_bool(const char *name)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_bool_del_local(sh, key) >= 0);
+
+	semanage_bool_key_free(key);
 }
 
 /* Function bool_key_create */
@@ -447,6 +456,8 @@ void helper_bool_create(level_t level)
 	CU_ASSERT_PTR_NULL(semanage_bool_get_name(boolean));
 	CU_ASSERT(semanage_bool_get_value(boolean) == 0);
 
+	semanage_bool_free(boolean);
+
 	cleanup_handle(level);
 }
 
@@ -483,6 +494,9 @@ void helper_bool_clone(level_t level, int bool_idx)
 
 	CU_ASSERT_EQUAL(val, val_clone);
 
+	semanage_bool_free(boolean_clone);
+	semanage_bool_free(boolean);
+
 	cleanup_handle(level);
 }
 
@@ -514,6 +528,9 @@ void helper_bool_query(level_t level, const char *bool_str, int exp_res)
 		CU_ASSERT_PTR_NULL(resp);
 	}
 
+	semanage_bool_free(resp);
+	semanage_bool_key_free(key);
+
 	cleanup_handle(level);
 }
 
@@ -647,6 +664,8 @@ void helper_bool_list(level_t level)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_bool_free(records[i]);
 
+	free(records);
+
 	cleanup_handle(level);
 }
 
@@ -662,7 +681,7 @@ void helper_bool_modify_del_local(level_t level, const char *name,
 				  int old_val, int exp_res)
 {
 	semanage_bool_t *boolean;
-	semanage_bool_t *boolean_local;
+	semanage_bool_t *boolean_local = NULL;
 	semanage_bool_key_t *key = NULL;
 	int res;
 	int new_val;
@@ -696,6 +715,8 @@ void helper_bool_modify_del_local(level_t level, const char *name,
 		CU_ASSERT(semanage_bool_query_local(sh, key,
 					            &boolean_local) >= 0);
 		CU_ASSERT(semanage_bool_compare2(boolean_local, boolean) == 0);
+		semanage_bool_free(boolean_local);
+
 		CU_ASSERT(semanage_bool_del_local(sh, key) >= 0);
 		CU_ASSERT(semanage_bool_query_local(sh, key,
 						    &boolean_local) < 0);
@@ -734,15 +755,18 @@ void test_bool_query_local(void)
 
 	/* transaction */
 	setup_handle(SH_TRANS);
+	semanage_bool_key_free(key);
 	CU_ASSERT(semanage_bool_key_create(sh, BOOL1_NAME, &key) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(key);
 
 	CU_ASSERT(semanage_bool_query_local(sh, key, &resp) < 0);
 	CU_ASSERT_PTR_NULL(resp);
+	semanage_bool_free(resp);
 
 	add_local_bool(BOOL1_NAME);
 	CU_ASSERT(semanage_bool_query_local(sh, key, &resp) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(resp);
+	semanage_bool_free(resp);
 
 	semanage_bool_key_free(key);
 	CU_ASSERT(semanage_bool_key_create(sh, BOOL2_NAME, &key) >= 0);
@@ -751,8 +775,10 @@ void test_bool_query_local(void)
 	add_local_bool(BOOL2_NAME);
 	CU_ASSERT(semanage_bool_query_local(sh, key, &resp) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(resp);
+	semanage_bool_free(resp);
 
 	/* cleanup */
+	semanage_bool_key_free(key);
 	delete_local_bool(BOOL1_NAME);
 	delete_local_bool(BOOL2_NAME);
 	cleanup_handle(SH_TRANS);
@@ -784,6 +810,7 @@ void test_bool_exists_local(void)
 	CU_ASSERT(resp == 0);
 
 	/* cleanup */
+	semanage_bool_key_free(key);
 	cleanup_handle(SH_TRANS);
 }
 
@@ -918,12 +945,17 @@ void test_bool_list_local(void)
 	CU_ASSERT(semanage_bool_list_local(sh, &records, &count) >= 0);
 	CU_ASSERT(count == init_count + 1);
 	CU_ASSERT_PTR_NOT_NULL(records[0]);
+	semanage_bool_free(records[0]);
+	free(records);
 
 	add_local_bool(BOOL2_NAME);
 	CU_ASSERT(semanage_bool_list_local(sh, &records, &count) >= 0);
 	CU_ASSERT(count == init_count + 2);
 	CU_ASSERT_PTR_NOT_NULL(records[0]);
 	CU_ASSERT_PTR_NOT_NULL(records[1]);
+	semanage_bool_free(records[0]);
+	semanage_bool_free(records[1]);
+	free(records);
 
 	/* cleanup */
 	delete_local_bool(BOOL1_NAME);
diff --git a/libsemanage/tests/test_fcontext.c b/libsemanage/tests/test_fcontext.c
index 62af711f..a5fcf849 100644
--- a/libsemanage/tests/test_fcontext.c
+++ b/libsemanage/tests/test_fcontext.c
@@ -214,6 +214,8 @@ semanage_fcontext_t *get_fcontext_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_fcontext_free(records[i]);
 
+	free(records);
+
 	return fcontext;
 }
 
@@ -230,6 +232,8 @@ semanage_fcontext_key_t *get_fcontext_key_nth(int idx)
 	CU_ASSERT_FATAL(semanage_fcontext_key_extract(sh, fcontext, &key) >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_fcontext_free(fcontext);
+
 	return key;
 }
 
@@ -246,6 +250,10 @@ void add_local_fcontext(int fcontext_idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_fcontext_modify_local(sh, key, fcontext) >= 0);
+
+	/* cleanup */
+	semanage_fcontext_key_free(key);
+	semanage_fcontext_free(fcontext);
 }
 
 void delete_local_fcontext(int fcontext_idx)
@@ -257,6 +265,8 @@ void delete_local_fcontext(int fcontext_idx)
 	key = get_fcontext_key_nth(fcontext_idx);
 
 	CU_ASSERT_FATAL(semanage_fcontext_del_local(sh, key) >= 0);
+
+	semanage_fcontext_key_free(key);
 }
 
 semanage_fcontext_key_t *get_fcontext_key_from_str(const char *str, int type)
@@ -477,6 +487,7 @@ void helper_fcontext_get_set_con(level_t level, int fcontext_idx,
 	}
 
 	/* cleanup */
+	semanage_context_free(con);
 	semanage_fcontext_free(fcontext);
 	cleanup_handle(level);
 }
@@ -587,12 +598,14 @@ void helper_fcontext_query(level_t level, const char *fcontext_expr,
 		CU_ASSERT(res >= 0);
 		const char *expr = semanage_fcontext_get_expr(resp);
 		CU_ASSERT_STRING_EQUAL(expr, fcontext_expr);
+		semanage_fcontext_free(resp);
 	} else {
 		CU_ASSERT(res < 0);
 		CU_ASSERT(resp == (void *) 42);
 	}
 
 	/* cleanup */
+	semanage_fcontext_key_free(key);
 	cleanup_handle(level);
 }
 
@@ -752,6 +765,8 @@ void helper_fcontext_list(level_t level)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_fcontext_free(records[i]);
 
+	free(records);
+
 	/* cleanup */
 	cleanup_handle(level);
 }
@@ -768,7 +783,7 @@ void helper_fcontext_modify_del_local(level_t level, int fcontext_idx,
 				      const char *con_str, int exp_res)
 {
 	semanage_fcontext_t *fcontext;
-	semanage_fcontext_t *fcontext_local;
+	semanage_fcontext_t *fcontext_local = NULL;
 	semanage_fcontext_key_t *key = NULL;
 	semanage_context_t *con = NULL;
 	int res;
@@ -803,6 +818,8 @@ void helper_fcontext_modify_del_local(level_t level, int fcontext_idx,
 					                &fcontext_local) >= 0);
 		CU_ASSERT(semanage_fcontext_compare2(fcontext_local,
 						     fcontext) == 0);
+		semanage_fcontext_free(fcontext_local);
+
 		CU_ASSERT(semanage_fcontext_del_local(sh, key) >= 0);
 		CU_ASSERT(semanage_fcontext_query_local(sh, key,
 					                &fcontext_local) < 0);
@@ -811,6 +828,7 @@ void helper_fcontext_modify_del_local(level_t level, int fcontext_idx,
 	}
 
 	/* cleanup */
+	semanage_context_free(con);
 	semanage_fcontext_key_free(key);
 	semanage_fcontext_free(fcontext);
 	cleanup_handle(level);
@@ -846,6 +864,7 @@ void test_fcontext_query_local(void)
 	/* transaction */
 	setup_handle(SH_TRANS);
 
+	semanage_fcontext_key_free(key);
 	key = get_fcontext_key_nth(I_FIRST);
 	CU_ASSERT(semanage_fcontext_query_local(sh, key, &resp) < 0);
 	CU_ASSERT_PTR_NULL(resp);
@@ -853,14 +872,19 @@ void test_fcontext_query_local(void)
 	add_local_fcontext(I_FIRST);
 	CU_ASSERT(semanage_fcontext_query_local(sh, key, &resp) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(resp);
+	semanage_fcontext_free(resp);
+	resp = NULL;
 
 	semanage_fcontext_key_free(key);
 	key = get_fcontext_key_nth(I_SECOND);
 	add_local_fcontext(I_SECOND);
 	CU_ASSERT(semanage_fcontext_query_local(sh, key, &resp) >= 0);
 	CU_ASSERT_PTR_NOT_NULL(resp);
+	semanage_fcontext_free(resp);
+	resp = NULL;
 
 	/* cleanup */
+	semanage_fcontext_key_free(key);
 	delete_local_fcontext(I_FIRST);
 	delete_local_fcontext(I_SECOND);
 	cleanup_handle(SH_TRANS);
@@ -898,6 +922,7 @@ void test_fcontext_exists_local(void)
 	CU_ASSERT(resp == 0);
 
 	/* cleanup */
+	semanage_fcontext_key_free(key);
 	cleanup_handle(SH_TRANS);
 }
 
@@ -1031,12 +1056,17 @@ void test_fcontext_list_local(void)
 	CU_ASSERT(semanage_fcontext_list_local(sh, &records, &count) >= 0);
 	CU_ASSERT(count == 1);
 	CU_ASSERT_PTR_NOT_NULL(records[0]);
+	semanage_fcontext_free(records[0]);
+	free(records);
 
 	add_local_fcontext(I_SECOND);
 	CU_ASSERT(semanage_fcontext_list_local(sh, &records, &count) >= 0);
 	CU_ASSERT(count == 2);
 	CU_ASSERT_PTR_NOT_NULL(records[0]);
 	CU_ASSERT_PTR_NOT_NULL(records[1]);
+	semanage_fcontext_free(records[0]);
+	semanage_fcontext_free(records[1]);
+	free(records);
 
 	/* cleanup */
 	delete_local_fcontext(I_FIRST);
diff --git a/libsemanage/tests/test_ibendport.c b/libsemanage/tests/test_ibendport.c
index 79a8e2c8..8addc908 100644
--- a/libsemanage/tests/test_ibendport.c
+++ b/libsemanage/tests/test_ibendport.c
@@ -113,6 +113,8 @@ semanage_ibendport_t *get_ibendport_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_ibendport_free(records[i]);
 
+	free(records);
+
 	return ibendport;
 }
 
@@ -132,6 +134,8 @@ semanage_ibendport_key_t *get_ibendport_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_ibendport_free(ibendport);
+
 	return key;
 }
 
@@ -148,6 +152,9 @@ void add_local_ibendport(int idx)
 
 	CU_ASSERT_FATAL(semanage_ibendport_modify_local(sh, key,
 							ibendport) >= 0);
+
+	semanage_ibendport_key_free(key);
+	semanage_ibendport_free(ibendport);
 }
 
 void delete_local_ibendport(int idx)
@@ -155,6 +162,8 @@ void delete_local_ibendport(int idx)
 	semanage_ibendport_key_t *key = NULL;
 	key = get_ibendport_key_nth(idx);
 	CU_ASSERT_FATAL(semanage_ibendport_del_local(sh, key) >= 0);
+
+	semanage_ibendport_key_free(key);
 }
 
 /* Function semanage_ibendport_query */
@@ -195,7 +204,9 @@ void test_ibendport_query(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con_exp);
 
 	/* cleanup */
+	free(name_exp);
 	free(name);
+	semanage_ibendport_key_free(key);
 	semanage_ibendport_free(ibendport);
 	semanage_ibendport_free(ibendport_exp);
 	cleanup_handle(SH_CONNECT);
@@ -356,12 +367,14 @@ void test_ibendport_modify_del_query_local(void)
 	CU_ASSERT(semanage_ibendport_query_local(sh, key,
 						 &ibendport_local) >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(ibendport_local);
+	semanage_ibendport_free(ibendport_local);
 
 	CU_ASSERT(semanage_ibendport_del_local(sh, key) >= 0);
 	CU_ASSERT(semanage_ibendport_query_local(sh, key,
 						 &ibendport_local) < 0);
 
 	/* cleanup */
+	semanage_ibendport_key_free(key);
 	semanage_ibendport_free(ibendport);
 	cleanup_handle(SH_TRANS);
 }
diff --git a/libsemanage/tests/test_iface.c b/libsemanage/tests/test_iface.c
index d5d530a8..434372f8 100644
--- a/libsemanage/tests/test_iface.c
+++ b/libsemanage/tests/test_iface.c
@@ -139,6 +139,8 @@ semanage_iface_t *get_iface_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_iface_free(records[i]);
 
+	free(records);
+
 	return iface;
 }
 
@@ -157,6 +159,9 @@ semanage_iface_key_t *get_iface_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	/* cleanup */
+	semanage_iface_free(iface);
+
 	return key;
 }
 
@@ -171,6 +176,10 @@ void add_local_iface(int idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_iface_modify_local(sh, key, iface) >= 0);
+
+	/* cleanup */
+	semanage_iface_key_free(key);
+	semanage_iface_free(iface);
 }
 
 void delete_local_iface(int idx)
@@ -178,6 +187,9 @@ void delete_local_iface(int idx)
 	semanage_iface_key_t *key = NULL;
 	key = get_iface_key_nth(idx);
 	CU_ASSERT_FATAL(semanage_iface_del_local(sh, key) >= 0);
+
+	/* cleanup */
+	semanage_iface_key_free(key);
 }
 
 /* Function semanage_iface_compare */
@@ -309,6 +321,7 @@ void test_iface_get_set_ifcon(void)
 	CU_ASSERT_CONTEXT_EQUAL(con1, con2);
 
 	/* cleanup */
+	semanage_context_free(con1);
 	semanage_iface_free(iface);
 	cleanup_handle(SH_CONNECT);
 }
@@ -332,6 +345,7 @@ void test_iface_get_set_msgcon(void)
 	CU_ASSERT_CONTEXT_EQUAL(con1, con2);
 
 	/* cleanup */
+	semanage_context_free(con1);
 	semanage_iface_free(iface);
 	cleanup_handle(SH_CONNECT);
 }
@@ -357,6 +371,8 @@ void test_iface_create(void)
 	CU_ASSERT(semanage_iface_set_msgcon(sh, iface, msgcon) >= 0);
 
 	/* cleanup */
+	semanage_context_free(msgcon);
+	semanage_context_free(ifcon);
 	semanage_iface_free(iface);
 	cleanup_handle(SH_CONNECT);
 }
@@ -393,6 +409,8 @@ void test_iface_clone(void)
 	CU_ASSERT_CONTEXT_EQUAL(msgcon, msgcon2);
 
 	/* cleanup */
+	semanage_context_free(msgcon);
+	semanage_context_free(ifcon);
 	semanage_iface_free(iface);
 	semanage_iface_free(iface_clone);
 	cleanup_handle(SH_CONNECT);
@@ -426,6 +444,7 @@ void test_iface_query(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con_exp);
 
 	/* cleanup */
+	semanage_iface_key_free(key);
 	semanage_iface_free(iface);
 	semanage_iface_free(iface_exp);
 	cleanup_handle(SH_CONNECT);
@@ -513,6 +532,8 @@ void test_iface_list(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_iface_free(records[i]);
 
+	free(records);
+
 	/* cleanup */
 	cleanup_handle(SH_CONNECT);
 }
@@ -541,11 +562,13 @@ void test_iface_modify_del_query_local(void)
 
 	CU_ASSERT(semanage_iface_query_local(sh, key, &iface_local) >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(iface_local);
+	semanage_iface_free(iface_local);
 
 	CU_ASSERT(semanage_iface_del_local(sh, key) >= 0);
 	CU_ASSERT(semanage_iface_query_local(sh, key, &iface_local) < 0);
 
 	/* cleanup */
+	semanage_iface_key_free(key);
 	semanage_iface_free(iface);
 	cleanup_handle(SH_TRANS);
 }
@@ -658,6 +681,7 @@ void test_iface_list_local(void)
 	/* cleanup */
 	for (unsigned int i = 0; i < count; i++)
 		semanage_iface_free(records[i]);
+	free(records);
 
 	delete_local_iface(I_FIRST);
 	delete_local_iface(I_SECOND);
diff --git a/libsemanage/tests/test_node.c b/libsemanage/tests/test_node.c
index 53c2eb69..e49e8c3b 100644
--- a/libsemanage/tests/test_node.c
+++ b/libsemanage/tests/test_node.c
@@ -148,6 +148,8 @@ semanage_node_t *get_node_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_node_free(records[i]);
 
+	free(records);
+
 	return node;
 }
 
@@ -167,6 +169,8 @@ semanage_node_key_t *get_node_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_node_free(node);
+
 	return key;
 }
 
@@ -181,6 +185,10 @@ void add_local_node(int idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_node_modify_local(sh, key, node) >= 0);
+
+	/* cleanup */
+	semanage_node_key_free(key);
+	semanage_node_free(node);
 }
 
 void delete_local_node(int idx)
@@ -190,6 +198,9 @@ void delete_local_node(int idx)
 	key = get_node_key_nth(idx);
 
 	CU_ASSERT_FATAL(semanage_node_del_local(sh, key) >= 0);
+
+	/* cleanup */
+	semanage_node_key_free(key);
 }
 
 /* Function semanage_node_compare */
@@ -305,6 +316,7 @@ void test_node_get_set_addr(void)
 	CU_ASSERT_STRING_EQUAL(addr, "192.168.0.1");
 
 	/* cleanup */
+	free(addr);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -334,6 +346,7 @@ void test_node_get_set_addr_bytes(void)
 		CU_ASSERT(addr1[i] == addr2[i]);
 
 	/* cleanup */
+	free(addr2);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -357,6 +370,7 @@ void test_node_get_set_mask(void)
 	CU_ASSERT_STRING_EQUAL(mask, "255.255.255.0");
 
 	/* cleanup */
+	free(mask);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -386,6 +400,7 @@ void test_node_get_set_mask_bytes(void)
 		CU_ASSERT(mask1[i] == mask2[i]);
 
 	/* cleanup */
+	free(mask2);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -436,6 +451,7 @@ void test_node_get_set_con(void)
 	CU_ASSERT_CONTEXT_EQUAL(con1, con2);
 
 	/* cleanup */
+	semanage_context_free(con1);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -461,6 +477,7 @@ void test_node_create(void)
 	CU_ASSERT(semanage_node_set_con(sh, node, con) >= 0);
 
 	/* cleanup */
+	semanage_context_free(con);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -508,6 +525,9 @@ void test_node_clone(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con2);
 
 	/* cleanup */
+	free(mask2);
+	free(addr2);
+	semanage_context_free(con);
 	semanage_node_free(node);
 	semanage_node_free(node_clone);
 	cleanup_handle(SH_CONNECT);
@@ -552,6 +572,8 @@ void test_node_query(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con_exp);
 
 	/* cleanup */
+	semanage_node_key_free(key);
+	semanage_node_free(node_exp);
 	semanage_node_free(node);
 	cleanup_handle(SH_CONNECT);
 }
@@ -638,6 +660,8 @@ void test_node_list(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_node_free(records[i]);
 
+	free(records);
+
 	/* cleanup */
 	cleanup_handle(SH_CONNECT);
 }
@@ -679,6 +703,7 @@ void test_node_modify_del_query_local(void)
 
 	CU_ASSERT(semanage_node_query_local(sh, key, &node_local) >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(node_local);
+	semanage_node_free(node_local);
 
 	CU_ASSERT(semanage_node_del_local(sh, key) >= 0);
 	CU_ASSERT(semanage_node_del_local(sh, key_tmp) >= 0);
@@ -686,6 +711,8 @@ void test_node_modify_del_query_local(void)
 	CU_ASSERT(semanage_node_query_local(sh, key, &node_local) < 0);
 
 	/* cleanup */
+	semanage_node_key_free(key_tmp);
+	semanage_node_key_free(key);
 	semanage_node_free(node);
 	semanage_node_free(node_tmp);
 	cleanup_handle(SH_TRANS);
@@ -800,6 +827,8 @@ void test_node_list_local(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_node_free(records[i]);
 
+	free(records);
+
 	delete_local_node(I_FIRST);
 	delete_local_node(I_SECOND);
 	delete_local_node(I_THIRD);
diff --git a/libsemanage/tests/test_other.c b/libsemanage/tests/test_other.c
index c4ee0ed8..0a57e247 100644
--- a/libsemanage/tests/test_other.c
+++ b/libsemanage/tests/test_other.c
@@ -81,6 +81,9 @@ void test_semanage_context(void)
 	assert(str);
 	CU_ASSERT_STRING_EQUAL(str, "user_u:role_r:type_t:s0");
 
+	semanage_context_free(con);
+	con = NULL;
+
 	CU_ASSERT(semanage_context_from_string(sh, "my_u:my_r:my_t:s0",
 					       &con) >= 0);
 	CU_ASSERT_STRING_EQUAL(semanage_context_get_user(con), "my_u");
@@ -95,6 +98,7 @@ void test_semanage_context(void)
 	CU_ASSERT_STRING_EQUAL(semanage_context_get_mls(con_clone), "s0");
 
 	/* cleanup */
+	free(str);
 	semanage_context_free(con);
 	semanage_context_free(con_clone);
 	cleanup_handle(SH_CONNECT);
@@ -115,6 +119,8 @@ void test_debug(void)
 	CU_ASSERT(semanage_module_info_set_priority(sh, modinfo, -42) < 0);
 
 	/* cleanup */
+	semanage_module_info_destroy(sh, modinfo);
+	free(modinfo);
 	CU_ASSERT(semanage_disconnect(sh) >= 0);
 	semanage_handle_destroy(sh);
 }
diff --git a/libsemanage/tests/test_port.c b/libsemanage/tests/test_port.c
index 0408be4d..f4c6ec21 100644
--- a/libsemanage/tests/test_port.c
+++ b/libsemanage/tests/test_port.c
@@ -146,6 +146,8 @@ semanage_port_t *get_port_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_port_free(records[i]);
 
+	free(records);
+
 	return port;
 }
 
@@ -165,6 +167,9 @@ semanage_port_key_t *get_port_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	/* cleanup */
+	semanage_port_free(port);
+
 	return key;
 }
 
@@ -181,6 +186,10 @@ void add_local_port(int port_idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_port_modify_local(sh, key, port) >= 0);
+
+	/* cleanup */
+	semanage_port_key_free(key);
+	semanage_port_free(port);
 }
 
 void delete_local_port(int port_idx)
@@ -192,6 +201,8 @@ void delete_local_port(int port_idx)
 	key = get_port_key_nth(port_idx);
 
 	CU_ASSERT_FATAL(semanage_port_del_local(sh, key) >= 0);
+
+	semanage_port_key_free(key);
 }
 
 /* Function semanage_port_compare */
@@ -447,6 +458,7 @@ void test_port_clone(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con2);
 
 	/* cleanup */
+	semanage_context_free(con);
 	semanage_port_free(port);
 	semanage_port_free(port_clone);
 	cleanup_handle(SH_CONNECT);
@@ -480,6 +492,7 @@ void test_port_query(void)
 	CU_ASSERT_CONTEXT_EQUAL(con, con_exp);
 
 	/* cleanup */
+	semanage_port_key_free(key);
 	semanage_port_free(port);
 	semanage_port_free(port_exp);
 	cleanup_handle(SH_CONNECT);
@@ -567,6 +580,8 @@ void test_port_list(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_port_free(records[i]);
 
+	free(records);
+
 	cleanup_handle(SH_CONNECT);
 }
 
@@ -594,11 +609,14 @@ void test_port_modify_del_local(void)
 
 	con_local = semanage_port_get_con(port_local);
 	CU_ASSERT_CONTEXT_EQUAL(con, con_local);
+	semanage_port_free(port_local);
 
 	CU_ASSERT(semanage_port_del_local(sh, key) >= 0);
 	CU_ASSERT(semanage_port_query_local(sh, key, &port_local) < 0);
 
 	/* cleanup */
+	semanage_context_free(con);
+	semanage_port_key_free(key);
 	semanage_port_free(port);
 	cleanup_handle(SH_TRANS);
 }
@@ -633,6 +651,7 @@ void test_port_query_local(void)
 
 	/* cleanup */
 	delete_local_port(I_FIRST);
+	semanage_port_key_free(key);
 	semanage_port_free(port);
 	semanage_port_free(port_exp);
 	cleanup_handle(SH_TRANS);
@@ -747,6 +766,8 @@ void test_port_list_local(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_port_free(records[i]);
 
+	free(records);
+
 	delete_local_port(I_FIRST);
 	delete_local_port(I_SECOND);
 	delete_local_port(I_THIRD);
@@ -773,6 +794,7 @@ void helper_port_validate_local_noport(void)
 	helper_commit();
 
 	/* cleanup */
+	semanage_port_key_free(key);
 	helper_begin_transaction();
 	delete_local_port(I_FIRST);
 	cleanup_handle(SH_TRANS);
@@ -832,6 +854,8 @@ void helper_port_validate_local_twoports(void)
 	helper_begin_transaction();
 	CU_ASSERT(semanage_port_del_local(sh, key1) >= 0);
 	CU_ASSERT(semanage_port_del_local(sh, key2) >= 0);
+	semanage_context_free(con2);
+	semanage_context_free(con1);
 	semanage_port_key_free(key1);
 	semanage_port_key_free(key2);
 	semanage_port_free(port1);
diff --git a/libsemanage/tests/test_user.c b/libsemanage/tests/test_user.c
index cd082030..c3835c8d 100644
--- a/libsemanage/tests/test_user.c
+++ b/libsemanage/tests/test_user.c
@@ -130,6 +130,8 @@ semanage_user_t *get_user_nth(int idx)
 		if (i != (unsigned int) idx)
 			semanage_user_free(records[i]);
 
+	free(records);
+
 	return user;
 }
 
@@ -149,6 +151,8 @@ semanage_user_key_t *get_user_key_nth(int idx)
 	CU_ASSERT_FATAL(res >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
+	semanage_user_free(user);
+
 	return key;
 }
 
@@ -165,6 +169,9 @@ void add_local_user(int user_idx)
 	CU_ASSERT_PTR_NOT_NULL_FATAL(key);
 
 	CU_ASSERT_FATAL(semanage_user_modify_local(sh, key, user) >= 0);
+
+	semanage_user_key_free(key);
+	semanage_user_free(user);
 }
 
 void delete_local_user(int user_idx)
@@ -176,6 +183,8 @@ void delete_local_user(int user_idx)
 	key = get_user_key_nth(user_idx);
 
 	CU_ASSERT_FATAL(semanage_user_del_local(sh, key) >= 0);
+
+	semanage_user_key_free(key);
 }
 
 /* Function semanage_user_compare */
@@ -391,6 +400,7 @@ void test_user_roles(void)
 	CU_ASSERT(semanage_user_get_num_roles(user) == 0);
 
 	/* cleanup */
+	free(roles_arr);
 	semanage_user_free(user);
 	cleanup_handle(SH_CONNECT);
 }
@@ -459,6 +469,7 @@ void test_user_query(void)
 	CU_ASSERT_PTR_NOT_NULL(user);
 
 	/* cleanup */
+	semanage_user_key_free(key);
 	semanage_user_free(user);
 	cleanup_handle(SH_CONNECT);
 }
@@ -546,6 +557,8 @@ void test_user_list(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_user_free(records[i]);
 
+	free(records);
+
 	cleanup_handle(SH_CONNECT);
 }
 
@@ -573,10 +586,12 @@ void test_user_modify_del_query_local(void)
 
 	CU_ASSERT(semanage_user_query_local(sh, key, &user_local) >= 0);
 	CU_ASSERT_PTR_NOT_NULL_FATAL(user_local);
+	semanage_user_free(user_local);
 	CU_ASSERT(semanage_user_del_local(sh, key) >= 0);
 	CU_ASSERT(semanage_user_query_local(sh, key, &user_local) < 0);
 
 	/* cleanup */
+	semanage_user_key_free(key);
 	semanage_user_free(user);
 	cleanup_handle(SH_TRANS);
 }
@@ -683,6 +698,8 @@ void test_user_list_local(void)
 	for (unsigned int i = 0; i < count; i++)
 		semanage_user_free(records[i]);
 
+	free(records);
+
 	delete_local_user(I_FIRST);
 	delete_local_user(I_SECOND);
 	delete_local_user(I_THIRD);
diff --git a/libsemanage/tests/utilities.c b/libsemanage/tests/utilities.c
index 18393215..b28ae155 100644
--- a/libsemanage/tests/utilities.c
+++ b/libsemanage/tests/utilities.c
@@ -99,6 +99,7 @@ int write_test_policy_from_file(const char *filename) {
 	char *buf = NULL;
 	size_t len = 0;
 	FILE *fptr = fopen(filename, "rb");
+	int rc;
 
 	if (!fptr) {
 		perror("fopen");
@@ -120,7 +121,9 @@ int write_test_policy_from_file(const char *filename) {
 	fread(buf, len, 1, fptr);
 	fclose(fptr);
 
-	return write_test_policy(buf, len);
+	rc = write_test_policy(buf, len);
+	free(buf);
+	return rc;
 }
 
 int write_test_policy_src(unsigned char *data, unsigned int data_len) {
diff --git a/libsemanage/tests/utilities.h b/libsemanage/tests/utilities.h
index db4dabf9..298b3280 100644
--- a/libsemanage/tests/utilities.h
+++ b/libsemanage/tests/utilities.h
@@ -39,6 +39,8 @@
 		CU_ASSERT(semanage_context_to_string(sh, CON1, &__str) >= 0); \
 		CU_ASSERT(semanage_context_to_string(sh, CON2, &__str2) >= 0); \
 		CU_ASSERT_STRING_EQUAL(__str, __str2); \
+		free(__str2); \
+		free(__str); \
 	} while (0)
 
 
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] libsepol: do not pass NULL to memcpy
  2021-10-19 15:11 ` [PATCH v2 " Christian Göttsche
  2021-10-19 15:11   ` [PATCH v2 2/3] libsemanage: do not sort empty records Christian Göttsche
  2021-10-19 15:11   ` [PATCH v2 3/3] libsemanage/tests: free memory Christian Göttsche
@ 2021-11-08 21:38   ` Nicolas Iooss
  2021-11-11 22:01     ` Nicolas Iooss
  2 siblings, 1 reply; 12+ messages in thread
From: Nicolas Iooss @ 2021-11-08 21:38 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: SElinux list

On Tue, Oct 19, 2021 at 5:13 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not
> use it as source of a memcpy(3), even with a size of 0.  memcpy(3) might
> be annotated with the function attribute nonnull and UBSan then
> complains:
>
>     link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

For these 3 patches:

Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Sorry for the delay, I have been busy with other topics.

Thanks!
Nicolas

> ---
> v2:
>    drop realloc rewrite, just check for 0 size
> ---
>  libsepol/src/link.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/src/link.c b/libsepol/src/link.c
> index 7512a4d9..b14240d5 100644
> --- a/libsepol/src/link.c
> +++ b/libsepol/src/link.c
> @@ -190,8 +190,9 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
>                         ERR(state->handle, "Out of memory!");
>                         return -1;
>                 }
> -               memcpy(newmap, mod->perm_map[sclassi],
> -                      mod->perm_map_len[sclassi] * sizeof(*newmap));
> +               if (mod->perm_map_len[sclassi] > 0) {
> +                       memcpy(newmap, mod->perm_map[sclassi], mod->perm_map_len[sclassi] * sizeof(*newmap));
> +               }
>                 free(mod->perm_map[sclassi]);
>                 mod->perm_map[sclassi] = newmap;
>                 mod->perm_map_len[sclassi] = perm->s.value;
> --
> 2.33.0
>


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] libsepol: do not pass NULL to memcpy
  2021-11-08 21:38   ` [PATCH v2 1/3] libsepol: do not pass NULL to memcpy Nicolas Iooss
@ 2021-11-11 22:01     ` Nicolas Iooss
  0 siblings, 0 replies; 12+ messages in thread
From: Nicolas Iooss @ 2021-11-11 22:01 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: SElinux list

On Mon, Nov 8, 2021 at 10:38 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Tue, Oct 19, 2021 at 5:13 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not
> > use it as source of a memcpy(3), even with a size of 0.  memcpy(3) might
> > be annotated with the function attribute nonnull and UBSan then
> > complains:
> >
> >     link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> For these 3 patches:
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Sorry for the delay, I have been busy with other topics.
>
> Thanks!
> Nicolas

I applied these 3 patches.

Thanks!
Nicolas

> > ---
> > v2:
> >    drop realloc rewrite, just check for 0 size
> > ---
> >  libsepol/src/link.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/libsepol/src/link.c b/libsepol/src/link.c
> > index 7512a4d9..b14240d5 100644
> > --- a/libsepol/src/link.c
> > +++ b/libsepol/src/link.c
> > @@ -190,8 +190,9 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
> >                         ERR(state->handle, "Out of memory!");
> >                         return -1;
> >                 }
> > -               memcpy(newmap, mod->perm_map[sclassi],
> > -                      mod->perm_map_len[sclassi] * sizeof(*newmap));
> > +               if (mod->perm_map_len[sclassi] > 0) {
> > +                       memcpy(newmap, mod->perm_map[sclassi], mod->perm_map_len[sclassi] * sizeof(*newmap));
> > +               }
> >                 free(mod->perm_map[sclassi]);
> >                 mod->perm_map[sclassi] = newmap;
> >                 mod->perm_map_len[sclassi] = perm->s.value;
> > --
> > 2.33.0
> >


^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-11-11 22:01 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13 12:53 [PATCH 1/3] libsepol: do not pass NULL to memcpy Christian Göttsche
2021-10-13 12:53 ` [PATCH 2/3] libsemanage: do not sort empty records Christian Göttsche
2021-10-16 19:34   ` Nicolas Iooss
2021-10-13 12:53 ` [PATCH 3/3] libsemanage/tests: free memory Christian Göttsche
2021-10-16 20:08   ` Nicolas Iooss
2021-10-16 19:30 ` [PATCH 1/3] libsepol: do not pass NULL to memcpy Nicolas Iooss
2021-10-19 12:50   ` Christian Göttsche
2021-10-19 15:11 ` [PATCH v2 " Christian Göttsche
2021-10-19 15:11   ` [PATCH v2 2/3] libsemanage: do not sort empty records Christian Göttsche
2021-10-19 15:11   ` [PATCH v2 3/3] libsemanage/tests: free memory Christian Göttsche
2021-11-08 21:38   ` [PATCH v2 1/3] libsepol: do not pass NULL to memcpy Nicolas Iooss
2021-11-11 22:01     ` Nicolas Iooss

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.