[PATCH 00/23] libsepol: miscellaneous cleanup

[PATCH 00/23] libsepol: miscellaneous cleanup

[Christian Göttsche]

Clean up several code smells, compiler warnings, static analyzer issues
and UBSAN findings in libsepol.

Also declare local functions and variables static and some interface
parameters const to improve maintainability.

Christian Göttsche (23):
  libsepol: fix typos
  libsepol: resolve missing prototypes
  libsepol: remove unused functions
  libsepol: ignore UBSAN false-positives
  libsepol: avoid implicit conversions
  libsepol: avoid unsigned integer overflow
  libsepol: follow declaration-after-statement
  libsepol/cil: follow declaration-after-statement
  libsepol: remove dead stores
  libsepol: mark read-only parameters of ebitmap interfaces const
  libsepol: mark read-only parameters of type_set_ interfaces const
  libsepol: do not allocate memory of size 0
  libsepol: assure string NUL-termination
  libsepol: remove dead stores
  libsepol/cil: silence cast warning
  libsepol/cil: drop extra semicolon
  libsepol/cil: drop dead store
  libsepol/cil: drop unnecessary casts
  libsepol/cil: avoid using maybe uninitialized variables
  libsepol: drop repeated semicolons
  libsepol: drop unnecessary casts
  libsepol: declare file local variable static
  libsepol: declare read-only arrays const

 libsepol/cil/src/cil_binary.c                 |  19 +--
 libsepol/cil/src/cil_build_ast.c              |   9 +-
 libsepol/cil/src/cil_fqn.c                    |   3 +-
 libsepol/cil/src/cil_list.c                   |   7 +-
 libsepol/cil/src/cil_post.c                   |   2 +-
 libsepol/cil/src/cil_resolve_ast.c            |   8 +-
 libsepol/cil/src/cil_strpool.c                |  16 +--
 libsepol/cil/src/cil_write_ast.c              |   4 +-
 libsepol/include/sepol/policydb/conditional.h |   2 +-
 libsepol/include/sepol/policydb/ebitmap.h     |  16 +--
 libsepol/include/sepol/policydb/policydb.h    |   6 +-
 libsepol/src/assertion.c                      |   2 +-
 libsepol/src/avrule_block.c                   |   2 +-
 libsepol/src/avtab.c                          |   8 +-
 libsepol/src/booleans.c                       |   6 +-
 libsepol/src/conditional.c                    |   3 -
 libsepol/src/context_internal.h               |   1 +
 libsepol/src/debug.c                          |   2 +-
 libsepol/src/ebitmap.c                        |  29 +++--
 libsepol/src/expand.c                         |   8 +-
 libsepol/src/ibendport_record.c               |   4 +-
 libsepol/src/kernel_to_cil.c                  |   7 +-
 libsepol/src/kernel_to_conf.c                 |   5 +-
 libsepol/src/link.c                           |   2 +-
 libsepol/src/module.c                         |   2 +-
 libsepol/src/module_to_cil.c                  |  14 +-
 libsepol/src/nodes.c                          |   6 +-
 libsepol/src/polcaps.c                        |   2 +-
 libsepol/src/policydb.c                       |  70 +++++-----
 libsepol/src/policydb_internal.h              |   2 +-
 libsepol/src/policydb_validate.c              |   2 +-
 libsepol/src/private.h                        |   6 +-
 libsepol/src/services.c                       | 120 ++++++------------
 libsepol/src/sidtab.c                         |  31 -----
 libsepol/src/symtab.c                         |   6 +
 libsepol/src/util.c                           |   2 +-
 libsepol/src/write.c                          |   8 +-
 37 files changed, 187 insertions(+), 255 deletions(-)

-- 
2.32.0

[PATCH 01/23] libsepol: fix typos

[Christian Göttsche]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/cil/src/cil_build_ast.c   | 2 +-
 libsepol/cil/src/cil_resolve_ast.c | 2 +-
 libsepol/src/module_to_cil.c       | 2 +-
 libsepol/src/policydb_validate.c   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 71f14e20..42d10c87 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -3692,7 +3692,7 @@ int cil_gen_sensitivityorder(struct cil_db *db, struct cil_tree_node *parse_curr
 
 	cil_list_for_each(curr, sensorder->sens_list_str) {
 		if (curr->data == CIL_KEY_UNORDERED) {
-			cil_log(CIL_ERR, "Sensitivy order cannot be unordered.\n");
+			cil_log(CIL_ERR, "Sensitivity order cannot be unordered.\n");
 			rc = SEPOL_ERR;
 			goto exit;
 		}
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 77ffe0ff..d8481002 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -1619,7 +1619,7 @@ int cil_resolve_sensitivityorder(struct cil_tree_node *current, void *extra_args
 	cil_list_for_each(curr, sensorder->sens_list_str) {
 		rc = cil_resolve_name(current, (char *)curr->data, CIL_SYM_SENS, extra_args, &datum);
 		if (rc != SEPOL_OK) {
-			cil_log(CIL_ERR, "Failed to resolve sensitivty %s in sensitivityorder\n", (char *)curr->data);
+			cil_log(CIL_ERR, "Failed to resolve sensitivity %s in sensitivityorder\n", (char *)curr->data);
 			goto exit;
 		}
 		if (FLAVOR(datum) != CIL_SENS) {
diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 496693f4..41605eb8 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -3972,7 +3972,7 @@ int sepol_module_policydb_to_cil(FILE *fp, struct policydb *pdb, int linked)
 
 	if (pdb->policy_type != SEPOL_POLICY_BASE &&
 		pdb->policy_type != SEPOL_POLICY_MOD) {
-		log_err("Policy pakcage is not a base or module");
+		log_err("Policy package is not a base or module");
 		rc = -1;
 		goto exit;
 	}
diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
index b2891ddd..246aa6e3 100644
--- a/libsepol/src/policydb_validate.c
+++ b/libsepol/src/policydb_validate.c
@@ -641,7 +641,7 @@ static int validate_scope_index(sepol_handle_t *handle, scope_index_t *scope_ind
 	return 0;
 
 bad:
-	ERR(handle, "Invalide scope");
+	ERR(handle, "Invalid scope");
 	return -1;
 }
 
-- 
2.32.0

[PATCH 02/23] libsepol: resolve missing prototypes

[Christian Göttsche]

Declare the functions as static or include the corresponding header
file.

assertion.c:294:5: error: no previous prototype for function 'report_assertion_failures' [-Werror,-Wmissing-prototypes]
int report_assertion_failures(sepol_handle_t *handle, policydb_t *p, avrule_t *avrule)
    ^

context.c:23:5: error: no previous prototype for function 'sepol_check_context' [-Werror,-Wmissing-prototypes]
int sepol_check_context(const char *context)
    ^

expand.c:3377:5: error: no previous prototype for function 'expand_cond_av_node' [-Werror,-Wmissing-prototypes]
int expand_cond_av_node(policydb_t * p,
    ^

policydb.c:638:6: error: no previous prototype for function 'role_trans_rule_destroy' [-Werror,-Wmissing-prototypes]
void role_trans_rule_destroy(role_trans_rule_t * x)
     ^

policydb.c:1169:5: error: no previous prototype for function 'policydb_index_decls' [-Werror,-Wmissing-prototypes]
int policydb_index_decls(sepol_handle_t * handle, policydb_t * p)
    ^

policydb.c:1429:6: error: no previous prototype for function 'ocontext_selinux_free' [-Werror,-Wmissing-prototypes]
void ocontext_selinux_free(ocontext_t **ocontexts)
     ^

policydb.c:1451:6: error: no previous prototype for function 'ocontext_xen_free' [-Werror,-Wmissing-prototypes]
void ocontext_xen_free(ocontext_t **ocontexts)
     ^

policydb.c:1750:5: error: no previous prototype for function 'type_set_or' [-Werror,-Wmissing-prototypes]
int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
    ^

policydb.c:2524:5: error: no previous prototype for function 'role_trans_read' [-Werror,-Wmissing-prototypes]
int role_trans_read(policydb_t *p, struct policy_file *fp)
    ^

policydb.c:2567:5: error: no previous prototype for function 'role_allow_read' [-Werror,-Wmissing-prototypes]
int role_allow_read(role_allow_t ** r, struct policy_file *fp)
    ^

policydb.c:2842:5: error: no previous prototype for function 'filename_trans_read' [-Werror,-Wmissing-prototypes]
int filename_trans_read(policydb_t *p, struct policy_file *fp)
    ^

services.c:1027:5: error: no previous prototype for function 'sepol_validate_transition' [-Werror,-Wmissing-prototypes]
int sepol_validate_transition(sepol_security_id_t oldsid,
    ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/assertion.c        |  2 +-
 libsepol/src/context_internal.h |  1 +
 libsepol/src/expand.c           |  6 +++---
 libsepol/src/policydb.c         | 16 ++++++++--------
 libsepol/src/services.c         |  2 +-
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 266f67d7..dd2749a0 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -291,7 +291,7 @@ exit:
 	return rc;
 }
 
-int report_assertion_failures(sepol_handle_t *handle, policydb_t *p, avrule_t *avrule)
+static int report_assertion_failures(sepol_handle_t *handle, policydb_t *p, avrule_t *avrule)
 {
 	int rc;
 	struct avtab_match_args args;
diff --git a/libsepol/src/context_internal.h b/libsepol/src/context_internal.h
index 3cae28cc..3dc9cd15 100644
--- a/libsepol/src/context_internal.h
+++ b/libsepol/src/context_internal.h
@@ -1,6 +1,7 @@
 #ifndef _SEPOL_CONTEXT_INTERNAL_H_
 #define _SEPOL_CONTEXT_INTERNAL_H_
 
+#include <sepol/context.h>
 #include <sepol/context_record.h>
 
 #endif
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index a656ffad..84bfcfa3 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -3374,9 +3374,9 @@ static int expand_cond_insert(cond_av_list_t ** l,
 	return 0;
 }
 
-int expand_cond_av_node(policydb_t * p,
-			avtab_ptr_t node,
-			cond_av_list_t ** newl, avtab_t * expa)
+static int expand_cond_av_node(policydb_t * p,
+			       avtab_ptr_t node,
+			       cond_av_list_t ** newl, avtab_t * expa)
 {
 	avtab_key_t *k = &node->key;
 	avtab_datum_t *d = &node->datum;
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index ffa27971..3f7ddb11 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -635,7 +635,7 @@ void role_trans_rule_init(role_trans_rule_t * x)
 	ebitmap_init(&x->classes);
 }
 
-void role_trans_rule_destroy(role_trans_rule_t * x)
+static void role_trans_rule_destroy(role_trans_rule_t * x)
 {
 	if (x != NULL) {
 		role_set_destroy(&x->roles);
@@ -1166,7 +1166,7 @@ int policydb_index_bools(policydb_t * p)
 	return 0;
 }
 
-int policydb_index_decls(sepol_handle_t * handle, policydb_t * p)
+static int policydb_index_decls(sepol_handle_t * handle, policydb_t * p)
 {
 	avrule_block_t *curblock;
 	avrule_decl_t *decl;
@@ -1426,7 +1426,7 @@ static int range_tr_destroy(hashtab_key_t key, hashtab_datum_t datum,
 	return 0;
 }
 
-void ocontext_selinux_free(ocontext_t **ocontexts)
+static void ocontext_selinux_free(ocontext_t **ocontexts)
 {
 	ocontext_t *c, *ctmp;
 	int i;
@@ -1448,7 +1448,7 @@ void ocontext_selinux_free(ocontext_t **ocontexts)
 	}
 }
 
-void ocontext_xen_free(ocontext_t **ocontexts)
+static void ocontext_xen_free(ocontext_t **ocontexts)
 {
 	ocontext_t *c, *ctmp;
 	int i;
@@ -1747,7 +1747,7 @@ int symtab_insert(policydb_t * pol, uint32_t sym,
 	return retval;
 }
 
-int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
+static int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
 {
 	type_set_init(dst);
 
@@ -2521,7 +2521,7 @@ static int type_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
 	return -1;
 }
 
-int role_trans_read(policydb_t *p, struct policy_file *fp)
+static int role_trans_read(policydb_t *p, struct policy_file *fp)
 {
 	role_trans_t **t = &p->role_tr;
 	unsigned int i;
@@ -2564,7 +2564,7 @@ int role_trans_read(policydb_t *p, struct policy_file *fp)
 	return 0;
 }
 
-int role_allow_read(role_allow_t ** r, struct policy_file *fp)
+static int role_allow_read(role_allow_t ** r, struct policy_file *fp)
 {
 	unsigned int i;
 	uint32_t buf[2], nel;
@@ -2839,7 +2839,7 @@ err:
 	return -1;
 }
 
-int filename_trans_read(policydb_t *p, struct policy_file *fp)
+static int filename_trans_read(policydb_t *p, struct policy_file *fp)
 {
 	unsigned int i;
 	uint32_t buf[1], nel;
diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index 6596431c..39fbd979 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -1024,7 +1024,7 @@ static int context_struct_compute_av(context_struct_t * scontext,
 	return 0;
 }
 
-int sepol_validate_transition(sepol_security_id_t oldsid,
+static int sepol_validate_transition(sepol_security_id_t oldsid,
 				     sepol_security_id_t newsid,
 				     sepol_security_id_t tasksid,
 				     sepol_security_class_t tclass)
-- 
2.32.0

[PATCH 03/23] libsepol: remove unused functions

[Christian Göttsche]

The functions `role_set_get_role`, `sepol_validate_transition` and
`sepol_sidtab_remove` seem to be unused since the initial import.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/policydb.c | 18 ----------------
 libsepol/src/services.c | 47 -----------------------------------------
 libsepol/src/sidtab.c   | 31 ---------------------------
 3 files changed, 96 deletions(-)

diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 3f7ddb11..fc1d0711 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -1791,24 +1791,6 @@ int type_set_or_eq(type_set_t * dst, type_set_t * other)
 	return ret;
 }
 
-int role_set_get_role(role_set_t * x, uint32_t role)
-{
-	if (x->flags & ROLE_STAR)
-		return 1;
-
-	if (ebitmap_get_bit(&x->roles, role - 1)) {
-		if (x->flags & ROLE_COMP)
-			return 0;
-		else
-			return 1;
-	} else {
-		if (x->flags & ROLE_COMP)
-			return 1;
-		else
-			return 0;
-	}
-}
-
 /***********************************************************************/
 /* everything below is for policy reads */
 
diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index 39fbd979..ff91f7d2 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -1024,53 +1024,6 @@ static int context_struct_compute_av(context_struct_t * scontext,
 	return 0;
 }
 
-static int sepol_validate_transition(sepol_security_id_t oldsid,
-				     sepol_security_id_t newsid,
-				     sepol_security_id_t tasksid,
-				     sepol_security_class_t tclass)
-{
-	context_struct_t *ocontext;
-	context_struct_t *ncontext;
-	context_struct_t *tcontext;
-	class_datum_t *tclass_datum;
-	constraint_node_t *constraint;
-
-	if (!tclass || tclass > policydb->p_classes.nprim) {
-		ERR(NULL, "unrecognized class %d", tclass);
-		return -EINVAL;
-	}
-	tclass_datum = policydb->class_val_to_struct[tclass - 1];
-
-	ocontext = sepol_sidtab_search(sidtab, oldsid);
-	if (!ocontext) {
-		ERR(NULL, "unrecognized SID %d", oldsid);
-		return -EINVAL;
-	}
-
-	ncontext = sepol_sidtab_search(sidtab, newsid);
-	if (!ncontext) {
-		ERR(NULL, "unrecognized SID %d", newsid);
-		return -EINVAL;
-	}
-
-	tcontext = sepol_sidtab_search(sidtab, tasksid);
-	if (!tcontext) {
-		ERR(NULL, "unrecognized SID %d", tasksid);
-		return -EINVAL;
-	}
-
-	constraint = tclass_datum->validatetrans;
-	while (constraint) {
-		if (!constraint_expr_eval_reason(ocontext, ncontext, tcontext,
-					  0, constraint, NULL, 0)) {
-			return -EPERM;
-		}
-		constraint = constraint->next;
-	}
-
-	return 0;
-}
-
 /*
  * sepol_validate_transition_reason_buffer - the reason buffer is realloc'd
  * in the constraint_expr_eval_reason() function.
diff --git a/libsepol/src/sidtab.c b/libsepol/src/sidtab.c
index e6bf5716..255e0725 100644
--- a/libsepol/src/sidtab.c
+++ b/libsepol/src/sidtab.c
@@ -84,37 +84,6 @@ int sepol_sidtab_insert(sidtab_t * s, sepol_security_id_t sid,
 	return 0;
 }
 
-int sepol_sidtab_remove(sidtab_t * s, sepol_security_id_t sid)
-{
-	int hvalue;
-	sidtab_node_t *cur, *last;
-
-	if (!s || !s->htable)
-		return -ENOENT;
-
-	hvalue = SIDTAB_HASH(sid);
-	last = NULL;
-	cur = s->htable[hvalue];
-	while (cur != NULL && sid > cur->sid) {
-		last = cur;
-		cur = cur->next;
-	}
-
-	if (cur == NULL || sid != cur->sid)
-		return -ENOENT;
-
-	if (last == NULL)
-		s->htable[hvalue] = cur->next;
-	else
-		last->next = cur->next;
-
-	context_destroy(&cur->context);
-
-	free(cur);
-	s->nel--;
-	return 0;
-}
-
 context_struct_t *sepol_sidtab_search(sidtab_t * s, sepol_security_id_t sid)
 {
 	int hvalue;
-- 
2.32.0

[PATCH 04/23] libsepol: ignore UBSAN false-positives

[Christian Göttsche]

Unsigned integer overflow is well-defined and not undefined behavior.
But it is still useful to enable undefined behavior sanitizer checks on
unsigned arithmetic to detect possible issues on counters or variables
with similar purpose.

Annotate functions in which unsigned overflows are expected to happen.

avtab.c:76:2: runtime error: unsigned integer overflow: 6 * 3432918353 cannot be represented in type 'unsigned int'
policydb.c:795:42: runtime error: unsigned integer overflow: 8160943042179512010 * 11 cannot be represented in type 'unsigned long'
symtab.c:25:12: runtime error: left shift of 1766601759 by 4 places cannot be represented in type 'unsigned int'

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/avtab.c    | 6 ++++++
 libsepol/src/policydb.c | 6 ++++++
 libsepol/src/symtab.c   | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
index 257f051a..c2ccb005 100644
--- a/libsepol/src/avtab.c
+++ b/libsepol/src/avtab.c
@@ -52,6 +52,12 @@
 /* Based on MurmurHash3, written by Austin Appleby and placed in the
  * public domain.
  */
+#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
+__attribute__((no_sanitize("unsigned-integer-overflow")))
+#if (__clang_major__ >= 12)
+__attribute__((no_sanitize("unsigned-shift-base")))
+#endif
+#endif
 static inline int avtab_hash(struct avtab_key *keyp, uint32_t mask)
 {
 	static const uint32_t c1 = 0xcc9e2d51;
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index fc1d0711..cbe0c432 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -789,6 +789,12 @@ static int roles_init(policydb_t * p)
 	goto out;
 }
 
+#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
+__attribute__((no_sanitize("unsigned-integer-overflow")))
+#if (__clang_major__ >= 12)
+__attribute__((no_sanitize("unsigned-shift-base")))
+#endif
+#endif
 static inline unsigned long
 partial_name_hash(unsigned long c, unsigned long prevhash)
 {
diff --git a/libsepol/src/symtab.c b/libsepol/src/symtab.c
index 9a417ca2..738fa0a4 100644
--- a/libsepol/src/symtab.c
+++ b/libsepol/src/symtab.c
@@ -11,6 +11,12 @@
 #include <sepol/policydb/hashtab.h>
 #include <sepol/policydb/symtab.h>
 
+#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
+__attribute__((no_sanitize("unsigned-integer-overflow")))
+#if (__clang_major__ >= 12)
+__attribute__((no_sanitize("unsigned-shift-base")))
+#endif
+#endif
 static unsigned int symhash(hashtab_t h, const_hashtab_key_t key)
 {
 	const char *p, *keyp;
-- 
2.32.0

[PATCH 05/23] libsepol: avoid implicit conversions

[Christian Göttsche]

Avoid implicit conversions from signed to unsigned values, found by
UB sanitizers, by using unsigned values in the first place.

expand.c:1644:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)

expand.c:2892:24: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned int' changed the value to 4294967294 (32-bit, unsigned)

policy_define.c:2344:4: runtime error: implicit conversion from type 'int' of value -1048577 (32-bit, signed) to type 'unsigned int' changed the value to 4293918719 (32-bit, unsigned)

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/include/sepol/policydb/conditional.h | 2 +-
 libsepol/include/sepol/policydb/policydb.h    | 2 +-
 libsepol/src/expand.c                         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libsepol/include/sepol/policydb/conditional.h b/libsepol/include/sepol/policydb/conditional.h
index 9c3df3ef..db3ef98d 100644
--- a/libsepol/include/sepol/policydb/conditional.h
+++ b/libsepol/include/sepol/policydb/conditional.h
@@ -90,7 +90,7 @@ typedef struct cond_node {
 	uint32_t expr_pre_comp;
 	struct cond_node *next;
 	/* a tunable conditional, calculated and used at expansion */
-#define	COND_NODE_FLAGS_TUNABLE	0x01
+#define	COND_NODE_FLAGS_TUNABLE	0x01U
 	uint32_t flags;
 } cond_node_t;
 
diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index 9ef43abc..c29339dc 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -253,7 +253,7 @@ typedef struct class_perm_node {
 
 #define xperm_test(x, p) (1 & (p[x >> 5] >> (x & 0x1f)))
 #define xperm_set(x, p) (p[x >> 5] |= (1 << (x & 0x1f)))
-#define xperm_clear(x, p) (p[x >> 5] &= ~(1 << (x & 0x1f)))
+#define xperm_clear(x, p) (p[x >> 5] &= ~(1U << (x & 0x1f)))
 #define EXTENDED_PERMS_LEN 8
 
 typedef struct av_extended_perms {
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 84bfcfa3..35e45780 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1641,7 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
 		 * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
 		 * others. Initialize the data accordingly.
 		 */
-		avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
+		avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0U : 0U;
 		/* this is used to get the node - insertion is actually unique */
 		node = avtab_insert_nonunique(avtab, key, &avdatum);
 		if (!node) {
-- 
2.32.0

[PATCH 06/23] libsepol: avoid unsigned integer overflow

[Christian Göttsche]

Unsigned integer overflow is well-defined and not undefined behavior.
But it is still useful to enable undefined behavior sanitizer checks on
unsigned arithmetic to detect possible issues on counters or variables
with similar purpose.

Use a spaceship operator like comparison instead of subtraction.

Modern compilers will generate a single comparison instruction instead
of actually perform the subtraction.

policydb.c:826:17: runtime error: unsigned integer overflow: 24 - 1699 cannot be represented in type 'unsigned int'

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

diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index cbe0c432..3389a943 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -823,11 +823,11 @@ static int filenametr_cmp(hashtab_t h __attribute__ ((unused)),
 	const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2;
 	int v;
 
-	v = ft1->ttype - ft2->ttype;
+	v = (ft1->ttype > ft2->ttype) - (ft1->ttype < ft2->ttype);
 	if (v)
 		return v;
 
-	v = ft1->tclass - ft2->tclass;
+	v = (ft1->tclass > ft2->tclass) - (ft1->tclass < ft2->tclass);
 	if (v)
 		return v;
 
-- 
2.32.0

[PATCH 07/23] libsepol: follow declaration-after-statement

[Christian Göttsche]

Follow the project style of no declaration after statement.

Found by the gcc warning -Wdeclaration-after-statement

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/booleans.c      |  6 ++--
 libsepol/src/debug.c         |  2 +-
 libsepol/src/ebitmap.c       | 11 ++++---
 libsepol/src/module_to_cil.c | 10 +++---
 libsepol/src/nodes.c         |  6 ++--
 libsepol/src/services.c      | 59 ++++++++++++++++++------------------
 libsepol/src/util.c          |  2 +-
 7 files changed, 50 insertions(+), 46 deletions(-)

diff --git a/libsepol/src/booleans.c b/libsepol/src/booleans.c
index 30fcf29d..716da6b4 100644
--- a/libsepol/src/booleans.c
+++ b/libsepol/src/booleans.c
@@ -19,6 +19,7 @@ static int bool_update(sepol_handle_t * handle,
 	const char *cname;
 	char *name;
 	int value;
+	cond_bool_datum_t *datum;
 
 	sepol_bool_key_unpack(key, &cname);
 	name = strdup(cname);
@@ -27,8 +28,7 @@ static int bool_update(sepol_handle_t * handle,
 	if (!name)
 		goto omem;
 
-	cond_bool_datum_t *datum =
-	    hashtab_search(policydb->p_bools.table, name);
+	datum = hashtab_search(policydb->p_bools.table, name);
 	if (!datum) {
 		ERR(handle, "boolean %s no longer in policy", name);
 		goto err;
@@ -84,10 +84,10 @@ int sepol_bool_set(sepol_handle_t * handle,
 		   const sepol_bool_key_t * key, const sepol_bool_t * data)
 {
 
+	policydb_t *policydb = &p->p;
 	const char *name;
 	sepol_bool_key_unpack(key, &name);
 
-	policydb_t *policydb = &p->p;
 	if (bool_update(handle, policydb, key, data) < 0)
 		goto err;
 
diff --git a/libsepol/src/debug.c b/libsepol/src/debug.c
index 0458e353..f6a59ae7 100644
--- a/libsepol/src/debug.c
+++ b/libsepol/src/debug.c
@@ -44,6 +44,7 @@ void sepol_msg_default_handler(void *varg __attribute__ ((unused)),
 {
 
 	FILE *stream = NULL;
+	va_list ap;
 
 	switch (sepol_msg_get_level(handle)) {
 
@@ -60,7 +61,6 @@ void sepol_msg_default_handler(void *varg __attribute__ ((unused)),
 	fprintf(stream, "%s.%s: ",
 		sepol_msg_get_channel(handle), sepol_msg_get_fname(handle));
 
-	va_list ap;
 	va_start(ap, fmt);
 	vfprintf(stream, fmt, ap);
 	va_end(ap);
diff --git a/libsepol/src/ebitmap.c b/libsepol/src/ebitmap.c
index 7f425349..522e14a6 100644
--- a/libsepol/src/ebitmap.c
+++ b/libsepol/src/ebitmap.c
@@ -113,9 +113,10 @@ int ebitmap_not(ebitmap_t *dst, ebitmap_t *e1, unsigned int maxbit)
 
 int ebitmap_andnot(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2, unsigned int maxbit)
 {
+	int rc;
 	ebitmap_t e3;
 	ebitmap_init(dst);
-	int rc = ebitmap_not(&e3, e2, maxbit);
+	rc = ebitmap_not(&e3, e2, maxbit);
 	if (rc < 0)
 		return rc;
 	rc = ebitmap_and(dst, e1, &e3);
@@ -138,13 +139,15 @@ unsigned int ebitmap_cardinality(ebitmap_t *e1)
 
 int ebitmap_hamming_distance(ebitmap_t * e1, ebitmap_t * e2)
 {
+	int rc;
+	ebitmap_t tmp;
+	int distance;
 	if (ebitmap_cmp(e1, e2))
 		return 0;
-	ebitmap_t tmp;
-	int rc = ebitmap_xor(&tmp, e1, e2);
+	rc = ebitmap_xor(&tmp, e1, e2);
 	if (rc < 0)
 		return -1;
-	int distance = ebitmap_cardinality(&tmp);
+	distance = ebitmap_cardinality(&tmp);
 	ebitmap_destroy(&tmp);
 	return distance;
 }
diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 41605eb8..73ec7971 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -107,8 +107,8 @@ static void cil_printf(const char *fmt, ...) {
 __attribute__ ((format(printf, 2, 3)))
 static void cil_println(int indent, const char *fmt, ...)
 {
-	cil_indent(indent);
 	va_list argptr;
+	cil_indent(indent);
 	va_start(argptr, fmt);
 	if (vfprintf(out_file, fmt, argptr) < 0) {
 		log_err("Failed to write to output");
@@ -235,12 +235,14 @@ static void role_list_destroy(void)
 
 static void attr_list_destroy(struct list **attr_list)
 {
+	struct list_node *curr;
+	struct attr_list_node *attr;
+
 	if (attr_list == NULL || *attr_list == NULL) {
 		return;
 	}
 
-	struct list_node *curr = (*attr_list)->head;
-	struct attr_list_node *attr;
+	curr = (*attr_list)->head;
 
 	while (curr != NULL) {
 		attr = curr->data;
@@ -3525,12 +3527,12 @@ exit:
 static int additive_scopes_to_cil(int indent, struct policydb *pdb, struct avrule_block *block, struct stack *decl_stack)
 {
 	int rc = -1;
+	struct avrule_decl *decl = stack_peek(decl_stack);
 	struct map_args args;
 	args.pdb = pdb;
 	args.block = block;
 	args.decl_stack = decl_stack;
 	args.indent = indent;
-	struct avrule_decl *decl = stack_peek(decl_stack);
 
 	for (args.sym_index = 0; args.sym_index < SYM_NUM; args.sym_index++) {
 		if (func_to_cil[args.sym_index] == NULL) {
diff --git a/libsepol/src/nodes.c b/libsepol/src/nodes.c
index 820346d0..97a0f959 100644
--- a/libsepol/src/nodes.c
+++ b/libsepol/src/nodes.c
@@ -19,20 +19,20 @@ static int node_from_record(sepol_handle_t * handle,
 	ocontext_t *tmp_node = NULL;
 	context_struct_t *tmp_con = NULL;
 	char *addr_buf = NULL, *mask_buf = NULL;
+	size_t addr_bsize, mask_bsize;
+	int proto;
 
 	tmp_node = (ocontext_t *) calloc(1, sizeof(ocontext_t));
 	if (!tmp_node)
 		goto omem;
 
-	size_t addr_bsize, mask_bsize;
-
 	/* Address and netmask */
 	if (sepol_node_get_addr_bytes(handle, data, &addr_buf, &addr_bsize) < 0)
 		goto err;
 	if (sepol_node_get_mask_bytes(handle, data, &mask_buf, &mask_bsize) < 0)
 		goto err;
 
-	int proto = sepol_node_get_proto(data);
+	proto = sepol_node_get_proto(data);
 
 	switch (proto) {
 	case SEPOL_PROTO_IP4:
diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index ff91f7d2..d647c8f5 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -290,6 +290,19 @@ static char *get_class_info(sepol_security_class_t tclass,
 {
 	constraint_expr_t *e;
 	int mls, state_num;
+	/* Determine statement type */
+	const char *statements[] = {
+		"constrain ",			/* 0 */
+		"mlsconstrain ",		/* 1 */
+		"validatetrans ",		/* 2 */
+		"mlsvalidatetrans ",	/* 3 */
+		0 };
+	size_t class_buf_len = 0;
+	size_t new_class_buf_len;
+	size_t buf_used;
+	int len;
+	char *class_buf = NULL, *p;
+	char *new_class_buf = NULL;
 
 	/* Find if MLS statement or not */
 	mls = 0;
@@ -300,26 +313,11 @@ static char *get_class_info(sepol_security_class_t tclass,
 		}
 	}
 
-	/* Determine statement type */
-	const char *statements[] = {
-		"constrain ",			/* 0 */
-		"mlsconstrain ",		/* 1 */
-		"validatetrans ",		/* 2 */
-		"mlsvalidatetrans ",	/* 3 */
-		0 };
-
 	if (xcontext == NULL)
 		state_num = mls + 0;
 	else
 		state_num = mls + 2;
 
-	size_t class_buf_len = 0;
-	size_t new_class_buf_len;
-	size_t buf_used;
-	int len;
-	char *class_buf = NULL, *p;
-	char *new_class_buf = NULL;
-
 	while (1) {
 		new_class_buf_len = class_buf_len + EXPR_BUF_SIZE;
 		new_class_buf = realloc(class_buf, new_class_buf_len);
@@ -417,6 +415,8 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
 	char *tgt = NULL;
 	int rc = 0, x;
 	char *class_buf = NULL;
+	int expr_list_len = 0;
+	int expr_count;
 
 	/*
 	 * The array of expression answer buffer pointers and counter.
@@ -424,6 +424,11 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
 	char **answer_list = NULL;
 	int answer_counter = 0;
 
+	/* The pop operands */
+	char *a;
+	char *b;
+	int a_len, b_len;
+
 	class_buf = get_class_info(tclass, constraint, xcontext);
 	if (!class_buf) {
 		ERR(NULL, "failed to allocate class buffer");
@@ -431,7 +436,6 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
 	}
 
 	/* Original function but with buffer support */
-	int expr_list_len = 0;
 	expr_counter = 0;
 	expr_list = NULL;
 	for (e = constraint->expr; e; e = e->next) {
@@ -701,7 +705,7 @@ mls_ops:
 	 * expr_list malloc's. Normally they are released by the RPN to
 	 * infix code.
 	 */
-	int expr_count = expr_counter;
+	expr_count = expr_counter;
 	expr_counter = 0;
 
 	/*
@@ -715,11 +719,6 @@ mls_ops:
 		goto out;
 	}
 
-	/* The pop operands */
-	char *a;
-	char *b;
-	int a_len, b_len;
-
 	/* Convert constraint from RPN to infix notation. */
 	for (x = 0; x != expr_count; x++) {
 		if (strncmp(expr_list[x], "and", 3) == 0 || strncmp(expr_list[x],
@@ -778,14 +777,6 @@ mls_ops:
 			xcontext ? "Validatetrans" : "Constraint",
 			s[0] ? "GRANTED" : "DENIED");
 
-	int len, new_buf_len;
-	char *p, **new_buf = r_buf;
-	/*
-	 * These contain the constraint components that are added to the
-	 * callers reason buffer.
-	 */
-	const char *buffers[] = { class_buf, a, "); ", tmp_buf, 0 };
-
 	/*
 	 * This will add the constraints to the callers reason buffer (who is
 	 * responsible for freeing the memory). It will handle any realloc's
@@ -796,6 +787,14 @@ mls_ops:
 
 	if (r_buf && ((s[0] == 0) || ((s[0] == 1 &&
 				(flags & SHOW_GRANTED) == SHOW_GRANTED)))) {
+		int len, new_buf_len;
+		char *p, **new_buf = r_buf;
+		/*
+		* These contain the constraint components that are added to the
+		* callers reason buffer.
+		*/
+		const char *buffers[] = { class_buf, a, "); ", tmp_buf, 0 };
+
 		for (x = 0; buffers[x] != NULL; x++) {
 			while (1) {
 				p = *r_buf + reason_buf_used;
diff --git a/libsepol/src/util.c b/libsepol/src/util.c
index d51750af..a47cae87 100644
--- a/libsepol/src/util.c
+++ b/libsepol/src/util.c
@@ -129,9 +129,9 @@ char *sepol_extended_perms_to_string(avtab_extended_perms_t *xperms)
 	unsigned int bit;
 	unsigned int in_range = 0;
 	static char xpermsbuf[2048];
-	xpermsbuf[0] = '\0';
 	char *p;
 	int len, xpermslen = 0;
+	xpermsbuf[0] = '\0';
 	p = xpermsbuf;
 
 	if ((xperms->specified != AVTAB_XPERMS_IOCTLFUNCTION)
-- 
2.32.0

[PATCH 08/23] libsepol/cil: follow declaration-after-statement

[Christian Göttsche]

Follow the project style of no declaration after statement.

Found by the gcc warning -Wdeclaration-after-statement

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/cil/src/cil_binary.c      | 5 +++--
 libsepol/cil/src/cil_build_ast.c   | 5 +++--
 libsepol/cil/src/cil_fqn.c         | 3 ++-
 libsepol/cil/src/cil_list.c        | 7 ++++---
 libsepol/cil/src/cil_post.c        | 2 +-
 libsepol/cil/src/cil_resolve_ast.c | 6 +++---
 libsepol/cil/src/cil_strpool.c     | 3 ++-
 7 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 18532aad..85094b01 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -593,11 +593,11 @@ exit:
 int __cil_typeattr_bitmap_init(policydb_t *pdb)
 {
 	int rc = SEPOL_ERR;
+	uint32_t i;
 
 	pdb->type_attr_map = cil_malloc(pdb->p_types.nprim * sizeof(ebitmap_t));
 	pdb->attr_type_map = cil_malloc(pdb->p_types.nprim * sizeof(ebitmap_t));
 
-	uint32_t i = 0;
 	for (i = 0; i < pdb->p_types.nprim; i++) {
 		ebitmap_init(&pdb->type_attr_map[i]);
 		ebitmap_init(&pdb->attr_type_map[i]);
@@ -2657,6 +2657,7 @@ int __cil_constrain_expr_to_sepol_expr_helper(policydb_t *pdb, const struct cil_
 	int rc = SEPOL_ERR;
 	struct cil_list_item *item;
 	enum cil_flavor flavor;
+	enum cil_flavor cil_op;
 	constraint_expr_t *op, *h1, *h2, *t1, *t2;
 	int is_leaf = CIL_FALSE;
 
@@ -2673,7 +2674,7 @@ int __cil_constrain_expr_to_sepol_expr_helper(policydb_t *pdb, const struct cil_
 		goto exit;
 	}
 
-	enum cil_flavor cil_op = (enum cil_flavor)(uintptr_t)item->data;
+	cil_op = (enum cil_flavor)(uintptr_t)item->data;
 	switch (cil_op) {
 	case CIL_NOT:
 		op->expr_type = CEXPR_NOT;
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 42d10c87..9a9bc598 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -5173,6 +5173,7 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
 	char *key = NULL;
 	struct cil_macro *macro = NULL;
 	struct cil_tree_node *macro_content = NULL;
+	struct cil_tree_node *current_item;
 	enum cil_syntax syntax[] = {
 		CIL_SYN_STRING,
 		CIL_SYN_STRING,
@@ -5195,7 +5196,7 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
 
 	key = parse_current->next->data;
 
-	struct cil_tree_node *current_item = parse_current->next->next->cl_head;
+	current_item = parse_current->next->next->cl_head;
 	while (current_item != NULL) {
 		enum cil_syntax param_syntax[] = {
 			CIL_SYN_STRING,
@@ -5205,6 +5206,7 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
 		int param_syntax_len = sizeof(param_syntax)/sizeof(*param_syntax);
 		char *kind = NULL;
 		struct cil_param *param = NULL;
+		struct cil_list_item *curr_param;
 
 		rc =__cil_verify_syntax(current_item->cl_head, param_syntax, param_syntax_len);
 		if (rc != SEPOL_OK) {
@@ -5263,7 +5265,6 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
 		}
 
 		//walk current list and check for duplicate parameters
-		struct cil_list_item *curr_param;
 		cil_list_for_each(curr_param, macro->params) {
 			if (param->str == ((struct cil_param*)curr_param->data)->str) {
 				cil_log(CIL_ERR, "Duplicate parameter\n");
diff --git a/libsepol/cil/src/cil_fqn.c b/libsepol/cil/src/cil_fqn.c
index 097222a8..46db069b 100644
--- a/libsepol/cil/src/cil_fqn.c
+++ b/libsepol/cil/src/cil_fqn.c
@@ -78,12 +78,13 @@ static int __cil_fqn_qualify_blocks(__attribute__((unused)) hashtab_key_t k, has
 	struct cil_tree_node *node = NODE(datum);
 	int i;
 	int rc = SEPOL_OK;
+	int newlen;
 
 	if (node->flavor != CIL_BLOCK) {
 		goto exit;
 	}
 
-	int newlen = fqn_args->len + strlen(datum->name) + 1;
+	newlen = fqn_args->len + strlen(datum->name) + 1;
 	if (newlen >= CIL_MAX_NAME_LENGTH) {
 		cil_log(CIL_INFO, "Fully qualified name for block %s is too long\n", datum->name);
 		rc = SEPOL_ERR;
diff --git a/libsepol/cil/src/cil_list.c b/libsepol/cil/src/cil_list.c
index 4e7843cb..8a426f1f 100644
--- a/libsepol/cil/src/cil_list.c
+++ b/libsepol/cil/src/cil_list.c
@@ -55,15 +55,16 @@ void cil_list_init(struct cil_list **list, enum cil_flavor flavor)
 
 void cil_list_destroy(struct cil_list **list, unsigned destroy_data)
 {
+	struct cil_list_item *item;
+
 	if (*list == NULL) {
 		return;
 	}
 
-	struct cil_list_item *item = (*list)->head;
-	struct cil_list_item *next = NULL;
+	item = (*list)->head;
 	while (item != NULL)
 	{
-		next = item->next;
+		struct cil_list_item *next = item->next;
 		if (item->flavor == CIL_LIST) {
 			cil_list_destroy((struct cil_list**)&(item->data), destroy_data);
 			free(item);
diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index 05842b64..7bca0834 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -213,8 +213,8 @@ int cil_post_filecon_compare(const void *a, const void *b)
 	struct fc_data *a_data = cil_malloc(sizeof(*a_data));
 	struct fc_data *b_data = cil_malloc(sizeof(*b_data));
 	char *a_path = cil_malloc(strlen(a_filecon->path_str) + 1);
-	a_path[0] = '\0';
 	char *b_path = cil_malloc(strlen(b_filecon->path_str) + 1);
+	a_path[0] = '\0';
 	b_path[0] = '\0';
 	strcat(a_path, a_filecon->path_str);
 	strcat(b_path, b_filecon->path_str);
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index d8481002..a322b1b7 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3949,10 +3949,10 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 		enum cil_log_level lvl = CIL_ERR;
 
 		if (optional != NULL) {
-			lvl = CIL_INFO;
-
 			struct cil_optional *opt = (struct cil_optional *)optional->data;
-			struct cil_tree_node *opt_node = NODE(opt);;
+			struct cil_tree_node *opt_node = NODE(opt);
+
+			lvl = CIL_INFO;
 			/* disable an optional if something failed to resolve */
 			opt->enabled = CIL_FALSE;
 			cil_tree_log(node, lvl, "Failed to resolve %s statement", cil_node_to_string(node));
diff --git a/libsepol/cil/src/cil_strpool.c b/libsepol/cil/src/cil_strpool.c
index 2598bbf3..70bca363 100644
--- a/libsepol/cil/src/cil_strpool.c
+++ b/libsepol/cil/src/cil_strpool.c
@@ -75,9 +75,10 @@ char *cil_strpool_add(const char *str)
 
 	strpool_ref = hashtab_search(cil_strpool_tab, (hashtab_key_t)str);
 	if (strpool_ref == NULL) {
+		int rc;
 		strpool_ref = cil_malloc(sizeof(*strpool_ref));
 		strpool_ref->str = cil_strdup(str);
-		int rc = hashtab_insert(cil_strpool_tab, (hashtab_key_t)strpool_ref->str, strpool_ref);
+		rc = hashtab_insert(cil_strpool_tab, (hashtab_key_t)strpool_ref->str, strpool_ref);
 		if (rc != SEPOL_OK) {
 			pthread_mutex_unlock(&cil_strpool_mutex);
 			cil_log(CIL_ERR, "Failed to allocate memory\n");
-- 
2.32.0

[PATCH 09/23] libsepol: remove dead stores

[Christian Göttsche]

conditional.c:391:4: warning: Value stored to 'i' is never read [deadcode.DeadStores]
                        i = 0;
                        ^   ~
conditional.c:718:2: warning: Value stored to 'len' is never read [deadcode.DeadStores]
        len = 0;
        ^     ~
conditional.c:772:2: warning: Value stored to 'len' is never read [deadcode.DeadStores]
        len = 0;
        ^     ~

services.c:89:10: warning: Value stored to 'new_stack' during its initialization is never read [deadcode.DeadStores]
                char **new_stack = stack;
                       ^~~~~~~~~   ~~~~~

services.c:440:11: warning: Value stored to 'new_expr_list' during its initialization is never read [deadcode.DeadStores]
                        char **new_expr_list = expr_list;
                               ^~~~~~~~~~~~~   ~~~~~~~~~

../cil/src/cil_binary.c:2230:24: warning: Value stored to 'cb_node' during its initialization is never read [deadcode.DeadStores]
        struct cil_tree_node *cb_node = node->cl_head;
                              ^~~~~~~   ~~~~~~~~~~~~~

Found by clang-analyzer

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/conditional.c | 3 ---
 libsepol/src/services.c    | 4 ++--
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/libsepol/src/conditional.c b/libsepol/src/conditional.c
index 823b649a..e3ede694 100644
--- a/libsepol/src/conditional.c
+++ b/libsepol/src/conditional.c
@@ -388,7 +388,6 @@ int cond_normalize_expr(policydb_t * p, cond_node_t * cn)
 	for (e = cn->expr; e != NULL; e = e->next) {
 		switch (e->expr_type) {
 		case COND_BOOL:
-			i = 0;
 			/* see if we've already seen this bool */
 			if (!bool_present(e->bool, cn->bool_ids, cn->nbools)) {
 				/* count em all but only record up to COND_MAX_BOOLS */
@@ -715,7 +714,6 @@ static int cond_read_av_list(policydb_t * p, void *fp,
 
 	*ret_list = NULL;
 
-	len = 0;
 	rc = next_entry(buf, fp, sizeof(uint32_t));
 	if (rc < 0)
 		return -1;
@@ -769,7 +767,6 @@ static int cond_read_node(policydb_t * p, cond_node_t * node, void *fp)
 
 	node->cur_state = le32_to_cpu(buf[0]);
 
-	len = 0;
 	rc = next_entry(buf, fp, sizeof(uint32_t));
 	if (rc < 0)
 		goto err;
diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index d647c8f5..c34bb966 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -86,7 +86,7 @@ static int next_stack_entry;
 static void push(char *expr_ptr)
 {
 	if (next_stack_entry >= stack_len) {
-		char **new_stack = stack;
+		char **new_stack;
 		int new_stack_len;
 
 		if (stack_len == 0)
@@ -441,7 +441,7 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
 	for (e = constraint->expr; e; e = e->next) {
 		/* Allocate a stack to hold expression buffer entries */
 		if (expr_counter >= expr_list_len) {
-			char **new_expr_list = expr_list;
+			char **new_expr_list;
 			int new_expr_list_len;
 
 			if (expr_list_len == 0)
-- 
2.32.0

[PATCH 10/23] libsepol: mark read-only parameters of ebitmap interfaces const

[Christian Göttsche]

Make it more obvious which parameters are read-only and not being
modified and allow callers to pass const pointers.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/include/sepol/policydb/ebitmap.h | 16 ++++++++--------
 libsepol/src/ebitmap.c                    | 18 +++++++++---------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/libsepol/include/sepol/policydb/ebitmap.h b/libsepol/include/sepol/policydb/ebitmap.h
index 634436f6..81d0c7a6 100644
--- a/libsepol/include/sepol/policydb/ebitmap.h
+++ b/libsepol/include/sepol/policydb/ebitmap.h
@@ -67,7 +67,7 @@ static inline unsigned int ebitmap_next(ebitmap_node_t ** n, unsigned int bit)
 	return (bit + 1);
 }
 
-static inline int ebitmap_node_get_bit(ebitmap_node_t * n, unsigned int bit)
+static inline int ebitmap_node_get_bit(const ebitmap_node_t * n, unsigned int bit)
 {
 	if (n->map & (MAPBIT << (bit - n->startbit)))
 		return 1;
@@ -83,18 +83,18 @@ static inline int ebitmap_node_get_bit(ebitmap_node_t * n, unsigned int bit)
 extern int ebitmap_cmp(const ebitmap_t * e1, const ebitmap_t * e2);
 extern int ebitmap_or(ebitmap_t * dst, const ebitmap_t * e1, const ebitmap_t * e2);
 extern int ebitmap_union(ebitmap_t * dst, const ebitmap_t * e1);
-extern int ebitmap_and(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2);
-extern int ebitmap_xor(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2);
-extern int ebitmap_not(ebitmap_t *dst, ebitmap_t *e1, unsigned int maxbit);
-extern int ebitmap_andnot(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2, unsigned int maxbit);
-extern unsigned int ebitmap_cardinality(ebitmap_t *e1);
-extern int ebitmap_hamming_distance(ebitmap_t * e1, ebitmap_t * e2);
+extern int ebitmap_and(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2);
+extern int ebitmap_xor(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2);
+extern int ebitmap_not(ebitmap_t *dst, const ebitmap_t *e1, unsigned int maxbit);
+extern int ebitmap_andnot(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2, unsigned int maxbit);
+extern unsigned int ebitmap_cardinality(const ebitmap_t *e1);
+extern int ebitmap_hamming_distance(const ebitmap_t * e1, const ebitmap_t * e2);
 extern int ebitmap_cpy(ebitmap_t * dst, const ebitmap_t * src);
 extern int ebitmap_contains(const ebitmap_t * e1, const ebitmap_t * e2);
 extern int ebitmap_match_any(const ebitmap_t *e1, const ebitmap_t *e2);
 extern int ebitmap_get_bit(const ebitmap_t * e, unsigned int bit);
 extern int ebitmap_set_bit(ebitmap_t * e, unsigned int bit, int value);
-extern unsigned int ebitmap_highest_set_bit(ebitmap_t * e);
+extern unsigned int ebitmap_highest_set_bit(const ebitmap_t * e);
 extern void ebitmap_destroy(ebitmap_t * e);
 extern int ebitmap_read(ebitmap_t * e, void *fp);
 
diff --git a/libsepol/src/ebitmap.c b/libsepol/src/ebitmap.c
index 522e14a6..4e9acdf8 100644
--- a/libsepol/src/ebitmap.c
+++ b/libsepol/src/ebitmap.c
@@ -71,7 +71,7 @@ int ebitmap_union(ebitmap_t * dst, const ebitmap_t * e1)
 	return 0;
 }
 
-int ebitmap_and(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2)
+int ebitmap_and(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2)
 {
 	unsigned int i, length = min(ebitmap_length(e1), ebitmap_length(e2));
 	ebitmap_init(dst);
@@ -85,7 +85,7 @@ int ebitmap_and(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2)
 	return 0;
 }
 
-int ebitmap_xor(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2)
+int ebitmap_xor(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2)
 {
 	unsigned int i, length = max(ebitmap_length(e1), ebitmap_length(e2));
 	ebitmap_init(dst);
@@ -98,7 +98,7 @@ int ebitmap_xor(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2)
 	return 0;
 }
 
-int ebitmap_not(ebitmap_t *dst, ebitmap_t *e1, unsigned int maxbit)
+int ebitmap_not(ebitmap_t *dst, const ebitmap_t *e1, unsigned int maxbit)
 {
 	unsigned int i;
 	ebitmap_init(dst);
@@ -111,7 +111,7 @@ int ebitmap_not(ebitmap_t *dst, ebitmap_t *e1, unsigned int maxbit)
 	return 0;
 }
 
-int ebitmap_andnot(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2, unsigned int maxbit)
+int ebitmap_andnot(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2, unsigned int maxbit)
 {
 	int rc;
 	ebitmap_t e3;
@@ -126,10 +126,10 @@ int ebitmap_andnot(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2, unsigned int ma
 	return 0;
 }
 
-unsigned int ebitmap_cardinality(ebitmap_t *e1)
+unsigned int ebitmap_cardinality(const ebitmap_t *e1)
 {
 	unsigned int count = 0;
-	ebitmap_node_t *n;
+	const ebitmap_node_t *n;
 
 	for (n = e1->node; n; n = n->next) {
 		count += __builtin_popcountll(n->map);
@@ -137,7 +137,7 @@ unsigned int ebitmap_cardinality(ebitmap_t *e1)
 	return count;
 }
 
-int ebitmap_hamming_distance(ebitmap_t * e1, ebitmap_t * e2)
+int ebitmap_hamming_distance(const ebitmap_t * e1, const ebitmap_t * e2)
 {
 	int rc;
 	ebitmap_t tmp;
@@ -347,9 +347,9 @@ int ebitmap_set_bit(ebitmap_t * e, unsigned int bit, int value)
 	return 0;
 }
 
-unsigned int ebitmap_highest_set_bit(ebitmap_t * e)
+unsigned int ebitmap_highest_set_bit(const ebitmap_t * e)
 {
-	ebitmap_node_t *n;
+	const ebitmap_node_t *n;
 	MAPTYPE map;
 	unsigned int pos = 0;
 
-- 
2.32.0

[PATCH 11/23] libsepol: mark read-only parameters of type_set_ interfaces const

[Christian Göttsche]

Make it more obvious which parameters are read-only and not being
modified and allow callers to pass const pointers.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/include/sepol/policydb/policydb.h | 4 ++--
 libsepol/src/policydb.c                    | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index c29339dc..78699fb4 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -667,8 +667,8 @@ extern int scope_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p);
 extern void class_perm_node_init(class_perm_node_t * x);
 extern void type_set_init(type_set_t * x);
 extern void type_set_destroy(type_set_t * x);
-extern int type_set_cpy(type_set_t * dst, type_set_t * src);
-extern int type_set_or_eq(type_set_t * dst, type_set_t * other);
+extern int type_set_cpy(type_set_t * dst, const type_set_t * src);
+extern int type_set_or_eq(type_set_t * dst, const type_set_t * other);
 extern void role_set_init(role_set_t * x);
 extern void role_set_destroy(role_set_t * x);
 extern void avrule_init(avrule_t * x);
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 3389a943..7739b0fb 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -1753,7 +1753,7 @@ int symtab_insert(policydb_t * pol, uint32_t sym,
 	return retval;
 }
 
-static int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
+static int type_set_or(type_set_t * dst, const type_set_t * a, const type_set_t * b)
 {
 	type_set_init(dst);
 
@@ -1770,7 +1770,7 @@ static int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
 	return 0;
 }
 
-int type_set_cpy(type_set_t * dst, type_set_t * src)
+int type_set_cpy(type_set_t * dst, const type_set_t * src)
 {
 	type_set_init(dst);
 
@@ -1783,7 +1783,7 @@ int type_set_cpy(type_set_t * dst, type_set_t * src)
 	return 0;
 }
 
-int type_set_or_eq(type_set_t * dst, type_set_t * other)
+int type_set_or_eq(type_set_t * dst, const type_set_t * other)
 {
 	int ret;
 	type_set_t tmp;
-- 
2.32.0

[PATCH 12/23] libsepol: do not allocate memory of size 0

[Christian Göttsche]

In case cats_ebitmap_len() returns 0, do not allocate but quit.

Found by clang-analyzer

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/kernel_to_cil.c  | 5 ++++-
 libsepol/src/kernel_to_conf.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index 989aacde..17b5ebf0 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -1034,11 +1034,14 @@ static char *cats_ebitmap_to_str(struct ebitmap *cats, char **val_to_name)
 {
 	struct ebitmap_node *node;
 	uint32_t i, start, range;
-	char *catsbuf, *p;
+	char *catsbuf = NULL, *p;
 	const char *fmt;
 	int len, remaining;
 
 	remaining = (int)cats_ebitmap_len(cats, val_to_name);
+	if (remaining == 0) {
+		goto exit;
+	}
 	catsbuf = malloc(remaining);
 	if (!catsbuf) {
 		goto exit;
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 5db47fe4..c1253820 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -1025,12 +1025,15 @@ static char *cats_ebitmap_to_str(struct ebitmap *cats, char **val_to_name)
 {
 	struct ebitmap_node *node;
 	uint32_t i, start, range, first;
-	char *catsbuf, *p;
+	char *catsbuf = NULL, *p;
 	const char *fmt;
 	char sep;
 	int len, remaining;
 
 	remaining = (int)cats_ebitmap_len(cats, val_to_name);
+	if (remaining == 0) {
+		goto exit;
+	}
 	catsbuf = malloc(remaining);
 	if (!catsbuf) {
 		goto exit;
-- 
2.32.0

[PATCH 13/23] libsepol: assure string NUL-termination

[Christian Göttsche]

ibendport_record.c: In function ‘sepol_ibendport_get_ibdev_name’:
ibendport_record.c:169:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
  169 |  strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ibendport_record.c: In function ‘sepol_ibendport_set_ibdev_name’:
ibendport_record.c:189:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
  189 |  strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

strncpy(3) does not NUL-terminate the destination if the source is of
the same length or longer then the specified size.
Reduce the size to copy by 1.

Found by Clang

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

diff --git a/libsepol/src/ibendport_record.c b/libsepol/src/ibendport_record.c
index adf67161..2eb8ca18 100644
--- a/libsepol/src/ibendport_record.c
+++ b/libsepol/src/ibendport_record.c
@@ -166,7 +166,7 @@ int sepol_ibendport_get_ibdev_name(sepol_handle_t *handle,
 	if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_ibdev_name) < 0)
 		goto err;
 
-	strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
+	strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
 	*ibdev_name = tmp_ibdev_name;
 	return STATUS_SUCCESS;
 
@@ -186,7 +186,7 @@ int sepol_ibendport_set_ibdev_name(sepol_handle_t *handle,
 	if (sepol_ibendport_alloc_ibdev_name(handle, &tmp) < 0)
 		goto err;
 
-	strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
+	strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX - 1);
 	free(ibendport->ibdev_name);
 	ibendport->ibdev_name = tmp;
 	return STATUS_SUCCESS;
-- 
2.32.0

[PATCH 14/23] libsepol: remove dead stores

[Christian Göttsche]

Found by Infer

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

diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index c34bb966..f7c31d80 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -175,7 +175,7 @@ static int expr_buf_len;
 static void cat_expr_buf(char *e_buf, const char *string)
 {
 	int len, new_buf_len;
-	char *p, *new_buf = e_buf;
+	char *p, *new_buf;
 
 	while (1) {
 		p = e_buf + expr_buf_used;
@@ -406,7 +406,7 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
 #define TARGET  2
 #define XTARGET 3
 
-	int s_t_x_num = SOURCE;
+	int s_t_x_num;
 
 	/* Set 0 = fail, u = CEXPR_USER, r = CEXPR_ROLE, t = CEXPR_TYPE */
 	int u_r_t = 0;
-- 
2.32.0

[PATCH 15/23] libsepol/cil: silence cast warning

[Christian Göttsche]

../cil/src/cil_write_ast.c:86:32: error: cast to smaller integer type 'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
                        enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../cil/src/cil_write_ast.c:130:37: error: cast to smaller integer type 'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
                        enum cil_flavor operand_flavor = (enum cil_flavor)curr->data;
                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~

Silence this warning by casting the pointer to an integer the cast to
enum cil_flavor.

See 32f8ed3d6b0b ("libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast")

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

diff --git a/libsepol/cil/src/cil_write_ast.c b/libsepol/cil/src/cil_write_ast.c
index 4871f704..186070c1 100644
--- a/libsepol/cil/src/cil_write_ast.c
+++ b/libsepol/cil/src/cil_write_ast.c
@@ -83,7 +83,7 @@ static void write_expr(FILE *out, struct cil_list *expr)
 			break;
 		case CIL_OP: {
 			const char *op_str;
-			enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
+			enum cil_flavor op_flavor = (enum cil_flavor)(uintptr_t)curr->data;
 			switch (op_flavor) {
 			case CIL_AND:
 				op_str = CIL_KEY_AND;
@@ -127,7 +127,7 @@ static void write_expr(FILE *out, struct cil_list *expr)
 		}
 		case CIL_CONS_OPERAND: {
 			const char *operand_str;
-			enum cil_flavor operand_flavor = (enum cil_flavor)curr->data;
+			enum cil_flavor operand_flavor = (enum cil_flavor)(uintptr_t)curr->data;
 			switch (operand_flavor) {
 			case CIL_CONS_U1:
 				operand_str = CIL_KEY_CONS_U1;
-- 
2.32.0

[PATCH 16/23] libsepol/cil: drop extra semicolon

[Christian Göttsche]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/cil/src/cil_build_ast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 9a9bc598..da298311 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -4153,7 +4153,7 @@ void cil_destroy_context(struct cil_context *context)
 		return;
 	}
 
-	cil_symtab_datum_destroy(&context->datum);;
+	cil_symtab_datum_destroy(&context->datum);
 
 	if (context->range_str == NULL && context->range != NULL) {
 		cil_destroy_levelrange(context->range);
-- 
2.32.0

[PATCH 17/23] libsepol/cil: drop dead store

[Christian Göttsche]

../cil/src/cil_binary.c:2230:24: warning: Value stored to 'cb_node' during its initialization is never read [deadcode.DeadStores]
        struct cil_tree_node *cb_node = node->cl_head;
                              ^~~~~~~   ~~~~~~~~~~~~~

Found by clang-analyzer

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/cil/src/cil_binary.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 85094b01..601fe8d1 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -2227,7 +2227,7 @@ int cil_booleanif_to_policydb(policydb_t *pdb, const struct cil_db *db, struct c
 	int rc = SEPOL_ERR;
 	struct cil_args_booleanif bool_args;
 	struct cil_booleanif *cil_boolif = (struct cil_booleanif*)node->data;
-	struct cil_tree_node *cb_node = node->cl_head;
+	struct cil_tree_node *cb_node;
 	struct cil_tree_node *true_node = NULL;
 	struct cil_tree_node *false_node = NULL;
 	struct cil_tree_node *tmp_node = NULL;
-- 
2.32.0

[PATCH 18/23] libsepol/cil: drop unnecessary casts

[Christian Göttsche]

`const_hashtab_key_t` is a typedef of `const char *`, so these casts are
not needed.

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

diff --git a/libsepol/cil/src/cil_strpool.c b/libsepol/cil/src/cil_strpool.c
index 70bca363..e32ee4e9 100644
--- a/libsepol/cil/src/cil_strpool.c
+++ b/libsepol/cil/src/cil_strpool.c
@@ -47,14 +47,13 @@ static hashtab_t cil_strpool_tab = NULL;
 
 static unsigned int cil_strpool_hash(hashtab_t h, const_hashtab_key_t key)
 {
-	const char *p, *keyp;
+	const char *p;
 	size_t size;
 	unsigned int val;
 
 	val = 0;
-	keyp = (const char*)key;
-	size = strlen(keyp);
-	for (p = keyp; ((size_t) (p - keyp)) < size; p++)
+	size = strlen(key);
+	for (p = key; ((size_t) (p - key)) < size; p++)
 		val =
 		    (val << 4 | (val >> (8 * sizeof(unsigned int) - 4))) ^ (*p);
 	return val & (h->size - 1);
@@ -62,9 +61,7 @@ static unsigned int cil_strpool_hash(hashtab_t h, const_hashtab_key_t key)
 
 static int cil_strpool_compare(hashtab_t h __attribute__ ((unused)), const_hashtab_key_t key1, const_hashtab_key_t key2)
 {
-	const char *keyp1 = (const char*)key1;
-	const char *keyp2 = (const char*)key2;
-	return strcmp(keyp1, keyp2);
+	return strcmp(key1, key2);
 }
 
 char *cil_strpool_add(const char *str)
@@ -73,12 +70,12 @@ char *cil_strpool_add(const char *str)
 
 	pthread_mutex_lock(&cil_strpool_mutex);
 
-	strpool_ref = hashtab_search(cil_strpool_tab, (hashtab_key_t)str);
+	strpool_ref = hashtab_search(cil_strpool_tab, str);
 	if (strpool_ref == NULL) {
 		int rc;
 		strpool_ref = cil_malloc(sizeof(*strpool_ref));
 		strpool_ref->str = cil_strdup(str);
-		rc = hashtab_insert(cil_strpool_tab, (hashtab_key_t)strpool_ref->str, strpool_ref);
+		rc = hashtab_insert(cil_strpool_tab, strpool_ref->str, strpool_ref);
 		if (rc != SEPOL_OK) {
 			pthread_mutex_unlock(&cil_strpool_mutex);
 			cil_log(CIL_ERR, "Failed to allocate memory\n");
-- 
2.32.0

[PATCH 19/23] libsepol/cil: avoid using maybe uninitialized variables

[Christian Göttsche]

Initialize variables, as they are set after goto statements, which jump
to cleanup code using them.

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

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 601fe8d1..54d13f2f 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -1073,7 +1073,7 @@ int __cil_type_rule_to_avtab(policydb_t *pdb, const struct cil_db *db, struct ci
 	type_datum_t *sepol_src = NULL;
 	type_datum_t *sepol_tgt = NULL;
 	class_datum_t *sepol_obj = NULL;
-	struct cil_list *class_list;
+	struct cil_list *class_list = NULL;
 	type_datum_t *sepol_result = NULL;
 	ebitmap_t src_bitmap, tgt_bitmap;
 	ebitmap_node_t *node1, *node2;
@@ -1129,7 +1129,7 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
 	type_datum_t *sepol_src = NULL;
 	type_datum_t *sepol_tgt = NULL;
 	class_datum_t *sepol_obj = NULL;
-	struct cil_list *class_list;
+	struct cil_list *class_list = NULL;
 	type_datum_t *sepol_result = NULL;
 	ebitmap_t src_bitmap, tgt_bitmap;
 	ebitmap_node_t *node1, *node2;
@@ -2338,7 +2338,7 @@ int cil_roletrans_to_policydb(policydb_t *pdb, const struct cil_db *db, struct c
 	role_datum_t *sepol_src = NULL;
 	type_datum_t *sepol_tgt = NULL;
 	class_datum_t *sepol_obj = NULL;
-	struct cil_list *class_list;
+	struct cil_list *class_list = NULL;
 	role_datum_t *sepol_result = NULL;
 	role_trans_t *new = NULL;
 	uint32_t *new_role = NULL;
@@ -3166,7 +3166,7 @@ int cil_rangetransition_to_policydb(policydb_t *pdb, const struct cil_db *db, st
 	type_datum_t *sepol_src = NULL;
 	type_datum_t *sepol_tgt = NULL;
 	class_datum_t *sepol_class = NULL;
-	struct cil_list *class_list;
+	struct cil_list *class_list = NULL;
 	range_trans_t *newkey = NULL;
 	struct mls_range *newdatum = NULL;
 	ebitmap_t src_bitmap, tgt_bitmap;
@@ -3603,7 +3603,7 @@ int cil_default_to_policydb(policydb_t *pdb, struct cil_default *def)
 {
 	struct cil_list_item *curr;
 	class_datum_t *sepol_class;
-	struct cil_list *class_list;
+	struct cil_list *class_list = NULL;
 
 	cil_list_for_each(curr, def->class_datums) {
 		struct cil_list_item *c;
@@ -3658,7 +3658,7 @@ int cil_defaultrange_to_policydb(policydb_t *pdb, struct cil_defaultrange *def)
 {
 	struct cil_list_item *curr;
 	class_datum_t *sepol_class;
-	struct cil_list *class_list;
+	struct cil_list *class_list = NULL;
 
 	cil_list_for_each(curr, def->class_datums) {
 		struct cil_list_item *c;
-- 
2.32.0

[PATCH 20/23] libsepol: drop repeated semicolons

[Christian Göttsche]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/kernel_to_cil.c | 2 +-
 libsepol/src/module.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index 17b5ebf0..238a2483 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -1050,7 +1050,7 @@ static char *cats_ebitmap_to_str(struct ebitmap *cats, char **val_to_name)
 	p = catsbuf;
 
 	*p++ = '(';
-	remaining--;;
+	remaining--;
 
 	range = 0;
 	ebitmap_for_each_positive_bit(cats, node, i) {
diff --git a/libsepol/src/module.c b/libsepol/src/module.c
index 836da308..9b53bc47 100644
--- a/libsepol/src/module.c
+++ b/libsepol/src/module.c
@@ -82,7 +82,7 @@ static int policy_file_length(struct policy_file *fp, size_t *out)
 		break;
 	case PF_USE_MEMORY:
 		*out = fp->size;
-		break;;
+		break;
 	default:
 		*out = 0;
 		break;
-- 
2.32.0

[PATCH 21/23] libsepol: drop unnecessary casts

[Christian Göttsche]

`hashtab_search()` does take `const_hashtab_key_t` as second parameter,
which is a typedef for `const char *`.
Drop the unnecessary and const-violating cast.

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

diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index f7c31d80..47a3dc14 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -1182,7 +1182,7 @@ int sepol_string_to_security_class(const char *class_name,
 	class_datum_t *tclass_datum;
 
 	tclass_datum = hashtab_search(policydb->p_classes.table,
-				      (hashtab_key_t) class_name);
+				      class_name);
 	if (!tclass_datum) {
 		ERR(NULL, "unrecognized class %s", class_name);
 		return STATUS_ERR;
@@ -1211,7 +1211,7 @@ int sepol_string_to_av_perm(sepol_security_class_t tclass,
 	/* Check for unique perms then the common ones (if any) */
 	perm_datum = (perm_datum_t *)
 			hashtab_search(tclass_datum->permissions.table,
-			(hashtab_key_t)perm_name);
+			perm_name);
 	if (perm_datum != NULL) {
 		*av = 0x1 << (perm_datum->s.value - 1);
 		return STATUS_SUCCESS;
@@ -1222,7 +1222,7 @@ int sepol_string_to_av_perm(sepol_security_class_t tclass,
 
 	perm_datum = (perm_datum_t *)
 			hashtab_search(tclass_datum->comdatum->permissions.table,
-			(hashtab_key_t)perm_name);
+			perm_name);
 
 	if (perm_datum != NULL) {
 		*av = 0x1 << (perm_datum->s.value - 1);
-- 
2.32.0

[PATCH 22/23] libsepol: declare file local variable static

[Christian Göttsche]

Clang issues:

    module_to_cil.c:65:7: warning: no previous extern declaration for non-static variable 'out_file' [-Wmissing-variable-declarations]
    FILE *out_file;
          ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/module_to_cil.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 73ec7971..1d724b91 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -62,7 +62,7 @@
 #  define UNUSED(x) UNUSED_ ## x
 #endif
 
-FILE *out_file;
+static FILE *out_file;
 
 #define STACK_SIZE 16
 #define DEFAULT_LEVEL "systemlow"
-- 
2.32.0

[PATCH 23/23] libsepol: declare read-only arrays const

[Christian Göttsche]

Make it more apparent that those data does not change and enforce it.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/avrule_block.c      |  2 +-
 libsepol/src/avtab.c             |  2 +-
 libsepol/src/link.c              |  2 +-
 libsepol/src/polcaps.c           |  2 +-
 libsepol/src/policydb.c          | 22 +++++++++++-----------
 libsepol/src/policydb_internal.h |  2 +-
 libsepol/src/private.h           |  6 +++---
 libsepol/src/write.c             |  8 ++++----
 8 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/libsepol/src/avrule_block.c b/libsepol/src/avrule_block.c
index a9832d0d..dcfce8b8 100644
--- a/libsepol/src/avrule_block.c
+++ b/libsepol/src/avrule_block.c
@@ -30,7 +30,7 @@
 /* It is anticipated that there be less declarations within an avrule
  * block than the global policy.  Thus the symbol table sizes are
  * smaller than those listed in policydb.c */
-static unsigned int symtab_sizes[SYM_NUM] = {
+static const unsigned int symtab_sizes[SYM_NUM] = {
 	2,
 	4,
 	8,
diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
index c2ccb005..7b80377c 100644
--- a/libsepol/src/avtab.c
+++ b/libsepol/src/avtab.c
@@ -424,7 +424,7 @@ void avtab_hash_eval(avtab_t * h, char *tag)
 }
 
 /* Ordering of datums in the original avtab format in the policy file. */
-static uint16_t spec_order[] = {
+static const uint16_t spec_order[] = {
 	AVTAB_ALLOWED,
 	AVTAB_AUDITDENY,
 	AVTAB_AUDITALLOW,
diff --git a/libsepol/src/link.c b/libsepol/src/link.c
index bdc1fcbf..461d2feb 100644
--- a/libsepol/src/link.c
+++ b/libsepol/src/link.c
@@ -78,7 +78,7 @@ typedef struct missing_requirement {
 	uint32_t perm_value;
 } missing_requirement_t;
 
-static const char *symtab_names[SYM_NUM] = {
+static const char * const symtab_names[SYM_NUM] = {
 	"common", "class", "role", "type/attribute", "user",
 	"bool", "level", "category"
 };
diff --git a/libsepol/src/polcaps.c b/libsepol/src/polcaps.c
index 67ed5786..6a74ec7d 100644
--- a/libsepol/src/polcaps.c
+++ b/libsepol/src/polcaps.c
@@ -5,7 +5,7 @@
 #include <string.h>
 #include <sepol/policydb/polcaps.h>
 
-static const char *polcap_names[] = {
+static const char * const polcap_names[] = {
 	"network_peer_controls",	/* POLICYDB_CAPABILITY_NETPEER */
 	"open_perms",			/* POLICYDB_CAPABILITY_OPENPERM */
 	"extended_socket_class",	/* POLICYDB_CAPABILITY_EXTSOCKCLASS */
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 7739b0fb..0bc0ead8 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -57,10 +57,10 @@
 #include "policydb_validate.h"
 
 #define POLICYDB_TARGET_SZ   ARRAY_SIZE(policydb_target_strings)
-const char *policydb_target_strings[] = { POLICYDB_STRING, POLICYDB_XEN_STRING };
+const char * const policydb_target_strings[] = { POLICYDB_STRING, POLICYDB_XEN_STRING };
 
 /* These need to be updated if SYM_NUM or OCON_NUM changes */
-static struct policydb_compat_info policydb_compat[] = {
+static const struct policydb_compat_info policydb_compat[] = {
 	{
 	 .type = POLICY_KERN,
 	 .version = POLICYDB_VERSION_BOUNDARY,
@@ -460,7 +460,7 @@ static char *symtab_name[SYM_NUM] = {
 };
 #endif
 
-static unsigned int symtab_sizes[SYM_NUM] = {
+static const unsigned int symtab_sizes[SYM_NUM] = {
 	2,
 	32,
 	16,
@@ -471,12 +471,12 @@ static unsigned int symtab_sizes[SYM_NUM] = {
 	16,
 };
 
-struct policydb_compat_info *policydb_lookup_compat(unsigned int version,
-						    unsigned int type,
-						unsigned int target_platform)
+const struct policydb_compat_info *policydb_lookup_compat(unsigned int version,
+						          unsigned int type,
+						          unsigned int target_platform)
 {
 	unsigned int i;
-	struct policydb_compat_info *info = NULL;
+	const struct policydb_compat_info *info = NULL;
 
 	for (i = 0; i < sizeof(policydb_compat) / sizeof(*info); i++) {
 		if (policydb_compat[i].version == version &&
@@ -2854,7 +2854,7 @@ static int filename_trans_read(policydb_t *p, struct policy_file *fp)
 	return 0;
 }
 
-static int ocontext_read_xen(struct policydb_compat_info *info,
+static int ocontext_read_xen(const struct policydb_compat_info *info,
 	policydb_t *p, struct policy_file *fp)
 {
 	unsigned int i, j;
@@ -2963,7 +2963,7 @@ static int ocontext_read_xen(struct policydb_compat_info *info,
 	}
 	return 0;
 }
-static int ocontext_read_selinux(struct policydb_compat_info *info,
+static int ocontext_read_selinux(const struct policydb_compat_info *info,
 			 policydb_t * p, struct policy_file *fp)
 {
 	unsigned int i, j;
@@ -3141,7 +3141,7 @@ static int ocontext_read_selinux(struct policydb_compat_info *info,
 	return 0;
 }
 
-static int ocontext_read(struct policydb_compat_info *info,
+static int ocontext_read(const struct policydb_compat_info *info,
 	policydb_t *p, struct policy_file *fp)
 {
 	int rc = -1;
@@ -4198,7 +4198,7 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose)
 	uint32_t buf[5];
 	size_t len, nprim, nel;
 	char *policydb_str;
-	struct policydb_compat_info *info;
+	const struct policydb_compat_info *info;
 	unsigned int policy_type, bufindex;
 	ebitmap_node_t *tnode;
 	int rc;
diff --git a/libsepol/src/policydb_internal.h b/libsepol/src/policydb_internal.h
index 06ba5c8b..dd8f25d0 100644
--- a/libsepol/src/policydb_internal.h
+++ b/libsepol/src/policydb_internal.h
@@ -3,5 +3,5 @@
 
 #include <sepol/policydb.h>
 
-extern const char *policydb_target_strings[];
+extern const char * const policydb_target_strings[];
 #endif
diff --git a/libsepol/src/private.h b/libsepol/src/private.h
index f5b5277f..72f21262 100644
--- a/libsepol/src/private.h
+++ b/libsepol/src/private.h
@@ -56,9 +56,9 @@ struct policydb_compat_info {
 	unsigned int target_platform;
 };
 
-extern struct policydb_compat_info *policydb_lookup_compat(unsigned int version,
-							   unsigned int type,
-						unsigned int target_platform);
+extern const struct policydb_compat_info *policydb_lookup_compat(unsigned int version,
+								 unsigned int type,
+								 unsigned int target_platform);
 
 /* Reading from a policy "file". */
 extern int next_entry(void *buf, struct policy_file *fp, size_t bytes);
diff --git a/libsepol/src/write.c b/libsepol/src/write.c
index 84bcaf3f..3bd034d6 100644
--- a/libsepol/src/write.c
+++ b/libsepol/src/write.c
@@ -1345,7 +1345,7 @@ static int (*write_f[SYM_NUM]) (hashtab_key_t key, hashtab_datum_t datum,
 common_write, class_write, role_write, type_write, user_write,
 	    cond_write_bool, sens_write, cat_write,};
 
-static int ocontext_write_xen(struct policydb_compat_info *info, policydb_t *p,
+static int ocontext_write_xen(const struct policydb_compat_info *info, policydb_t *p,
 			  struct policy_file *fp)
 {
 	unsigned int i, j;
@@ -1453,7 +1453,7 @@ static int ocontext_write_xen(struct policydb_compat_info *info, policydb_t *p,
 	return POLICYDB_SUCCESS;
 }
 
-static int ocontext_write_selinux(struct policydb_compat_info *info,
+static int ocontext_write_selinux(const struct policydb_compat_info *info,
 	policydb_t *p, struct policy_file *fp)
 {
 	unsigned int i, j;
@@ -1583,7 +1583,7 @@ static int ocontext_write_selinux(struct policydb_compat_info *info,
 	return POLICYDB_SUCCESS;
 }
 
-static int ocontext_write(struct policydb_compat_info *info, policydb_t * p,
+static int ocontext_write(const struct policydb_compat_info *info, policydb_t * p,
 	struct policy_file *fp)
 {
 	int rc = POLICYDB_ERROR;
@@ -2179,7 +2179,7 @@ int policydb_write(policydb_t * p, struct policy_file *fp)
 	unsigned int i, num_syms;
 	uint32_t buf[32], config;
 	size_t items, items2, len;
-	struct policydb_compat_info *info;
+	const struct policydb_compat_info *info;
 	struct policy_data pd;
 	const char *policydb_str;
 
-- 
2.32.0

Re: [PATCH 04/23] libsepol: ignore UBSAN false-positives

[Ondrej Mosnacek]

On Tue, Jun 8, 2021 at 6:01 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
> Unsigned integer overflow is well-defined and not undefined behavior.
> But it is still useful to enable undefined behavior sanitizer checks on
> unsigned arithmetic to detect possible issues on counters or variables
> with similar purpose.
>
> Annotate functions in which unsigned overflows are expected to happen.
>
> avtab.c:76:2: runtime error: unsigned integer overflow: 6 * 3432918353 cannot be represented in type 'unsigned int'
> policydb.c:795:42: runtime error: unsigned integer overflow: 8160943042179512010 * 11 cannot be represented in type 'unsigned long'
> symtab.c:25:12: runtime error: left shift of 1766601759 by 4 places cannot be represented in type 'unsigned int'
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsepol/src/avtab.c    | 6 ++++++
>  libsepol/src/policydb.c | 6 ++++++
>  libsepol/src/symtab.c   | 6 ++++++
>  3 files changed, 18 insertions(+)
>
> diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
> index 257f051a..c2ccb005 100644
> --- a/libsepol/src/avtab.c
> +++ b/libsepol/src/avtab.c
> @@ -52,6 +52,12 @@
>  /* Based on MurmurHash3, written by Austin Appleby and placed in the
>   * public domain.
>   */
> +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> +__attribute__((no_sanitize("unsigned-integer-overflow")))
> +#if (__clang_major__ >= 12)
> +__attribute__((no_sanitize("unsigned-shift-base")))
> +#endif
> +#endif

Perhaps this could be defined as a common macro in
libsepol/src/private.h instead of duplicating it in three places?

>  static inline int avtab_hash(struct avtab_key *keyp, uint32_t mask)
>  {
>         static const uint32_t c1 = 0xcc9e2d51;
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index fc1d0711..cbe0c432 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -789,6 +789,12 @@ static int roles_init(policydb_t * p)
>         goto out;
>  }
>
> +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> +__attribute__((no_sanitize("unsigned-integer-overflow")))
> +#if (__clang_major__ >= 12)
> +__attribute__((no_sanitize("unsigned-shift-base")))
> +#endif
> +#endif
>  static inline unsigned long
>  partial_name_hash(unsigned long c, unsigned long prevhash)
>  {
> diff --git a/libsepol/src/symtab.c b/libsepol/src/symtab.c
> index 9a417ca2..738fa0a4 100644
> --- a/libsepol/src/symtab.c
> +++ b/libsepol/src/symtab.c
> @@ -11,6 +11,12 @@
>  #include <sepol/policydb/hashtab.h>
>  #include <sepol/policydb/symtab.h>
>
> +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> +__attribute__((no_sanitize("unsigned-integer-overflow")))
> +#if (__clang_major__ >= 12)
> +__attribute__((no_sanitize("unsigned-shift-base")))
> +#endif
> +#endif
>  static unsigned int symhash(hashtab_t h, const_hashtab_key_t key)
>  {
>         const char *p, *keyp;
> --
> 2.32.0
>

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH 05/23] libsepol: avoid implicit conversions

[Ondrej Mosnacek]

On Tue, Jun 8, 2021 at 5:59 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Avoid implicit conversions from signed to unsigned values, found by
> UB sanitizers, by using unsigned values in the first place.
>
> expand.c:1644:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
>
> expand.c:2892:24: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned int' changed the value to 4294967294 (32-bit, unsigned)
>
> policy_define.c:2344:4: runtime error: implicit conversion from type 'int' of value -1048577 (32-bit, signed) to type 'unsigned int' changed the value to 4293918719 (32-bit, unsigned)
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsepol/include/sepol/policydb/conditional.h | 2 +-
>  libsepol/include/sepol/policydb/policydb.h    | 2 +-
>  libsepol/src/expand.c                         | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libsepol/include/sepol/policydb/conditional.h b/libsepol/include/sepol/policydb/conditional.h
> index 9c3df3ef..db3ef98d 100644
> --- a/libsepol/include/sepol/policydb/conditional.h
> +++ b/libsepol/include/sepol/policydb/conditional.h
> @@ -90,7 +90,7 @@ typedef struct cond_node {
>         uint32_t expr_pre_comp;
>         struct cond_node *next;
>         /* a tunable conditional, calculated and used at expansion */
> -#define        COND_NODE_FLAGS_TUNABLE 0x01
> +#define        COND_NODE_FLAGS_TUNABLE 0x01U

UINT32_C(0x01) would be better here.

>         uint32_t flags;
>  } cond_node_t;
>
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index 9ef43abc..c29339dc 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -253,7 +253,7 @@ typedef struct class_perm_node {
>
>  #define xperm_test(x, p) (1 & (p[x >> 5] >> (x & 0x1f)))
>  #define xperm_set(x, p) (p[x >> 5] |= (1 << (x & 0x1f)))
> -#define xperm_clear(x, p) (p[x >> 5] &= ~(1 << (x & 0x1f)))
> +#define xperm_clear(x, p) (p[x >> 5] &= ~(1U << (x & 0x1f)))

Ditto here (used on av_extended_perms_t::perms, which is uint32_t).
And xperm_test() and xperm_set() should be also updated in the same
way.

>  #define EXTENDED_PERMS_LEN 8
>
>  typedef struct av_extended_perms {
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 84bfcfa3..35e45780 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1641,7 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
>                  * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
>                  * others. Initialize the data accordingly.
>                  */
> -               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
> +               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0U : 0U;

Also UINT32_C().

>                 /* this is used to get the node - insertion is actually unique */
>                 node = avtab_insert_nonunique(avtab, key, &avdatum);
>                 if (!node) {
> --
> 2.32.0
>

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH 04/23] libsepol: ignore UBSAN false-positives

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Unsigned integer overflow is well-defined and not undefined behavior.
> But it is still useful to enable undefined behavior sanitizer checks on
> unsigned arithmetic to detect possible issues on counters or variables
> with similar purpose.
>
> Annotate functions in which unsigned overflows are expected to happen.
>
> avtab.c:76:2: runtime error: unsigned integer overflow: 6 * 3432918353 cannot be represented in type 'unsigned int'
> policydb.c:795:42: runtime error: unsigned integer overflow: 8160943042179512010 * 11 cannot be represented in type 'unsigned long'
> symtab.c:25:12: runtime error: left shift of 1766601759 by 4 places cannot be represented in type 'unsigned int'
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsepol/src/avtab.c    | 6 ++++++
>  libsepol/src/policydb.c | 6 ++++++
>  libsepol/src/symtab.c   | 6 ++++++
>  3 files changed, 18 insertions(+)
>
> diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
> index 257f051a..c2ccb005 100644
> --- a/libsepol/src/avtab.c
> +++ b/libsepol/src/avtab.c
> @@ -52,6 +52,12 @@
>  /* Based on MurmurHash3, written by Austin Appleby and placed in the
>   * public domain.
>   */
> +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> +__attribute__((no_sanitize("unsigned-integer-overflow")))
> +#if (__clang_major__ >= 12)
> +__attribute__((no_sanitize("unsigned-shift-base")))
> +#endif
> +#endif

My understanding is that there is no equivalent in gcc. Is that correct?
Jim

>  static inline int avtab_hash(struct avtab_key *keyp, uint32_t mask)
>  {
>         static const uint32_t c1 = 0xcc9e2d51;
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index fc1d0711..cbe0c432 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -789,6 +789,12 @@ static int roles_init(policydb_t * p)
>         goto out;
>  }
>
> +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> +__attribute__((no_sanitize("unsigned-integer-overflow")))
> +#if (__clang_major__ >= 12)
> +__attribute__((no_sanitize("unsigned-shift-base")))
> +#endif
> +#endif
>  static inline unsigned long
>  partial_name_hash(unsigned long c, unsigned long prevhash)
>  {
> diff --git a/libsepol/src/symtab.c b/libsepol/src/symtab.c
> index 9a417ca2..738fa0a4 100644
> --- a/libsepol/src/symtab.c
> +++ b/libsepol/src/symtab.c
> @@ -11,6 +11,12 @@
>  #include <sepol/policydb/hashtab.h>
>  #include <sepol/policydb/symtab.h>
>
> +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> +__attribute__((no_sanitize("unsigned-integer-overflow")))
> +#if (__clang_major__ >= 12)
> +__attribute__((no_sanitize("unsigned-shift-base")))
> +#endif
> +#endif
>  static unsigned int symhash(hashtab_t h, const_hashtab_key_t key)
>  {
>         const char *p, *keyp;
> --
> 2.32.0
>

Re: [PATCH 13/23] libsepol: assure string NUL-termination

[James Carter]

On Tue, Jun 8, 2021 at 12:00 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> ibendport_record.c: In function ‘sepol_ibendport_get_ibdev_name’:
> ibendport_record.c:169:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
>   169 |  strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ibendport_record.c: In function ‘sepol_ibendport_set_ibdev_name’:
> ibendport_record.c:189:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
>   189 |  strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> strncpy(3) does not NUL-terminate the destination if the source is of
> the same length or longer then the specified size.
> Reduce the size to copy by 1.
>
> Found by Clang
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsepol/src/ibendport_record.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/src/ibendport_record.c b/libsepol/src/ibendport_record.c
> index adf67161..2eb8ca18 100644
> --- a/libsepol/src/ibendport_record.c
> +++ b/libsepol/src/ibendport_record.c
> @@ -166,7 +166,7 @@ int sepol_ibendport_get_ibdev_name(sepol_handle_t *handle,
>         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_ibdev_name) < 0)
>                 goto err;
>
> -       strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
>         *ibdev_name = tmp_ibdev_name;
>         return STATUS_SUCCESS;
>
> @@ -186,7 +186,7 @@ int sepol_ibendport_set_ibdev_name(sepol_handle_t *handle,
>         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp) < 0)
>                 goto err;
>
> -       strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX - 1);
>         free(ibendport->ibdev_name);
>         ibendport->ibdev_name = tmp;
>         return STATUS_SUCCESS;
> --
> 2.32.0
>

It seems like the strncpy()s at line 65 in
sepol_ibendport_key_create(), at line 233 in sepol_ibendport_clone(),
and in ibendports.c at line 37 in ibendport_from_record() also need to
be changed.
Thanks,
Jim

Re: [PATCH 03/23] libsepol: remove unused functions

[James Carter]

On Tue, Jun 8, 2021 at 12:00 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The functions `role_set_get_role`, `sepol_validate_transition` and
> `sepol_sidtab_remove` seem to be unused since the initial import.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/policydb.c | 18 ----------------
>  libsepol/src/services.c | 47 -----------------------------------------
>  libsepol/src/sidtab.c   | 31 ---------------------------
>  3 files changed, 96 deletions(-)
>
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index 3f7ddb11..fc1d0711 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -1791,24 +1791,6 @@ int type_set_or_eq(type_set_t * dst, type_set_t * other)
>         return ret;
>  }
>
> -int role_set_get_role(role_set_t * x, uint32_t role)
> -{
> -       if (x->flags & ROLE_STAR)
> -               return 1;
> -
> -       if (ebitmap_get_bit(&x->roles, role - 1)) {
> -               if (x->flags & ROLE_COMP)
> -                       return 0;
> -               else
> -                       return 1;
> -       } else {
> -               if (x->flags & ROLE_COMP)
> -                       return 1;
> -               else
> -                       return 0;
> -       }
> -}
> -
>  /***********************************************************************/
>  /* everything below is for policy reads */
>
> diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> index 39fbd979..ff91f7d2 100644
> --- a/libsepol/src/services.c
> +++ b/libsepol/src/services.c
> @@ -1024,53 +1024,6 @@ static int context_struct_compute_av(context_struct_t * scontext,
>         return 0;
>  }
>
> -static int sepol_validate_transition(sepol_security_id_t oldsid,
> -                                    sepol_security_id_t newsid,
> -                                    sepol_security_id_t tasksid,
> -                                    sepol_security_class_t tclass)
> -{
> -       context_struct_t *ocontext;
> -       context_struct_t *ncontext;
> -       context_struct_t *tcontext;
> -       class_datum_t *tclass_datum;
> -       constraint_node_t *constraint;
> -
> -       if (!tclass || tclass > policydb->p_classes.nprim) {
> -               ERR(NULL, "unrecognized class %d", tclass);
> -               return -EINVAL;
> -       }
> -       tclass_datum = policydb->class_val_to_struct[tclass - 1];
> -
> -       ocontext = sepol_sidtab_search(sidtab, oldsid);
> -       if (!ocontext) {
> -               ERR(NULL, "unrecognized SID %d", oldsid);
> -               return -EINVAL;
> -       }
> -
> -       ncontext = sepol_sidtab_search(sidtab, newsid);
> -       if (!ncontext) {
> -               ERR(NULL, "unrecognized SID %d", newsid);
> -               return -EINVAL;
> -       }
> -
> -       tcontext = sepol_sidtab_search(sidtab, tasksid);
> -       if (!tcontext) {
> -               ERR(NULL, "unrecognized SID %d", tasksid);
> -               return -EINVAL;
> -       }
> -
> -       constraint = tclass_datum->validatetrans;
> -       while (constraint) {
> -               if (!constraint_expr_eval_reason(ocontext, ncontext, tcontext,
> -                                         0, constraint, NULL, 0)) {
> -                       return -EPERM;
> -               }
> -               constraint = constraint->next;
> -       }
> -
> -       return 0;
> -}
> -
>  /*
>   * sepol_validate_transition_reason_buffer - the reason buffer is realloc'd
>   * in the constraint_expr_eval_reason() function.
> diff --git a/libsepol/src/sidtab.c b/libsepol/src/sidtab.c
> index e6bf5716..255e0725 100644
> --- a/libsepol/src/sidtab.c
> +++ b/libsepol/src/sidtab.c
> @@ -84,37 +84,6 @@ int sepol_sidtab_insert(sidtab_t * s, sepol_security_id_t sid,
>         return 0;
>  }
>
> -int sepol_sidtab_remove(sidtab_t * s, sepol_security_id_t sid)
> -{
> -       int hvalue;
> -       sidtab_node_t *cur, *last;
> -
> -       if (!s || !s->htable)
> -               return -ENOENT;
> -
> -       hvalue = SIDTAB_HASH(sid);
> -       last = NULL;
> -       cur = s->htable[hvalue];
> -       while (cur != NULL && sid > cur->sid) {
> -               last = cur;
> -               cur = cur->next;
> -       }
> -
> -       if (cur == NULL || sid != cur->sid)
> -               return -ENOENT;
> -
> -       if (last == NULL)
> -               s->htable[hvalue] = cur->next;
> -       else
> -               last->next = cur->next;
> -
> -       context_destroy(&cur->context);
> -
> -       free(cur);
> -       s->nel--;
> -       return 0;
> -}
> -
>  context_struct_t *sepol_sidtab_search(sidtab_t * s, sepol_security_id_t sid)
>  {
>         int hvalue;
> --
> 2.32.0
>

Re: [PATCH 01/23] libsepol: fix typos

[James Carter]

On Tue, Jun 8, 2021 at 12:00 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_build_ast.c   | 2 +-
>  libsepol/cil/src/cil_resolve_ast.c | 2 +-
>  libsepol/src/module_to_cil.c       | 2 +-
>  libsepol/src/policydb_validate.c   | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 71f14e20..42d10c87 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -3692,7 +3692,7 @@ int cil_gen_sensitivityorder(struct cil_db *db, struct cil_tree_node *parse_curr
>
>         cil_list_for_each(curr, sensorder->sens_list_str) {
>                 if (curr->data == CIL_KEY_UNORDERED) {
> -                       cil_log(CIL_ERR, "Sensitivy order cannot be unordered.\n");
> +                       cil_log(CIL_ERR, "Sensitivity order cannot be unordered.\n");
>                         rc = SEPOL_ERR;
>                         goto exit;
>                 }
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 77ffe0ff..d8481002 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -1619,7 +1619,7 @@ int cil_resolve_sensitivityorder(struct cil_tree_node *current, void *extra_args
>         cil_list_for_each(curr, sensorder->sens_list_str) {
>                 rc = cil_resolve_name(current, (char *)curr->data, CIL_SYM_SENS, extra_args, &datum);
>                 if (rc != SEPOL_OK) {
> -                       cil_log(CIL_ERR, "Failed to resolve sensitivty %s in sensitivityorder\n", (char *)curr->data);
> +                       cil_log(CIL_ERR, "Failed to resolve sensitivity %s in sensitivityorder\n", (char *)curr->data);
>                         goto exit;
>                 }
>                 if (FLAVOR(datum) != CIL_SENS) {
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index 496693f4..41605eb8 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -3972,7 +3972,7 @@ int sepol_module_policydb_to_cil(FILE *fp, struct policydb *pdb, int linked)
>
>         if (pdb->policy_type != SEPOL_POLICY_BASE &&
>                 pdb->policy_type != SEPOL_POLICY_MOD) {
> -               log_err("Policy pakcage is not a base or module");
> +               log_err("Policy package is not a base or module");
>                 rc = -1;
>                 goto exit;
>         }
> diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
> index b2891ddd..246aa6e3 100644
> --- a/libsepol/src/policydb_validate.c
> +++ b/libsepol/src/policydb_validate.c
> @@ -641,7 +641,7 @@ static int validate_scope_index(sepol_handle_t *handle, scope_index_t *scope_ind
>         return 0;
>
>  bad:
> -       ERR(handle, "Invalide scope");
> +       ERR(handle, "Invalid scope");
>         return -1;
>  }
>
> --
> 2.32.0
>

Re: [PATCH 20/23] libsepol: drop repeated semicolons

[James Carter]

On Tue, Jun 8, 2021 at 12:00 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/kernel_to_cil.c | 2 +-
>  libsepol/src/module.c        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index 17b5ebf0..238a2483 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -1050,7 +1050,7 @@ static char *cats_ebitmap_to_str(struct ebitmap *cats, char **val_to_name)
>         p = catsbuf;
>
>         *p++ = '(';
> -       remaining--;;
> +       remaining--;
>
>         range = 0;
>         ebitmap_for_each_positive_bit(cats, node, i) {
> diff --git a/libsepol/src/module.c b/libsepol/src/module.c
> index 836da308..9b53bc47 100644
> --- a/libsepol/src/module.c
> +++ b/libsepol/src/module.c
> @@ -82,7 +82,7 @@ static int policy_file_length(struct policy_file *fp, size_t *out)
>                 break;
>         case PF_USE_MEMORY:
>                 *out = fp->size;
> -               break;;
> +               break;
>         default:
>                 *out = 0;
>                 break;
> --
> 2.32.0
>

Re: [PATCH 18/23] libsepol/cil: drop unnecessary casts

[James Carter]

On Tue, Jun 8, 2021 at 12:00 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> `const_hashtab_key_t` is a typedef of `const char *`, so these casts are
> not needed.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_strpool.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_strpool.c b/libsepol/cil/src/cil_strpool.c
> index 70bca363..e32ee4e9 100644
> --- a/libsepol/cil/src/cil_strpool.c
> +++ b/libsepol/cil/src/cil_strpool.c
> @@ -47,14 +47,13 @@ static hashtab_t cil_strpool_tab = NULL;
>
>  static unsigned int cil_strpool_hash(hashtab_t h, const_hashtab_key_t key)
>  {
> -       const char *p, *keyp;
> +       const char *p;
>         size_t size;
>         unsigned int val;
>
>         val = 0;
> -       keyp = (const char*)key;
> -       size = strlen(keyp);
> -       for (p = keyp; ((size_t) (p - keyp)) < size; p++)
> +       size = strlen(key);
> +       for (p = key; ((size_t) (p - key)) < size; p++)
>                 val =
>                     (val << 4 | (val >> (8 * sizeof(unsigned int) - 4))) ^ (*p);
>         return val & (h->size - 1);
> @@ -62,9 +61,7 @@ static unsigned int cil_strpool_hash(hashtab_t h, const_hashtab_key_t key)
>
>  static int cil_strpool_compare(hashtab_t h __attribute__ ((unused)), const_hashtab_key_t key1, const_hashtab_key_t key2)
>  {
> -       const char *keyp1 = (const char*)key1;
> -       const char *keyp2 = (const char*)key2;
> -       return strcmp(keyp1, keyp2);
> +       return strcmp(key1, key2);
>  }
>
>  char *cil_strpool_add(const char *str)
> @@ -73,12 +70,12 @@ char *cil_strpool_add(const char *str)
>
>         pthread_mutex_lock(&cil_strpool_mutex);
>
> -       strpool_ref = hashtab_search(cil_strpool_tab, (hashtab_key_t)str);
> +       strpool_ref = hashtab_search(cil_strpool_tab, str);
>         if (strpool_ref == NULL) {
>                 int rc;
>                 strpool_ref = cil_malloc(sizeof(*strpool_ref));
>                 strpool_ref->str = cil_strdup(str);
> -               rc = hashtab_insert(cil_strpool_tab, (hashtab_key_t)strpool_ref->str, strpool_ref);
> +               rc = hashtab_insert(cil_strpool_tab, strpool_ref->str, strpool_ref);
>                 if (rc != SEPOL_OK) {
>                         pthread_mutex_unlock(&cil_strpool_mutex);
>                         cil_log(CIL_ERR, "Failed to allocate memory\n");
> --
> 2.32.0
>

Re: [PATCH 02/23] libsepol: resolve missing prototypes

[James Carter]

On Tue, Jun 8, 2021 at 12:01 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Declare the functions as static or include the corresponding header
> file.
>
> assertion.c:294:5: error: no previous prototype for function 'report_assertion_failures' [-Werror,-Wmissing-prototypes]
> int report_assertion_failures(sepol_handle_t *handle, policydb_t *p, avrule_t *avrule)
>     ^
>
> context.c:23:5: error: no previous prototype for function 'sepol_check_context' [-Werror,-Wmissing-prototypes]
> int sepol_check_context(const char *context)
>     ^
>
> expand.c:3377:5: error: no previous prototype for function 'expand_cond_av_node' [-Werror,-Wmissing-prototypes]
> int expand_cond_av_node(policydb_t * p,
>     ^
>
> policydb.c:638:6: error: no previous prototype for function 'role_trans_rule_destroy' [-Werror,-Wmissing-prototypes]
> void role_trans_rule_destroy(role_trans_rule_t * x)
>      ^
>
> policydb.c:1169:5: error: no previous prototype for function 'policydb_index_decls' [-Werror,-Wmissing-prototypes]
> int policydb_index_decls(sepol_handle_t * handle, policydb_t * p)
>     ^
>
> policydb.c:1429:6: error: no previous prototype for function 'ocontext_selinux_free' [-Werror,-Wmissing-prototypes]
> void ocontext_selinux_free(ocontext_t **ocontexts)
>      ^
>
> policydb.c:1451:6: error: no previous prototype for function 'ocontext_xen_free' [-Werror,-Wmissing-prototypes]
> void ocontext_xen_free(ocontext_t **ocontexts)
>      ^
>
> policydb.c:1750:5: error: no previous prototype for function 'type_set_or' [-Werror,-Wmissing-prototypes]
> int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
>     ^
>
> policydb.c:2524:5: error: no previous prototype for function 'role_trans_read' [-Werror,-Wmissing-prototypes]
> int role_trans_read(policydb_t *p, struct policy_file *fp)
>     ^
>
> policydb.c:2567:5: error: no previous prototype for function 'role_allow_read' [-Werror,-Wmissing-prototypes]
> int role_allow_read(role_allow_t ** r, struct policy_file *fp)
>     ^
>
> policydb.c:2842:5: error: no previous prototype for function 'filename_trans_read' [-Werror,-Wmissing-prototypes]
> int filename_trans_read(policydb_t *p, struct policy_file *fp)
>     ^
>
> services.c:1027:5: error: no previous prototype for function 'sepol_validate_transition' [-Werror,-Wmissing-prototypes]
> int sepol_validate_transition(sepol_security_id_t oldsid,
>     ^
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/assertion.c        |  2 +-
>  libsepol/src/context_internal.h |  1 +
>  libsepol/src/expand.c           |  6 +++---
>  libsepol/src/policydb.c         | 16 ++++++++--------
>  libsepol/src/services.c         |  2 +-
>  5 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
> index 266f67d7..dd2749a0 100644
> --- a/libsepol/src/assertion.c
> +++ b/libsepol/src/assertion.c
> @@ -291,7 +291,7 @@ exit:
>         return rc;
>  }
>
> -int report_assertion_failures(sepol_handle_t *handle, policydb_t *p, avrule_t *avrule)
> +static int report_assertion_failures(sepol_handle_t *handle, policydb_t *p, avrule_t *avrule)
>  {
>         int rc;
>         struct avtab_match_args args;
> diff --git a/libsepol/src/context_internal.h b/libsepol/src/context_internal.h
> index 3cae28cc..3dc9cd15 100644
> --- a/libsepol/src/context_internal.h
> +++ b/libsepol/src/context_internal.h
> @@ -1,6 +1,7 @@
>  #ifndef _SEPOL_CONTEXT_INTERNAL_H_
>  #define _SEPOL_CONTEXT_INTERNAL_H_
>
> +#include <sepol/context.h>
>  #include <sepol/context_record.h>
>
>  #endif
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index a656ffad..84bfcfa3 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -3374,9 +3374,9 @@ static int expand_cond_insert(cond_av_list_t ** l,
>         return 0;
>  }
>
> -int expand_cond_av_node(policydb_t * p,
> -                       avtab_ptr_t node,
> -                       cond_av_list_t ** newl, avtab_t * expa)
> +static int expand_cond_av_node(policydb_t * p,
> +                              avtab_ptr_t node,
> +                              cond_av_list_t ** newl, avtab_t * expa)
>  {
>         avtab_key_t *k = &node->key;
>         avtab_datum_t *d = &node->datum;
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index ffa27971..3f7ddb11 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -635,7 +635,7 @@ void role_trans_rule_init(role_trans_rule_t * x)
>         ebitmap_init(&x->classes);
>  }
>
> -void role_trans_rule_destroy(role_trans_rule_t * x)
> +static void role_trans_rule_destroy(role_trans_rule_t * x)
>  {
>         if (x != NULL) {
>                 role_set_destroy(&x->roles);
> @@ -1166,7 +1166,7 @@ int policydb_index_bools(policydb_t * p)
>         return 0;
>  }
>
> -int policydb_index_decls(sepol_handle_t * handle, policydb_t * p)
> +static int policydb_index_decls(sepol_handle_t * handle, policydb_t * p)
>  {
>         avrule_block_t *curblock;
>         avrule_decl_t *decl;
> @@ -1426,7 +1426,7 @@ static int range_tr_destroy(hashtab_key_t key, hashtab_datum_t datum,
>         return 0;
>  }
>
> -void ocontext_selinux_free(ocontext_t **ocontexts)
> +static void ocontext_selinux_free(ocontext_t **ocontexts)
>  {
>         ocontext_t *c, *ctmp;
>         int i;
> @@ -1448,7 +1448,7 @@ void ocontext_selinux_free(ocontext_t **ocontexts)
>         }
>  }
>
> -void ocontext_xen_free(ocontext_t **ocontexts)
> +static void ocontext_xen_free(ocontext_t **ocontexts)
>  {
>         ocontext_t *c, *ctmp;
>         int i;
> @@ -1747,7 +1747,7 @@ int symtab_insert(policydb_t * pol, uint32_t sym,
>         return retval;
>  }
>
> -int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
> +static int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
>  {
>         type_set_init(dst);
>
> @@ -2521,7 +2521,7 @@ static int type_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
>         return -1;
>  }
>
> -int role_trans_read(policydb_t *p, struct policy_file *fp)
> +static int role_trans_read(policydb_t *p, struct policy_file *fp)
>  {
>         role_trans_t **t = &p->role_tr;
>         unsigned int i;
> @@ -2564,7 +2564,7 @@ int role_trans_read(policydb_t *p, struct policy_file *fp)
>         return 0;
>  }
>
> -int role_allow_read(role_allow_t ** r, struct policy_file *fp)
> +static int role_allow_read(role_allow_t ** r, struct policy_file *fp)
>  {
>         unsigned int i;
>         uint32_t buf[2], nel;
> @@ -2839,7 +2839,7 @@ err:
>         return -1;
>  }
>
> -int filename_trans_read(policydb_t *p, struct policy_file *fp)
> +static int filename_trans_read(policydb_t *p, struct policy_file *fp)
>  {
>         unsigned int i;
>         uint32_t buf[1], nel;
> diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> index 6596431c..39fbd979 100644
> --- a/libsepol/src/services.c
> +++ b/libsepol/src/services.c
> @@ -1024,7 +1024,7 @@ static int context_struct_compute_av(context_struct_t * scontext,
>         return 0;
>  }
>
> -int sepol_validate_transition(sepol_security_id_t oldsid,
> +static int sepol_validate_transition(sepol_security_id_t oldsid,
>                                      sepol_security_id_t newsid,
>                                      sepol_security_id_t tasksid,
>                                      sepol_security_class_t tclass)
> --
> 2.32.0
>

Re: [PATCH 10/23] libsepol: mark read-only parameters of ebitmap interfaces const

[James Carter]

On Tue, Jun 8, 2021 at 12:01 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Make it more obvious which parameters are read-only and not being
> modified and allow callers to pass const pointers.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/include/sepol/policydb/ebitmap.h | 16 ++++++++--------
>  libsepol/src/ebitmap.c                    | 18 +++++++++---------
>  2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/libsepol/include/sepol/policydb/ebitmap.h b/libsepol/include/sepol/policydb/ebitmap.h
> index 634436f6..81d0c7a6 100644
> --- a/libsepol/include/sepol/policydb/ebitmap.h
> +++ b/libsepol/include/sepol/policydb/ebitmap.h
> @@ -67,7 +67,7 @@ static inline unsigned int ebitmap_next(ebitmap_node_t ** n, unsigned int bit)
>         return (bit + 1);
>  }
>
> -static inline int ebitmap_node_get_bit(ebitmap_node_t * n, unsigned int bit)
> +static inline int ebitmap_node_get_bit(const ebitmap_node_t * n, unsigned int bit)
>  {
>         if (n->map & (MAPBIT << (bit - n->startbit)))
>                 return 1;
> @@ -83,18 +83,18 @@ static inline int ebitmap_node_get_bit(ebitmap_node_t * n, unsigned int bit)
>  extern int ebitmap_cmp(const ebitmap_t * e1, const ebitmap_t * e2);
>  extern int ebitmap_or(ebitmap_t * dst, const ebitmap_t * e1, const ebitmap_t * e2);
>  extern int ebitmap_union(ebitmap_t * dst, const ebitmap_t * e1);
> -extern int ebitmap_and(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2);
> -extern int ebitmap_xor(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2);
> -extern int ebitmap_not(ebitmap_t *dst, ebitmap_t *e1, unsigned int maxbit);
> -extern int ebitmap_andnot(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2, unsigned int maxbit);
> -extern unsigned int ebitmap_cardinality(ebitmap_t *e1);
> -extern int ebitmap_hamming_distance(ebitmap_t * e1, ebitmap_t * e2);
> +extern int ebitmap_and(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2);
> +extern int ebitmap_xor(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2);
> +extern int ebitmap_not(ebitmap_t *dst, const ebitmap_t *e1, unsigned int maxbit);
> +extern int ebitmap_andnot(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2, unsigned int maxbit);
> +extern unsigned int ebitmap_cardinality(const ebitmap_t *e1);
> +extern int ebitmap_hamming_distance(const ebitmap_t * e1, const ebitmap_t * e2);
>  extern int ebitmap_cpy(ebitmap_t * dst, const ebitmap_t * src);
>  extern int ebitmap_contains(const ebitmap_t * e1, const ebitmap_t * e2);
>  extern int ebitmap_match_any(const ebitmap_t *e1, const ebitmap_t *e2);
>  extern int ebitmap_get_bit(const ebitmap_t * e, unsigned int bit);
>  extern int ebitmap_set_bit(ebitmap_t * e, unsigned int bit, int value);
> -extern unsigned int ebitmap_highest_set_bit(ebitmap_t * e);
> +extern unsigned int ebitmap_highest_set_bit(const ebitmap_t * e);
>  extern void ebitmap_destroy(ebitmap_t * e);
>  extern int ebitmap_read(ebitmap_t * e, void *fp);
>
> diff --git a/libsepol/src/ebitmap.c b/libsepol/src/ebitmap.c
> index 522e14a6..4e9acdf8 100644
> --- a/libsepol/src/ebitmap.c
> +++ b/libsepol/src/ebitmap.c
> @@ -71,7 +71,7 @@ int ebitmap_union(ebitmap_t * dst, const ebitmap_t * e1)
>         return 0;
>  }
>
> -int ebitmap_and(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2)
> +int ebitmap_and(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2)
>  {
>         unsigned int i, length = min(ebitmap_length(e1), ebitmap_length(e2));
>         ebitmap_init(dst);
> @@ -85,7 +85,7 @@ int ebitmap_and(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2)
>         return 0;
>  }
>
> -int ebitmap_xor(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2)
> +int ebitmap_xor(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2)
>  {
>         unsigned int i, length = max(ebitmap_length(e1), ebitmap_length(e2));
>         ebitmap_init(dst);
> @@ -98,7 +98,7 @@ int ebitmap_xor(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2)
>         return 0;
>  }
>
> -int ebitmap_not(ebitmap_t *dst, ebitmap_t *e1, unsigned int maxbit)
> +int ebitmap_not(ebitmap_t *dst, const ebitmap_t *e1, unsigned int maxbit)
>  {
>         unsigned int i;
>         ebitmap_init(dst);
> @@ -111,7 +111,7 @@ int ebitmap_not(ebitmap_t *dst, ebitmap_t *e1, unsigned int maxbit)
>         return 0;
>  }
>
> -int ebitmap_andnot(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2, unsigned int maxbit)
> +int ebitmap_andnot(ebitmap_t *dst, const ebitmap_t *e1, const ebitmap_t *e2, unsigned int maxbit)
>  {
>         int rc;
>         ebitmap_t e3;
> @@ -126,10 +126,10 @@ int ebitmap_andnot(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2, unsigned int ma
>         return 0;
>  }
>
> -unsigned int ebitmap_cardinality(ebitmap_t *e1)
> +unsigned int ebitmap_cardinality(const ebitmap_t *e1)
>  {
>         unsigned int count = 0;
> -       ebitmap_node_t *n;
> +       const ebitmap_node_t *n;
>
>         for (n = e1->node; n; n = n->next) {
>                 count += __builtin_popcountll(n->map);
> @@ -137,7 +137,7 @@ unsigned int ebitmap_cardinality(ebitmap_t *e1)
>         return count;
>  }
>
> -int ebitmap_hamming_distance(ebitmap_t * e1, ebitmap_t * e2)
> +int ebitmap_hamming_distance(const ebitmap_t * e1, const ebitmap_t * e2)
>  {
>         int rc;
>         ebitmap_t tmp;
> @@ -347,9 +347,9 @@ int ebitmap_set_bit(ebitmap_t * e, unsigned int bit, int value)
>         return 0;
>  }
>
> -unsigned int ebitmap_highest_set_bit(ebitmap_t * e)
> +unsigned int ebitmap_highest_set_bit(const ebitmap_t * e)
>  {
> -       ebitmap_node_t *n;
> +       const ebitmap_node_t *n;
>         MAPTYPE map;
>         unsigned int pos = 0;
>
> --
> 2.32.0
>

Re: [PATCH 17/23] libsepol/cil: drop dead store

[James Carter]

On Tue, Jun 8, 2021 at 12:01 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> ../cil/src/cil_binary.c:2230:24: warning: Value stored to 'cb_node' during its initialization is never read [deadcode.DeadStores]
>         struct cil_tree_node *cb_node = node->cl_head;
>                               ^~~~~~~   ~~~~~~~~~~~~~
>
> Found by clang-analyzer
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_binary.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 85094b01..601fe8d1 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -2227,7 +2227,7 @@ int cil_booleanif_to_policydb(policydb_t *pdb, const struct cil_db *db, struct c
>         int rc = SEPOL_ERR;
>         struct cil_args_booleanif bool_args;
>         struct cil_booleanif *cil_boolif = (struct cil_booleanif*)node->data;
> -       struct cil_tree_node *cb_node = node->cl_head;
> +       struct cil_tree_node *cb_node;
>         struct cil_tree_node *true_node = NULL;
>         struct cil_tree_node *false_node = NULL;
>         struct cil_tree_node *tmp_node = NULL;
> --
> 2.32.0
>

Re: [PATCH 08/23] libsepol/cil: follow declaration-after-statement

[James Carter]

On Tue, Jun 8, 2021 at 12:01 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Follow the project style of no declaration after statement.
>
> Found by the gcc warning -Wdeclaration-after-statement
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_binary.c      | 5 +++--
>  libsepol/cil/src/cil_build_ast.c   | 5 +++--
>  libsepol/cil/src/cil_fqn.c         | 3 ++-
>  libsepol/cil/src/cil_list.c        | 7 ++++---
>  libsepol/cil/src/cil_post.c        | 2 +-
>  libsepol/cil/src/cil_resolve_ast.c | 6 +++---
>  libsepol/cil/src/cil_strpool.c     | 3 ++-
>  7 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 18532aad..85094b01 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -593,11 +593,11 @@ exit:
>  int __cil_typeattr_bitmap_init(policydb_t *pdb)
>  {
>         int rc = SEPOL_ERR;
> +       uint32_t i;
>
>         pdb->type_attr_map = cil_malloc(pdb->p_types.nprim * sizeof(ebitmap_t));
>         pdb->attr_type_map = cil_malloc(pdb->p_types.nprim * sizeof(ebitmap_t));
>
> -       uint32_t i = 0;
>         for (i = 0; i < pdb->p_types.nprim; i++) {
>                 ebitmap_init(&pdb->type_attr_map[i]);
>                 ebitmap_init(&pdb->attr_type_map[i]);
> @@ -2657,6 +2657,7 @@ int __cil_constrain_expr_to_sepol_expr_helper(policydb_t *pdb, const struct cil_
>         int rc = SEPOL_ERR;
>         struct cil_list_item *item;
>         enum cil_flavor flavor;
> +       enum cil_flavor cil_op;
>         constraint_expr_t *op, *h1, *h2, *t1, *t2;
>         int is_leaf = CIL_FALSE;
>
> @@ -2673,7 +2674,7 @@ int __cil_constrain_expr_to_sepol_expr_helper(policydb_t *pdb, const struct cil_
>                 goto exit;
>         }
>
> -       enum cil_flavor cil_op = (enum cil_flavor)(uintptr_t)item->data;
> +       cil_op = (enum cil_flavor)(uintptr_t)item->data;
>         switch (cil_op) {
>         case CIL_NOT:
>                 op->expr_type = CEXPR_NOT;
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 42d10c87..9a9bc598 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -5173,6 +5173,7 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
>         char *key = NULL;
>         struct cil_macro *macro = NULL;
>         struct cil_tree_node *macro_content = NULL;
> +       struct cil_tree_node *current_item;
>         enum cil_syntax syntax[] = {
>                 CIL_SYN_STRING,
>                 CIL_SYN_STRING,
> @@ -5195,7 +5196,7 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
>
>         key = parse_current->next->data;
>
> -       struct cil_tree_node *current_item = parse_current->next->next->cl_head;
> +       current_item = parse_current->next->next->cl_head;
>         while (current_item != NULL) {
>                 enum cil_syntax param_syntax[] = {
>                         CIL_SYN_STRING,
> @@ -5205,6 +5206,7 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
>                 int param_syntax_len = sizeof(param_syntax)/sizeof(*param_syntax);
>                 char *kind = NULL;
>                 struct cil_param *param = NULL;
> +               struct cil_list_item *curr_param;
>
>                 rc =__cil_verify_syntax(current_item->cl_head, param_syntax, param_syntax_len);
>                 if (rc != SEPOL_OK) {
> @@ -5263,7 +5265,6 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
>                 }
>
>                 //walk current list and check for duplicate parameters
> -               struct cil_list_item *curr_param;
>                 cil_list_for_each(curr_param, macro->params) {
>                         if (param->str == ((struct cil_param*)curr_param->data)->str) {
>                                 cil_log(CIL_ERR, "Duplicate parameter\n");
> diff --git a/libsepol/cil/src/cil_fqn.c b/libsepol/cil/src/cil_fqn.c
> index 097222a8..46db069b 100644
> --- a/libsepol/cil/src/cil_fqn.c
> +++ b/libsepol/cil/src/cil_fqn.c
> @@ -78,12 +78,13 @@ static int __cil_fqn_qualify_blocks(__attribute__((unused)) hashtab_key_t k, has
>         struct cil_tree_node *node = NODE(datum);
>         int i;
>         int rc = SEPOL_OK;
> +       int newlen;
>
>         if (node->flavor != CIL_BLOCK) {
>                 goto exit;
>         }
>
> -       int newlen = fqn_args->len + strlen(datum->name) + 1;
> +       newlen = fqn_args->len + strlen(datum->name) + 1;
>         if (newlen >= CIL_MAX_NAME_LENGTH) {
>                 cil_log(CIL_INFO, "Fully qualified name for block %s is too long\n", datum->name);
>                 rc = SEPOL_ERR;
> diff --git a/libsepol/cil/src/cil_list.c b/libsepol/cil/src/cil_list.c
> index 4e7843cb..8a426f1f 100644
> --- a/libsepol/cil/src/cil_list.c
> +++ b/libsepol/cil/src/cil_list.c
> @@ -55,15 +55,16 @@ void cil_list_init(struct cil_list **list, enum cil_flavor flavor)
>
>  void cil_list_destroy(struct cil_list **list, unsigned destroy_data)
>  {
> +       struct cil_list_item *item;
> +
>         if (*list == NULL) {
>                 return;
>         }
>
> -       struct cil_list_item *item = (*list)->head;
> -       struct cil_list_item *next = NULL;
> +       item = (*list)->head;
>         while (item != NULL)
>         {
> -               next = item->next;
> +               struct cil_list_item *next = item->next;
>                 if (item->flavor == CIL_LIST) {
>                         cil_list_destroy((struct cil_list**)&(item->data), destroy_data);
>                         free(item);
> diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> index 05842b64..7bca0834 100644
> --- a/libsepol/cil/src/cil_post.c
> +++ b/libsepol/cil/src/cil_post.c
> @@ -213,8 +213,8 @@ int cil_post_filecon_compare(const void *a, const void *b)
>         struct fc_data *a_data = cil_malloc(sizeof(*a_data));
>         struct fc_data *b_data = cil_malloc(sizeof(*b_data));
>         char *a_path = cil_malloc(strlen(a_filecon->path_str) + 1);
> -       a_path[0] = '\0';
>         char *b_path = cil_malloc(strlen(b_filecon->path_str) + 1);
> +       a_path[0] = '\0';
>         b_path[0] = '\0';
>         strcat(a_path, a_filecon->path_str);
>         strcat(b_path, b_filecon->path_str);
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index d8481002..a322b1b7 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -3949,10 +3949,10 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
>                 enum cil_log_level lvl = CIL_ERR;
>
>                 if (optional != NULL) {
> -                       lvl = CIL_INFO;
> -
>                         struct cil_optional *opt = (struct cil_optional *)optional->data;
> -                       struct cil_tree_node *opt_node = NODE(opt);;
> +                       struct cil_tree_node *opt_node = NODE(opt);
> +
> +                       lvl = CIL_INFO;
>                         /* disable an optional if something failed to resolve */
>                         opt->enabled = CIL_FALSE;
>                         cil_tree_log(node, lvl, "Failed to resolve %s statement", cil_node_to_string(node));
> diff --git a/libsepol/cil/src/cil_strpool.c b/libsepol/cil/src/cil_strpool.c
> index 2598bbf3..70bca363 100644
> --- a/libsepol/cil/src/cil_strpool.c
> +++ b/libsepol/cil/src/cil_strpool.c
> @@ -75,9 +75,10 @@ char *cil_strpool_add(const char *str)
>
>         strpool_ref = hashtab_search(cil_strpool_tab, (hashtab_key_t)str);
>         if (strpool_ref == NULL) {
> +               int rc;
>                 strpool_ref = cil_malloc(sizeof(*strpool_ref));
>                 strpool_ref->str = cil_strdup(str);
> -               int rc = hashtab_insert(cil_strpool_tab, (hashtab_key_t)strpool_ref->str, strpool_ref);
> +               rc = hashtab_insert(cil_strpool_tab, (hashtab_key_t)strpool_ref->str, strpool_ref);
>                 if (rc != SEPOL_OK) {
>                         pthread_mutex_unlock(&cil_strpool_mutex);
>                         cil_log(CIL_ERR, "Failed to allocate memory\n");
> --
> 2.32.0
>

Re: [PATCH 07/23] libsepol: follow declaration-after-statement

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Follow the project style of no declaration after statement.
>
> Found by the gcc warning -Wdeclaration-after-statement
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/booleans.c      |  6 ++--
>  libsepol/src/debug.c         |  2 +-
>  libsepol/src/ebitmap.c       | 11 ++++---
>  libsepol/src/module_to_cil.c | 10 +++---
>  libsepol/src/nodes.c         |  6 ++--
>  libsepol/src/services.c      | 59 ++++++++++++++++++------------------
>  libsepol/src/util.c          |  2 +-
>  7 files changed, 50 insertions(+), 46 deletions(-)
>
> diff --git a/libsepol/src/booleans.c b/libsepol/src/booleans.c
> index 30fcf29d..716da6b4 100644
> --- a/libsepol/src/booleans.c
> +++ b/libsepol/src/booleans.c
> @@ -19,6 +19,7 @@ static int bool_update(sepol_handle_t * handle,
>         const char *cname;
>         char *name;
>         int value;
> +       cond_bool_datum_t *datum;
>
>         sepol_bool_key_unpack(key, &cname);
>         name = strdup(cname);
> @@ -27,8 +28,7 @@ static int bool_update(sepol_handle_t * handle,
>         if (!name)
>                 goto omem;
>
> -       cond_bool_datum_t *datum =
> -           hashtab_search(policydb->p_bools.table, name);
> +       datum = hashtab_search(policydb->p_bools.table, name);
>         if (!datum) {
>                 ERR(handle, "boolean %s no longer in policy", name);
>                 goto err;
> @@ -84,10 +84,10 @@ int sepol_bool_set(sepol_handle_t * handle,
>                    const sepol_bool_key_t * key, const sepol_bool_t * data)
>  {
>
> +       policydb_t *policydb = &p->p;
>         const char *name;
>         sepol_bool_key_unpack(key, &name);
>
> -       policydb_t *policydb = &p->p;
>         if (bool_update(handle, policydb, key, data) < 0)
>                 goto err;
>
> diff --git a/libsepol/src/debug.c b/libsepol/src/debug.c
> index 0458e353..f6a59ae7 100644
> --- a/libsepol/src/debug.c
> +++ b/libsepol/src/debug.c
> @@ -44,6 +44,7 @@ void sepol_msg_default_handler(void *varg __attribute__ ((unused)),
>  {
>
>         FILE *stream = NULL;
> +       va_list ap;
>
>         switch (sepol_msg_get_level(handle)) {
>
> @@ -60,7 +61,6 @@ void sepol_msg_default_handler(void *varg __attribute__ ((unused)),
>         fprintf(stream, "%s.%s: ",
>                 sepol_msg_get_channel(handle), sepol_msg_get_fname(handle));
>
> -       va_list ap;
>         va_start(ap, fmt);
>         vfprintf(stream, fmt, ap);
>         va_end(ap);
> diff --git a/libsepol/src/ebitmap.c b/libsepol/src/ebitmap.c
> index 7f425349..522e14a6 100644
> --- a/libsepol/src/ebitmap.c
> +++ b/libsepol/src/ebitmap.c
> @@ -113,9 +113,10 @@ int ebitmap_not(ebitmap_t *dst, ebitmap_t *e1, unsigned int maxbit)
>
>  int ebitmap_andnot(ebitmap_t *dst, ebitmap_t *e1, ebitmap_t *e2, unsigned int maxbit)
>  {
> +       int rc;
>         ebitmap_t e3;
>         ebitmap_init(dst);
> -       int rc = ebitmap_not(&e3, e2, maxbit);
> +       rc = ebitmap_not(&e3, e2, maxbit);
>         if (rc < 0)
>                 return rc;
>         rc = ebitmap_and(dst, e1, &e3);
> @@ -138,13 +139,15 @@ unsigned int ebitmap_cardinality(ebitmap_t *e1)
>
>  int ebitmap_hamming_distance(ebitmap_t * e1, ebitmap_t * e2)
>  {
> +       int rc;
> +       ebitmap_t tmp;
> +       int distance;
>         if (ebitmap_cmp(e1, e2))
>                 return 0;
> -       ebitmap_t tmp;
> -       int rc = ebitmap_xor(&tmp, e1, e2);
> +       rc = ebitmap_xor(&tmp, e1, e2);
>         if (rc < 0)
>                 return -1;
> -       int distance = ebitmap_cardinality(&tmp);
> +       distance = ebitmap_cardinality(&tmp);
>         ebitmap_destroy(&tmp);
>         return distance;
>  }
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index 41605eb8..73ec7971 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -107,8 +107,8 @@ static void cil_printf(const char *fmt, ...) {
>  __attribute__ ((format(printf, 2, 3)))
>  static void cil_println(int indent, const char *fmt, ...)
>  {
> -       cil_indent(indent);
>         va_list argptr;
> +       cil_indent(indent);
>         va_start(argptr, fmt);
>         if (vfprintf(out_file, fmt, argptr) < 0) {
>                 log_err("Failed to write to output");
> @@ -235,12 +235,14 @@ static void role_list_destroy(void)
>
>  static void attr_list_destroy(struct list **attr_list)
>  {
> +       struct list_node *curr;
> +       struct attr_list_node *attr;
> +
>         if (attr_list == NULL || *attr_list == NULL) {
>                 return;
>         }
>
> -       struct list_node *curr = (*attr_list)->head;
> -       struct attr_list_node *attr;
> +       curr = (*attr_list)->head;
>
>         while (curr != NULL) {
>                 attr = curr->data;
> @@ -3525,12 +3527,12 @@ exit:
>  static int additive_scopes_to_cil(int indent, struct policydb *pdb, struct avrule_block *block, struct stack *decl_stack)
>  {
>         int rc = -1;
> +       struct avrule_decl *decl = stack_peek(decl_stack);
>         struct map_args args;
>         args.pdb = pdb;
>         args.block = block;
>         args.decl_stack = decl_stack;
>         args.indent = indent;
> -       struct avrule_decl *decl = stack_peek(decl_stack);
>
>         for (args.sym_index = 0; args.sym_index < SYM_NUM; args.sym_index++) {
>                 if (func_to_cil[args.sym_index] == NULL) {
> diff --git a/libsepol/src/nodes.c b/libsepol/src/nodes.c
> index 820346d0..97a0f959 100644
> --- a/libsepol/src/nodes.c
> +++ b/libsepol/src/nodes.c
> @@ -19,20 +19,20 @@ static int node_from_record(sepol_handle_t * handle,
>         ocontext_t *tmp_node = NULL;
>         context_struct_t *tmp_con = NULL;
>         char *addr_buf = NULL, *mask_buf = NULL;
> +       size_t addr_bsize, mask_bsize;
> +       int proto;
>
>         tmp_node = (ocontext_t *) calloc(1, sizeof(ocontext_t));
>         if (!tmp_node)
>                 goto omem;
>
> -       size_t addr_bsize, mask_bsize;
> -
>         /* Address and netmask */
>         if (sepol_node_get_addr_bytes(handle, data, &addr_buf, &addr_bsize) < 0)
>                 goto err;
>         if (sepol_node_get_mask_bytes(handle, data, &mask_buf, &mask_bsize) < 0)
>                 goto err;
>
> -       int proto = sepol_node_get_proto(data);
> +       proto = sepol_node_get_proto(data);
>
>         switch (proto) {
>         case SEPOL_PROTO_IP4:
> diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> index ff91f7d2..d647c8f5 100644
> --- a/libsepol/src/services.c
> +++ b/libsepol/src/services.c
> @@ -290,6 +290,19 @@ static char *get_class_info(sepol_security_class_t tclass,
>  {
>         constraint_expr_t *e;
>         int mls, state_num;
> +       /* Determine statement type */
> +       const char *statements[] = {
> +               "constrain ",                   /* 0 */
> +               "mlsconstrain ",                /* 1 */
> +               "validatetrans ",               /* 2 */
> +               "mlsvalidatetrans ",    /* 3 */
> +               0 };
> +       size_t class_buf_len = 0;
> +       size_t new_class_buf_len;
> +       size_t buf_used;
> +       int len;
> +       char *class_buf = NULL, *p;
> +       char *new_class_buf = NULL;
>
>         /* Find if MLS statement or not */
>         mls = 0;
> @@ -300,26 +313,11 @@ static char *get_class_info(sepol_security_class_t tclass,
>                 }
>         }
>
> -       /* Determine statement type */
> -       const char *statements[] = {
> -               "constrain ",                   /* 0 */
> -               "mlsconstrain ",                /* 1 */
> -               "validatetrans ",               /* 2 */
> -               "mlsvalidatetrans ",    /* 3 */
> -               0 };
> -
>         if (xcontext == NULL)
>                 state_num = mls + 0;
>         else
>                 state_num = mls + 2;
>
> -       size_t class_buf_len = 0;
> -       size_t new_class_buf_len;
> -       size_t buf_used;
> -       int len;
> -       char *class_buf = NULL, *p;
> -       char *new_class_buf = NULL;
> -
>         while (1) {
>                 new_class_buf_len = class_buf_len + EXPR_BUF_SIZE;
>                 new_class_buf = realloc(class_buf, new_class_buf_len);
> @@ -417,6 +415,8 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
>         char *tgt = NULL;
>         int rc = 0, x;
>         char *class_buf = NULL;
> +       int expr_list_len = 0;
> +       int expr_count;
>
>         /*
>          * The array of expression answer buffer pointers and counter.
> @@ -424,6 +424,11 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
>         char **answer_list = NULL;
>         int answer_counter = 0;
>
> +       /* The pop operands */
> +       char *a;
> +       char *b;
> +       int a_len, b_len;
> +
>         class_buf = get_class_info(tclass, constraint, xcontext);
>         if (!class_buf) {
>                 ERR(NULL, "failed to allocate class buffer");
> @@ -431,7 +436,6 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
>         }
>
>         /* Original function but with buffer support */
> -       int expr_list_len = 0;
>         expr_counter = 0;
>         expr_list = NULL;
>         for (e = constraint->expr; e; e = e->next) {
> @@ -701,7 +705,7 @@ mls_ops:
>          * expr_list malloc's. Normally they are released by the RPN to
>          * infix code.
>          */
> -       int expr_count = expr_counter;
> +       expr_count = expr_counter;
>         expr_counter = 0;
>
>         /*
> @@ -715,11 +719,6 @@ mls_ops:
>                 goto out;
>         }
>
> -       /* The pop operands */
> -       char *a;
> -       char *b;
> -       int a_len, b_len;
> -
>         /* Convert constraint from RPN to infix notation. */
>         for (x = 0; x != expr_count; x++) {
>                 if (strncmp(expr_list[x], "and", 3) == 0 || strncmp(expr_list[x],
> @@ -778,14 +777,6 @@ mls_ops:
>                         xcontext ? "Validatetrans" : "Constraint",
>                         s[0] ? "GRANTED" : "DENIED");
>
> -       int len, new_buf_len;
> -       char *p, **new_buf = r_buf;
> -       /*
> -        * These contain the constraint components that are added to the
> -        * callers reason buffer.
> -        */
> -       const char *buffers[] = { class_buf, a, "); ", tmp_buf, 0 };
> -
>         /*
>          * This will add the constraints to the callers reason buffer (who is
>          * responsible for freeing the memory). It will handle any realloc's
> @@ -796,6 +787,14 @@ mls_ops:
>
>         if (r_buf && ((s[0] == 0) || ((s[0] == 1 &&
>                                 (flags & SHOW_GRANTED) == SHOW_GRANTED)))) {
> +               int len, new_buf_len;
> +               char *p, **new_buf = r_buf;
> +               /*
> +               * These contain the constraint components that are added to the
> +               * callers reason buffer.
> +               */
> +               const char *buffers[] = { class_buf, a, "); ", tmp_buf, 0 };
> +
>                 for (x = 0; buffers[x] != NULL; x++) {
>                         while (1) {
>                                 p = *r_buf + reason_buf_used;
> diff --git a/libsepol/src/util.c b/libsepol/src/util.c
> index d51750af..a47cae87 100644
> --- a/libsepol/src/util.c
> +++ b/libsepol/src/util.c
> @@ -129,9 +129,9 @@ char *sepol_extended_perms_to_string(avtab_extended_perms_t *xperms)
>         unsigned int bit;
>         unsigned int in_range = 0;
>         static char xpermsbuf[2048];
> -       xpermsbuf[0] = '\0';
>         char *p;
>         int len, xpermslen = 0;
> +       xpermsbuf[0] = '\0';
>         p = xpermsbuf;
>
>         if ((xperms->specified != AVTAB_XPERMS_IOCTLFUNCTION)
> --
> 2.32.0
>

Re: [PATCH 16/23] libsepol/cil: drop extra semicolon

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_build_ast.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 9a9bc598..da298311 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -4153,7 +4153,7 @@ void cil_destroy_context(struct cil_context *context)
>                 return;
>         }
>
> -       cil_symtab_datum_destroy(&context->datum);;
> +       cil_symtab_datum_destroy(&context->datum);
>
>         if (context->range_str == NULL && context->range != NULL) {
>                 cil_destroy_levelrange(context->range);
> --
> 2.32.0
>

Re: [PATCH 21/23] libsepol: drop unnecessary casts

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> `hashtab_search()` does take `const_hashtab_key_t` as second parameter,
> which is a typedef for `const char *`.
> Drop the unnecessary and const-violating cast.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/services.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> index f7c31d80..47a3dc14 100644
> --- a/libsepol/src/services.c
> +++ b/libsepol/src/services.c
> @@ -1182,7 +1182,7 @@ int sepol_string_to_security_class(const char *class_name,
>         class_datum_t *tclass_datum;
>
>         tclass_datum = hashtab_search(policydb->p_classes.table,
> -                                     (hashtab_key_t) class_name);
> +                                     class_name);
>         if (!tclass_datum) {
>                 ERR(NULL, "unrecognized class %s", class_name);
>                 return STATUS_ERR;
> @@ -1211,7 +1211,7 @@ int sepol_string_to_av_perm(sepol_security_class_t tclass,
>         /* Check for unique perms then the common ones (if any) */
>         perm_datum = (perm_datum_t *)
>                         hashtab_search(tclass_datum->permissions.table,
> -                       (hashtab_key_t)perm_name);
> +                       perm_name);
>         if (perm_datum != NULL) {
>                 *av = 0x1 << (perm_datum->s.value - 1);
>                 return STATUS_SUCCESS;
> @@ -1222,7 +1222,7 @@ int sepol_string_to_av_perm(sepol_security_class_t tclass,
>
>         perm_datum = (perm_datum_t *)
>                         hashtab_search(tclass_datum->comdatum->permissions.table,
> -                       (hashtab_key_t)perm_name);
> +                       perm_name);
>
>         if (perm_datum != NULL) {
>                 *av = 0x1 << (perm_datum->s.value - 1);
> --
> 2.32.0
>

Re: [PATCH 06/23] libsepol: avoid unsigned integer overflow

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Unsigned integer overflow is well-defined and not undefined behavior.
> But it is still useful to enable undefined behavior sanitizer checks on
> unsigned arithmetic to detect possible issues on counters or variables
> with similar purpose.
>
> Use a spaceship operator like comparison instead of subtraction.
>
> Modern compilers will generate a single comparison instruction instead
> of actually perform the subtraction.
>
> policydb.c:826:17: runtime error: unsigned integer overflow: 24 - 1699 cannot be represented in type 'unsigned int'
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/policydb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index cbe0c432..3389a943 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -823,11 +823,11 @@ static int filenametr_cmp(hashtab_t h __attribute__ ((unused)),
>         const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2;
>         int v;
>
> -       v = ft1->ttype - ft2->ttype;
> +       v = (ft1->ttype > ft2->ttype) - (ft1->ttype < ft2->ttype);
>         if (v)
>                 return v;
>
> -       v = ft1->tclass - ft2->tclass;
> +       v = (ft1->tclass > ft2->tclass) - (ft1->tclass < ft2->tclass);
>         if (v)
>                 return v;
>
> --
> 2.32.0
>

Re: [PATCH 15/23] libsepol/cil: silence cast warning

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> ../cil/src/cil_write_ast.c:86:32: error: cast to smaller integer type 'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
>                         enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
>                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../cil/src/cil_write_ast.c:130:37: error: cast to smaller integer type 'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
>                         enum cil_flavor operand_flavor = (enum cil_flavor)curr->data;
>                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Silence this warning by casting the pointer to an integer the cast to
> enum cil_flavor.
>
> See 32f8ed3d6b0b ("libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast")
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_write_ast.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_write_ast.c b/libsepol/cil/src/cil_write_ast.c
> index 4871f704..186070c1 100644
> --- a/libsepol/cil/src/cil_write_ast.c
> +++ b/libsepol/cil/src/cil_write_ast.c
> @@ -83,7 +83,7 @@ static void write_expr(FILE *out, struct cil_list *expr)
>                         break;
>                 case CIL_OP: {
>                         const char *op_str;
> -                       enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
> +                       enum cil_flavor op_flavor = (enum cil_flavor)(uintptr_t)curr->data;
>                         switch (op_flavor) {
>                         case CIL_AND:
>                                 op_str = CIL_KEY_AND;
> @@ -127,7 +127,7 @@ static void write_expr(FILE *out, struct cil_list *expr)
>                 }
>                 case CIL_CONS_OPERAND: {
>                         const char *operand_str;
> -                       enum cil_flavor operand_flavor = (enum cil_flavor)curr->data;
> +                       enum cil_flavor operand_flavor = (enum cil_flavor)(uintptr_t)curr->data;
>                         switch (operand_flavor) {
>                         case CIL_CONS_U1:
>                                 operand_str = CIL_KEY_CONS_U1;
> --
> 2.32.0
>

Re: [PATCH 11/23] libsepol: mark read-only parameters of type_set_ interfaces const

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Make it more obvious which parameters are read-only and not being
> modified and allow callers to pass const pointers.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/include/sepol/policydb/policydb.h | 4 ++--
>  libsepol/src/policydb.c                    | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index c29339dc..78699fb4 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -667,8 +667,8 @@ extern int scope_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p);
>  extern void class_perm_node_init(class_perm_node_t * x);
>  extern void type_set_init(type_set_t * x);
>  extern void type_set_destroy(type_set_t * x);
> -extern int type_set_cpy(type_set_t * dst, type_set_t * src);
> -extern int type_set_or_eq(type_set_t * dst, type_set_t * other);
> +extern int type_set_cpy(type_set_t * dst, const type_set_t * src);
> +extern int type_set_or_eq(type_set_t * dst, const type_set_t * other);
>  extern void role_set_init(role_set_t * x);
>  extern void role_set_destroy(role_set_t * x);
>  extern void avrule_init(avrule_t * x);
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index 3389a943..7739b0fb 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -1753,7 +1753,7 @@ int symtab_insert(policydb_t * pol, uint32_t sym,
>         return retval;
>  }
>
> -static int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
> +static int type_set_or(type_set_t * dst, const type_set_t * a, const type_set_t * b)
>  {
>         type_set_init(dst);
>
> @@ -1770,7 +1770,7 @@ static int type_set_or(type_set_t * dst, type_set_t * a, type_set_t * b)
>         return 0;
>  }
>
> -int type_set_cpy(type_set_t * dst, type_set_t * src)
> +int type_set_cpy(type_set_t * dst, const type_set_t * src)
>  {
>         type_set_init(dst);
>
> @@ -1783,7 +1783,7 @@ int type_set_cpy(type_set_t * dst, type_set_t * src)
>         return 0;
>  }
>
> -int type_set_or_eq(type_set_t * dst, type_set_t * other)
> +int type_set_or_eq(type_set_t * dst, const type_set_t * other)
>  {
>         int ret;
>         type_set_t tmp;
> --
> 2.32.0
>

Re: [PATCH 12/23] libsepol: do not allocate memory of size 0

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> In case cats_ebitmap_len() returns 0, do not allocate but quit.
>
> Found by clang-analyzer
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/kernel_to_cil.c  | 5 ++++-
>  libsepol/src/kernel_to_conf.c | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index 989aacde..17b5ebf0 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -1034,11 +1034,14 @@ static char *cats_ebitmap_to_str(struct ebitmap *cats, char **val_to_name)
>  {
>         struct ebitmap_node *node;
>         uint32_t i, start, range;
> -       char *catsbuf, *p;
> +       char *catsbuf = NULL, *p;
>         const char *fmt;
>         int len, remaining;
>
>         remaining = (int)cats_ebitmap_len(cats, val_to_name);
> +       if (remaining == 0) {
> +               goto exit;
> +       }
>         catsbuf = malloc(remaining);
>         if (!catsbuf) {
>                 goto exit;
> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
> index 5db47fe4..c1253820 100644
> --- a/libsepol/src/kernel_to_conf.c
> +++ b/libsepol/src/kernel_to_conf.c
> @@ -1025,12 +1025,15 @@ static char *cats_ebitmap_to_str(struct ebitmap *cats, char **val_to_name)
>  {
>         struct ebitmap_node *node;
>         uint32_t i, start, range, first;
> -       char *catsbuf, *p;
> +       char *catsbuf = NULL, *p;
>         const char *fmt;
>         char sep;
>         int len, remaining;
>
>         remaining = (int)cats_ebitmap_len(cats, val_to_name);
> +       if (remaining == 0) {
> +               goto exit;
> +       }
>         catsbuf = malloc(remaining);
>         if (!catsbuf) {
>                 goto exit;
> --
> 2.32.0
>

Re: [PATCH 23/23] libsepol: declare read-only arrays const

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Make it more apparent that those data does not change and enforce it.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/avrule_block.c      |  2 +-
>  libsepol/src/avtab.c             |  2 +-
>  libsepol/src/link.c              |  2 +-
>  libsepol/src/polcaps.c           |  2 +-
>  libsepol/src/policydb.c          | 22 +++++++++++-----------
>  libsepol/src/policydb_internal.h |  2 +-
>  libsepol/src/private.h           |  6 +++---
>  libsepol/src/write.c             |  8 ++++----
>  8 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/libsepol/src/avrule_block.c b/libsepol/src/avrule_block.c
> index a9832d0d..dcfce8b8 100644
> --- a/libsepol/src/avrule_block.c
> +++ b/libsepol/src/avrule_block.c
> @@ -30,7 +30,7 @@
>  /* It is anticipated that there be less declarations within an avrule
>   * block than the global policy.  Thus the symbol table sizes are
>   * smaller than those listed in policydb.c */
> -static unsigned int symtab_sizes[SYM_NUM] = {
> +static const unsigned int symtab_sizes[SYM_NUM] = {
>         2,
>         4,
>         8,
> diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
> index c2ccb005..7b80377c 100644
> --- a/libsepol/src/avtab.c
> +++ b/libsepol/src/avtab.c
> @@ -424,7 +424,7 @@ void avtab_hash_eval(avtab_t * h, char *tag)
>  }
>
>  /* Ordering of datums in the original avtab format in the policy file. */
> -static uint16_t spec_order[] = {
> +static const uint16_t spec_order[] = {
>         AVTAB_ALLOWED,
>         AVTAB_AUDITDENY,
>         AVTAB_AUDITALLOW,
> diff --git a/libsepol/src/link.c b/libsepol/src/link.c
> index bdc1fcbf..461d2feb 100644
> --- a/libsepol/src/link.c
> +++ b/libsepol/src/link.c
> @@ -78,7 +78,7 @@ typedef struct missing_requirement {
>         uint32_t perm_value;
>  } missing_requirement_t;
>
> -static const char *symtab_names[SYM_NUM] = {
> +static const char * const symtab_names[SYM_NUM] = {
>         "common", "class", "role", "type/attribute", "user",
>         "bool", "level", "category"
>  };
> diff --git a/libsepol/src/polcaps.c b/libsepol/src/polcaps.c
> index 67ed5786..6a74ec7d 100644
> --- a/libsepol/src/polcaps.c
> +++ b/libsepol/src/polcaps.c
> @@ -5,7 +5,7 @@
>  #include <string.h>
>  #include <sepol/policydb/polcaps.h>
>
> -static const char *polcap_names[] = {
> +static const char * const polcap_names[] = {
>         "network_peer_controls",        /* POLICYDB_CAPABILITY_NETPEER */
>         "open_perms",                   /* POLICYDB_CAPABILITY_OPENPERM */
>         "extended_socket_class",        /* POLICYDB_CAPABILITY_EXTSOCKCLASS */
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index 7739b0fb..0bc0ead8 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -57,10 +57,10 @@
>  #include "policydb_validate.h"
>
>  #define POLICYDB_TARGET_SZ   ARRAY_SIZE(policydb_target_strings)
> -const char *policydb_target_strings[] = { POLICYDB_STRING, POLICYDB_XEN_STRING };
> +const char * const policydb_target_strings[] = { POLICYDB_STRING, POLICYDB_XEN_STRING };
>
>  /* These need to be updated if SYM_NUM or OCON_NUM changes */
> -static struct policydb_compat_info policydb_compat[] = {
> +static const struct policydb_compat_info policydb_compat[] = {
>         {
>          .type = POLICY_KERN,
>          .version = POLICYDB_VERSION_BOUNDARY,
> @@ -460,7 +460,7 @@ static char *symtab_name[SYM_NUM] = {
>  };
>  #endif
>
> -static unsigned int symtab_sizes[SYM_NUM] = {
> +static const unsigned int symtab_sizes[SYM_NUM] = {
>         2,
>         32,
>         16,
> @@ -471,12 +471,12 @@ static unsigned int symtab_sizes[SYM_NUM] = {
>         16,
>  };
>
> -struct policydb_compat_info *policydb_lookup_compat(unsigned int version,
> -                                                   unsigned int type,
> -                                               unsigned int target_platform)
> +const struct policydb_compat_info *policydb_lookup_compat(unsigned int version,
> +                                                         unsigned int type,
> +                                                         unsigned int target_platform)
>  {
>         unsigned int i;
> -       struct policydb_compat_info *info = NULL;
> +       const struct policydb_compat_info *info = NULL;
>
>         for (i = 0; i < sizeof(policydb_compat) / sizeof(*info); i++) {
>                 if (policydb_compat[i].version == version &&
> @@ -2854,7 +2854,7 @@ static int filename_trans_read(policydb_t *p, struct policy_file *fp)
>         return 0;
>  }
>
> -static int ocontext_read_xen(struct policydb_compat_info *info,
> +static int ocontext_read_xen(const struct policydb_compat_info *info,
>         policydb_t *p, struct policy_file *fp)
>  {
>         unsigned int i, j;
> @@ -2963,7 +2963,7 @@ static int ocontext_read_xen(struct policydb_compat_info *info,
>         }
>         return 0;
>  }
> -static int ocontext_read_selinux(struct policydb_compat_info *info,
> +static int ocontext_read_selinux(const struct policydb_compat_info *info,
>                          policydb_t * p, struct policy_file *fp)
>  {
>         unsigned int i, j;
> @@ -3141,7 +3141,7 @@ static int ocontext_read_selinux(struct policydb_compat_info *info,
>         return 0;
>  }
>
> -static int ocontext_read(struct policydb_compat_info *info,
> +static int ocontext_read(const struct policydb_compat_info *info,
>         policydb_t *p, struct policy_file *fp)
>  {
>         int rc = -1;
> @@ -4198,7 +4198,7 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose)
>         uint32_t buf[5];
>         size_t len, nprim, nel;
>         char *policydb_str;
> -       struct policydb_compat_info *info;
> +       const struct policydb_compat_info *info;
>         unsigned int policy_type, bufindex;
>         ebitmap_node_t *tnode;
>         int rc;
> diff --git a/libsepol/src/policydb_internal.h b/libsepol/src/policydb_internal.h
> index 06ba5c8b..dd8f25d0 100644
> --- a/libsepol/src/policydb_internal.h
> +++ b/libsepol/src/policydb_internal.h
> @@ -3,5 +3,5 @@
>
>  #include <sepol/policydb.h>
>
> -extern const char *policydb_target_strings[];
> +extern const char * const policydb_target_strings[];
>  #endif
> diff --git a/libsepol/src/private.h b/libsepol/src/private.h
> index f5b5277f..72f21262 100644
> --- a/libsepol/src/private.h
> +++ b/libsepol/src/private.h
> @@ -56,9 +56,9 @@ struct policydb_compat_info {
>         unsigned int target_platform;
>  };
>
> -extern struct policydb_compat_info *policydb_lookup_compat(unsigned int version,
> -                                                          unsigned int type,
> -                                               unsigned int target_platform);
> +extern const struct policydb_compat_info *policydb_lookup_compat(unsigned int version,
> +                                                                unsigned int type,
> +                                                                unsigned int target_platform);
>
>  /* Reading from a policy "file". */
>  extern int next_entry(void *buf, struct policy_file *fp, size_t bytes);
> diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> index 84bcaf3f..3bd034d6 100644
> --- a/libsepol/src/write.c
> +++ b/libsepol/src/write.c
> @@ -1345,7 +1345,7 @@ static int (*write_f[SYM_NUM]) (hashtab_key_t key, hashtab_datum_t datum,
>  common_write, class_write, role_write, type_write, user_write,
>             cond_write_bool, sens_write, cat_write,};
>
> -static int ocontext_write_xen(struct policydb_compat_info *info, policydb_t *p,
> +static int ocontext_write_xen(const struct policydb_compat_info *info, policydb_t *p,
>                           struct policy_file *fp)
>  {
>         unsigned int i, j;
> @@ -1453,7 +1453,7 @@ static int ocontext_write_xen(struct policydb_compat_info *info, policydb_t *p,
>         return POLICYDB_SUCCESS;
>  }
>
> -static int ocontext_write_selinux(struct policydb_compat_info *info,
> +static int ocontext_write_selinux(const struct policydb_compat_info *info,
>         policydb_t *p, struct policy_file *fp)
>  {
>         unsigned int i, j;
> @@ -1583,7 +1583,7 @@ static int ocontext_write_selinux(struct policydb_compat_info *info,
>         return POLICYDB_SUCCESS;
>  }
>
> -static int ocontext_write(struct policydb_compat_info *info, policydb_t * p,
> +static int ocontext_write(const struct policydb_compat_info *info, policydb_t * p,
>         struct policy_file *fp)
>  {
>         int rc = POLICYDB_ERROR;
> @@ -2179,7 +2179,7 @@ int policydb_write(policydb_t * p, struct policy_file *fp)
>         unsigned int i, num_syms;
>         uint32_t buf[32], config;
>         size_t items, items2, len;
> -       struct policydb_compat_info *info;
> +       const struct policydb_compat_info *info;
>         struct policy_data pd;
>         const char *policydb_str;
>
> --
> 2.32.0
>

Re: [PATCH 19/23] libsepol/cil: avoid using maybe uninitialized variables

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Initialize variables, as they are set after goto statements, which jump
> to cleanup code using them.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_binary.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 601fe8d1..54d13f2f 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -1073,7 +1073,7 @@ int __cil_type_rule_to_avtab(policydb_t *pdb, const struct cil_db *db, struct ci
>         type_datum_t *sepol_src = NULL;
>         type_datum_t *sepol_tgt = NULL;
>         class_datum_t *sepol_obj = NULL;
> -       struct cil_list *class_list;
> +       struct cil_list *class_list = NULL;
>         type_datum_t *sepol_result = NULL;
>         ebitmap_t src_bitmap, tgt_bitmap;
>         ebitmap_node_t *node1, *node2;
> @@ -1129,7 +1129,7 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
>         type_datum_t *sepol_src = NULL;
>         type_datum_t *sepol_tgt = NULL;
>         class_datum_t *sepol_obj = NULL;
> -       struct cil_list *class_list;
> +       struct cil_list *class_list = NULL;
>         type_datum_t *sepol_result = NULL;
>         ebitmap_t src_bitmap, tgt_bitmap;
>         ebitmap_node_t *node1, *node2;
> @@ -2338,7 +2338,7 @@ int cil_roletrans_to_policydb(policydb_t *pdb, const struct cil_db *db, struct c
>         role_datum_t *sepol_src = NULL;
>         type_datum_t *sepol_tgt = NULL;
>         class_datum_t *sepol_obj = NULL;
> -       struct cil_list *class_list;
> +       struct cil_list *class_list = NULL;
>         role_datum_t *sepol_result = NULL;
>         role_trans_t *new = NULL;
>         uint32_t *new_role = NULL;
> @@ -3166,7 +3166,7 @@ int cil_rangetransition_to_policydb(policydb_t *pdb, const struct cil_db *db, st
>         type_datum_t *sepol_src = NULL;
>         type_datum_t *sepol_tgt = NULL;
>         class_datum_t *sepol_class = NULL;
> -       struct cil_list *class_list;
> +       struct cil_list *class_list = NULL;
>         range_trans_t *newkey = NULL;
>         struct mls_range *newdatum = NULL;
>         ebitmap_t src_bitmap, tgt_bitmap;
> @@ -3603,7 +3603,7 @@ int cil_default_to_policydb(policydb_t *pdb, struct cil_default *def)
>  {
>         struct cil_list_item *curr;
>         class_datum_t *sepol_class;
> -       struct cil_list *class_list;
> +       struct cil_list *class_list = NULL;
>
>         cil_list_for_each(curr, def->class_datums) {
>                 struct cil_list_item *c;
> @@ -3658,7 +3658,7 @@ int cil_defaultrange_to_policydb(policydb_t *pdb, struct cil_defaultrange *def)
>  {
>         struct cil_list_item *curr;
>         class_datum_t *sepol_class;
> -       struct cil_list *class_list;
> +       struct cil_list *class_list = NULL;
>
>         cil_list_for_each(curr, def->class_datums) {
>                 struct cil_list_item *c;
> --
> 2.32.0
>

Re: [PATCH 22/23] libsepol: declare file local variable static

[James Carter]

On Tue, Jun 8, 2021 at 12:02 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Clang issues:
>
>     module_to_cil.c:65:7: warning: no previous extern declaration for non-static variable 'out_file' [-Wmissing-variable-declarations]
>     FILE *out_file;
>           ^
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/module_to_cil.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index 73ec7971..1d724b91 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -62,7 +62,7 @@
>  #  define UNUSED(x) UNUSED_ ## x
>  #endif
>
> -FILE *out_file;
> +static FILE *out_file;
>
>  #define STACK_SIZE 16
>  #define DEFAULT_LEVEL "systemlow"
> --
> 2.32.0
>

Re: [PATCH 00/23] libsepol: miscellaneous cleanup

[James Carter]

On Tue, Jun 8, 2021 at 12:00 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Clean up several code smells, compiler warnings, static analyzer issues
> and UBSAN findings in libsepol.
>
> Also declare local functions and variables static and some interface
> parameters const to improve maintainability.
>
> Christian Göttsche (23):
>   libsepol: fix typos
>   libsepol: resolve missing prototypes
>   libsepol: remove unused functions
>   libsepol: ignore UBSAN false-positives
>   libsepol: avoid implicit conversions
>   libsepol: avoid unsigned integer overflow
>   libsepol: follow declaration-after-statement
>   libsepol/cil: follow declaration-after-statement
>   libsepol: remove dead stores
>   libsepol: mark read-only parameters of ebitmap interfaces const
>   libsepol: mark read-only parameters of type_set_ interfaces const
>   libsepol: do not allocate memory of size 0
>   libsepol: assure string NUL-termination
>   libsepol: remove dead stores
>   libsepol/cil: silence cast warning
>   libsepol/cil: drop extra semicolon
>   libsepol/cil: drop dead store
>   libsepol/cil: drop unnecessary casts
>   libsepol/cil: avoid using maybe uninitialized variables
>   libsepol: drop repeated semicolons
>   libsepol: drop unnecessary casts
>   libsepol: declare file local variable static
>   libsepol: declare read-only arrays const
>
>  libsepol/cil/src/cil_binary.c                 |  19 +--
>  libsepol/cil/src/cil_build_ast.c              |   9 +-
>  libsepol/cil/src/cil_fqn.c                    |   3 +-
>  libsepol/cil/src/cil_list.c                   |   7 +-
>  libsepol/cil/src/cil_post.c                   |   2 +-
>  libsepol/cil/src/cil_resolve_ast.c            |   8 +-
>  libsepol/cil/src/cil_strpool.c                |  16 +--
>  libsepol/cil/src/cil_write_ast.c              |   4 +-
>  libsepol/include/sepol/policydb/conditional.h |   2 +-
>  libsepol/include/sepol/policydb/ebitmap.h     |  16 +--
>  libsepol/include/sepol/policydb/policydb.h    |   6 +-
>  libsepol/src/assertion.c                      |   2 +-
>  libsepol/src/avrule_block.c                   |   2 +-
>  libsepol/src/avtab.c                          |   8 +-
>  libsepol/src/booleans.c                       |   6 +-
>  libsepol/src/conditional.c                    |   3 -
>  libsepol/src/context_internal.h               |   1 +
>  libsepol/src/debug.c                          |   2 +-
>  libsepol/src/ebitmap.c                        |  29 +++--
>  libsepol/src/expand.c                         |   8 +-
>  libsepol/src/ibendport_record.c               |   4 +-
>  libsepol/src/kernel_to_cil.c                  |   7 +-
>  libsepol/src/kernel_to_conf.c                 |   5 +-
>  libsepol/src/link.c                           |   2 +-
>  libsepol/src/module.c                         |   2 +-
>  libsepol/src/module_to_cil.c                  |  14 +-
>  libsepol/src/nodes.c                          |   6 +-
>  libsepol/src/polcaps.c                        |   2 +-
>  libsepol/src/policydb.c                       |  70 +++++-----
>  libsepol/src/policydb_internal.h              |   2 +-
>  libsepol/src/policydb_validate.c              |   2 +-
>  libsepol/src/private.h                        |   6 +-
>  libsepol/src/services.c                       | 120 ++++++------------
>  libsepol/src/sidtab.c                         |  31 -----
>  libsepol/src/symtab.c                         |   6 +
>  libsepol/src/util.c                           |   2 +-
>  libsepol/src/write.c                          |   8 +-
>  37 files changed, 187 insertions(+), 255 deletions(-)
>
> --
> 2.32.0
>

Merged all the patches except for 0004, 0005, and 0013 which still
have outstanding comments.
Thanks,
Jim

[PATCH v2 1/3] libsepol: ignore UBSAN false-positives

[Christian Göttsche]

Unsigned integer overflow is well-defined and not undefined behavior.
But it is still useful to enable undefined behavior sanitizer checks on
unsigned arithmetic to detect possible issues on counters or variables
with similar purpose.

Annotate functions, in which unsigned overflows are expected to happen,
with the respective Clang function attribute[1].
GCC does not support sanitizing unsigned integer arithmetic[2].

    avtab.c:76:2: runtime error: unsigned integer overflow: 6 * 3432918353 cannot be represented in type 'unsigned int'
    policydb.c:795:42: runtime error: unsigned integer overflow: 8160943042179512010 * 11 cannot be represented in type 'unsigned long'
    symtab.c:25:12: runtime error: left shift of 1766601759 by 4 places cannot be represented in type 'unsigned int'

[1]: https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
[2]: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - use a common macro as suggested by Ondrej Mosnacek
  - mention that GCC does not support unsigned integer sanitation

 libsepol/src/avtab.c    |  1 +
 libsepol/src/policydb.c |  1 +
 libsepol/src/private.h  | 11 +++++++++++
 libsepol/src/symtab.c   |  4 ++++
 4 files changed, 17 insertions(+)

diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
index 88e9d510..37e8af07 100644
--- a/libsepol/src/avtab.c
+++ b/libsepol/src/avtab.c
@@ -52,6 +52,7 @@
 /* Based on MurmurHash3, written by Austin Appleby and placed in the
  * public domain.
  */
+ignore_unsigned_overflow_
 static inline int avtab_hash(struct avtab_key *keyp, uint32_t mask)
 {
 	static const uint32_t c1 = 0xcc9e2d51;
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index ef2217c2..0721c81e 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -789,6 +789,7 @@ static int roles_init(policydb_t * p)
 	goto out;
 }
 
+ignore_unsigned_overflow_
 static inline unsigned long
 partial_name_hash(unsigned long c, unsigned long prevhash)
 {
diff --git a/libsepol/src/private.h b/libsepol/src/private.h
index 72f21262..e54cefdb 100644
--- a/libsepol/src/private.h
+++ b/libsepol/src/private.h
@@ -47,6 +47,17 @@
 #define is_saturated(x) (x == (typeof(x))-1)
 #define zero_or_saturated(x) ((x == 0) || is_saturated(x))
 
+/* Use to ignore intentional unsigned under- and overflows while running under UBSAN. */
+#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
+#if (__clang_major__ >= 12)
+#define ignore_unsigned_overflow_        __attribute__((no_sanitize("unsigned-integer-overflow", "unsigned-shift-base")))
+#else
+#define ignore_unsigned_overflow_        __attribute__((no_sanitize("unsigned-integer-overflow")))
+#endif
+#else
+#define ignore_unsigned_overflow_
+#endif
+
 /* Policy compatibility information. */
 struct policydb_compat_info {
 	unsigned int type;
diff --git a/libsepol/src/symtab.c b/libsepol/src/symtab.c
index 9a417ca2..a6061851 100644
--- a/libsepol/src/symtab.c
+++ b/libsepol/src/symtab.c
@@ -8,9 +8,13 @@
  */
 
 #include <string.h>
+
+#include "private.h"
+
 #include <sepol/policydb/hashtab.h>
 #include <sepol/policydb/symtab.h>
 
+ignore_unsigned_overflow_
 static unsigned int symhash(hashtab_t h, const_hashtab_key_t key)
 {
 	const char *p, *keyp;
-- 
2.32.0

[PATCH v2 2/3] libsepol: avoid implicit conversions

[Christian Göttsche]

Avoid implicit conversions from signed to unsigned values, found by
UB sanitizers, by using unsigned values in the first place.

    expand.c:1644:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)

    expand.c:2892:24: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned int' changed the value to 4294967294 (32-bit, unsigned)

    policy_define.c:2344:4: runtime error: implicit conversion from type 'int' of value -1048577 (32-bit, signed) to type 'unsigned int' changed the value to 4293918719 (32-bit, unsigned)

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - use UINT32_C as suggested by Ondrej Mosnacek 

 libsepol/include/sepol/policydb/conditional.h | 2 +-
 libsepol/include/sepol/policydb/policydb.h    | 6 +++---
 libsepol/src/expand.c                         | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libsepol/include/sepol/policydb/conditional.h b/libsepol/include/sepol/policydb/conditional.h
index 9c3df3ef..49c0d766 100644
--- a/libsepol/include/sepol/policydb/conditional.h
+++ b/libsepol/include/sepol/policydb/conditional.h
@@ -90,7 +90,7 @@ typedef struct cond_node {
 	uint32_t expr_pre_comp;
 	struct cond_node *next;
 	/* a tunable conditional, calculated and used at expansion */
-#define	COND_NODE_FLAGS_TUNABLE	0x01
+#define	COND_NODE_FLAGS_TUNABLE	UINT32_C(0x01)
 	uint32_t flags;
 } cond_node_t;
 
diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index 6976ef48..4bf9f05d 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -251,9 +251,9 @@ typedef struct class_perm_node {
 	struct class_perm_node *next;
 } class_perm_node_t;
 
-#define xperm_test(x, p) (1 & (p[x >> 5] >> (x & 0x1f)))
-#define xperm_set(x, p) (p[x >> 5] |= (1 << (x & 0x1f)))
-#define xperm_clear(x, p) (p[x >> 5] &= ~(1 << (x & 0x1f)))
+#define xperm_test(x, p) (UINT32_C(1) & (p[x >> 5] >> (x & 0x1f)))
+#define xperm_set(x, p) (p[x >> 5] |= (UINT32_C(1) << (x & 0x1f)))
+#define xperm_clear(x, p) (p[x >> 5] &= ~(UINT32_C(1) << (x & 0x1f)))
 #define EXTENDED_PERMS_LEN 8
 
 typedef struct av_extended_perms {
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 84bfcfa3..aac5b35f 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1641,7 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
 		 * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
 		 * others. Initialize the data accordingly.
 		 */
-		avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
+		avdatum.data = key->specified == AVTAB_AUDITDENY ? ~UINT32_C(0) : UINT32_C(0);
 		/* this is used to get the node - insertion is actually unique */
 		node = avtab_insert_nonunique(avtab, key, &avdatum);
 		if (!node) {
-- 
2.32.0

[PATCH v2 3/3] libsepol: assure string NUL-termination of ibdev_name

[Christian Göttsche]

Clang complains:

    ibendport_record.c: In function ‘sepol_ibendport_get_ibdev_name’:
    ibendport_record.c:169:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
      169 |  strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
          |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ibendport_record.c: In function ‘sepol_ibendport_set_ibdev_name’:
    ibendport_record.c:189:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
      189 |  strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
          |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

strncpy(3) does not NUL-terminate the destination if the source is of
the same length or longer then the specified size.
The source of these copies are retrieved from
sepol_ibendport_alloc_ibdev_name(), which allocates a fixed amount of
IB_DEVICE_NAME_MAX bytes.
Reduce the size to copy by 1 of all memory regions allocated by
sepol_ibendport_alloc_ibdev_name().

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - use at all affected places as pointed out by James Carter

 libsepol/src/ibendport_record.c | 8 ++++----
 libsepol/src/ibendports.c       | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libsepol/src/ibendport_record.c b/libsepol/src/ibendport_record.c
index adf67161..1eb50914 100644
--- a/libsepol/src/ibendport_record.c
+++ b/libsepol/src/ibendport_record.c
@@ -62,7 +62,7 @@ int sepol_ibendport_key_create(sepol_handle_t *handle,
 	if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_key->ibdev_name) < 0)
 		goto err;
 
-	strncpy(tmp_key->ibdev_name, ibdev_name, IB_DEVICE_NAME_MAX);
+	strncpy(tmp_key->ibdev_name, ibdev_name, IB_DEVICE_NAME_MAX - 1);
 	tmp_key->port = port;
 
 	*key_ptr = tmp_key;
@@ -166,7 +166,7 @@ int sepol_ibendport_get_ibdev_name(sepol_handle_t *handle,
 	if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_ibdev_name) < 0)
 		goto err;
 
-	strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
+	strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
 	*ibdev_name = tmp_ibdev_name;
 	return STATUS_SUCCESS;
 
@@ -186,7 +186,7 @@ int sepol_ibendport_set_ibdev_name(sepol_handle_t *handle,
 	if (sepol_ibendport_alloc_ibdev_name(handle, &tmp) < 0)
 		goto err;
 
-	strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
+	strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX - 1);
 	free(ibendport->ibdev_name);
 	ibendport->ibdev_name = tmp;
 	return STATUS_SUCCESS;
@@ -230,7 +230,7 @@ int sepol_ibendport_clone(sepol_handle_t *handle,
 	if (sepol_ibendport_alloc_ibdev_name(handle, &new_ibendport->ibdev_name) < 0)
 		goto omem;
 
-	strncpy(new_ibendport->ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
+	strncpy(new_ibendport->ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
 	new_ibendport->port = ibendport->port;
 
 	if (ibendport->con &&
diff --git a/libsepol/src/ibendports.c b/libsepol/src/ibendports.c
index 6d56c9a1..ee5cb193 100644
--- a/libsepol/src/ibendports.c
+++ b/libsepol/src/ibendports.c
@@ -34,7 +34,7 @@ static int ibendport_from_record(sepol_handle_t *handle,
 					   &ibdev_name) < 0)
 		goto err;
 
-	strncpy(tmp_ibendport->u.ibendport.dev_name, ibdev_name, IB_DEVICE_NAME_MAX);
+	strncpy(tmp_ibendport->u.ibendport.dev_name, ibdev_name, IB_DEVICE_NAME_MAX - 1);
 
 	free(ibdev_name);
 	ibdev_name = NULL;
-- 
2.32.0

Re: [PATCH v2 1/3] libsepol: ignore UBSAN false-positives

[Nicolas Iooss]

On Thu, Jul 1, 2021 at 8:06 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Unsigned integer overflow is well-defined and not undefined behavior.
> But it is still useful to enable undefined behavior sanitizer checks on
> unsigned arithmetic to detect possible issues on counters or variables
> with similar purpose.
>
> Annotate functions, in which unsigned overflows are expected to happen,
> with the respective Clang function attribute[1].
> GCC does not support sanitizing unsigned integer arithmetic[2].
>
>     avtab.c:76:2: runtime error: unsigned integer overflow: 6 * 3432918353 cannot be represented in type 'unsigned int'
>     policydb.c:795:42: runtime error: unsigned integer overflow: 8160943042179512010 * 11 cannot be represented in type 'unsigned long'
>     symtab.c:25:12: runtime error: left shift of 1766601759 by 4 places cannot be represented in type 'unsigned int'
>
> [1]: https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
> [2]: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

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

Thanks,
Nicolas
> ---
> v2:
>   - use a common macro as suggested by Ondrej Mosnacek
>   - mention that GCC does not support unsigned integer sanitation
>
>  libsepol/src/avtab.c    |  1 +
>  libsepol/src/policydb.c |  1 +
>  libsepol/src/private.h  | 11 +++++++++++
>  libsepol/src/symtab.c   |  4 ++++
>  4 files changed, 17 insertions(+)
>
> diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
> index 88e9d510..37e8af07 100644
> --- a/libsepol/src/avtab.c
> +++ b/libsepol/src/avtab.c
> @@ -52,6 +52,7 @@
>  /* Based on MurmurHash3, written by Austin Appleby and placed in the
>   * public domain.
>   */
> +ignore_unsigned_overflow_
>  static inline int avtab_hash(struct avtab_key *keyp, uint32_t mask)
>  {
>         static const uint32_t c1 = 0xcc9e2d51;
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index ef2217c2..0721c81e 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -789,6 +789,7 @@ static int roles_init(policydb_t * p)
>         goto out;
>  }
>
> +ignore_unsigned_overflow_
>  static inline unsigned long
>  partial_name_hash(unsigned long c, unsigned long prevhash)
>  {
> diff --git a/libsepol/src/private.h b/libsepol/src/private.h
> index 72f21262..e54cefdb 100644
> --- a/libsepol/src/private.h
> +++ b/libsepol/src/private.h
> @@ -47,6 +47,17 @@
>  #define is_saturated(x) (x == (typeof(x))-1)
>  #define zero_or_saturated(x) ((x == 0) || is_saturated(x))
>
> +/* Use to ignore intentional unsigned under- and overflows while running under UBSAN. */
> +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> +#if (__clang_major__ >= 12)
> +#define ignore_unsigned_overflow_        __attribute__((no_sanitize("unsigned-integer-overflow", "unsigned-shift-base")))
> +#else
> +#define ignore_unsigned_overflow_        __attribute__((no_sanitize("unsigned-integer-overflow")))
> +#endif
> +#else
> +#define ignore_unsigned_overflow_
> +#endif
> +
>  /* Policy compatibility information. */
>  struct policydb_compat_info {
>         unsigned int type;
> diff --git a/libsepol/src/symtab.c b/libsepol/src/symtab.c
> index 9a417ca2..a6061851 100644
> --- a/libsepol/src/symtab.c
> +++ b/libsepol/src/symtab.c
> @@ -8,9 +8,13 @@
>   */
>
>  #include <string.h>
> +
> +#include "private.h"
> +
>  #include <sepol/policydb/hashtab.h>
>  #include <sepol/policydb/symtab.h>
>
> +ignore_unsigned_overflow_
>  static unsigned int symhash(hashtab_t h, const_hashtab_key_t key)
>  {
>         const char *p, *keyp;
> --
> 2.32.0
>

Re: [PATCH v2 3/3] libsepol: assure string NUL-termination of ibdev_name

[Nicolas Iooss]

On Thu, Jul 1, 2021 at 8:07 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Clang complains:
>
>     ibendport_record.c: In function ‘sepol_ibendport_get_ibdev_name’:
>     ibendport_record.c:169:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
>       169 |  strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
>           |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     ibendport_record.c: In function ‘sepol_ibendport_set_ibdev_name’:
>     ibendport_record.c:189:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
>       189 |  strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
>           |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> strncpy(3) does not NUL-terminate the destination if the source is of
> the same length or longer then the specified size.
> The source of these copies are retrieved from
> sepol_ibendport_alloc_ibdev_name(), which allocates a fixed amount of
> IB_DEVICE_NAME_MAX bytes.
> Reduce the size to copy by 1 of all memory regions allocated by
> sepol_ibendport_alloc_ibdev_name().
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

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

Thanks,
Nicolas

> ---
> v2:
>   - use at all affected places as pointed out by James Carter
>
>  libsepol/src/ibendport_record.c | 8 ++++----
>  libsepol/src/ibendports.c       | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libsepol/src/ibendport_record.c b/libsepol/src/ibendport_record.c
> index adf67161..1eb50914 100644
> --- a/libsepol/src/ibendport_record.c
> +++ b/libsepol/src/ibendport_record.c
> @@ -62,7 +62,7 @@ int sepol_ibendport_key_create(sepol_handle_t *handle,
>         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_key->ibdev_name) < 0)
>                 goto err;
>
> -       strncpy(tmp_key->ibdev_name, ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(tmp_key->ibdev_name, ibdev_name, IB_DEVICE_NAME_MAX - 1);
>         tmp_key->port = port;
>
>         *key_ptr = tmp_key;
> @@ -166,7 +166,7 @@ int sepol_ibendport_get_ibdev_name(sepol_handle_t *handle,
>         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_ibdev_name) < 0)
>                 goto err;
>
> -       strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
>         *ibdev_name = tmp_ibdev_name;
>         return STATUS_SUCCESS;
>
> @@ -186,7 +186,7 @@ int sepol_ibendport_set_ibdev_name(sepol_handle_t *handle,
>         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp) < 0)
>                 goto err;
>
> -       strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX - 1);
>         free(ibendport->ibdev_name);
>         ibendport->ibdev_name = tmp;
>         return STATUS_SUCCESS;
> @@ -230,7 +230,7 @@ int sepol_ibendport_clone(sepol_handle_t *handle,
>         if (sepol_ibendport_alloc_ibdev_name(handle, &new_ibendport->ibdev_name) < 0)
>                 goto omem;
>
> -       strncpy(new_ibendport->ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(new_ibendport->ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
>         new_ibendport->port = ibendport->port;
>
>         if (ibendport->con &&
> diff --git a/libsepol/src/ibendports.c b/libsepol/src/ibendports.c
> index 6d56c9a1..ee5cb193 100644
> --- a/libsepol/src/ibendports.c
> +++ b/libsepol/src/ibendports.c
> @@ -34,7 +34,7 @@ static int ibendport_from_record(sepol_handle_t *handle,
>                                            &ibdev_name) < 0)
>                 goto err;
>
> -       strncpy(tmp_ibendport->u.ibendport.dev_name, ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(tmp_ibendport->u.ibendport.dev_name, ibdev_name, IB_DEVICE_NAME_MAX - 1);
>
>         free(ibdev_name);
>         ibdev_name = NULL;
> --
> 2.32.0
>

Re: [PATCH v2 2/3] libsepol: avoid implicit conversions

[Nicolas Iooss]

On Thu, Jul 1, 2021 at 8:06 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Avoid implicit conversions from signed to unsigned values, found by
> UB sanitizers, by using unsigned values in the first place.
>
>     expand.c:1644:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
>
>     expand.c:2892:24: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned int' changed the value to 4294967294 (32-bit, unsigned)
>
>     policy_define.c:2344:4: runtime error: implicit conversion from type 'int' of value -1048577 (32-bit, signed) to type 'unsigned int' changed the value to 4293918719 (32-bit, unsigned)
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

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

Thanks,
Nicolas

> ---
> v2:
>   - use UINT32_C as suggested by Ondrej Mosnacek
>
>  libsepol/include/sepol/policydb/conditional.h | 2 +-
>  libsepol/include/sepol/policydb/policydb.h    | 6 +++---
>  libsepol/src/expand.c                         | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libsepol/include/sepol/policydb/conditional.h b/libsepol/include/sepol/policydb/conditional.h
> index 9c3df3ef..49c0d766 100644
> --- a/libsepol/include/sepol/policydb/conditional.h
> +++ b/libsepol/include/sepol/policydb/conditional.h
> @@ -90,7 +90,7 @@ typedef struct cond_node {
>         uint32_t expr_pre_comp;
>         struct cond_node *next;
>         /* a tunable conditional, calculated and used at expansion */
> -#define        COND_NODE_FLAGS_TUNABLE 0x01
> +#define        COND_NODE_FLAGS_TUNABLE UINT32_C(0x01)
>         uint32_t flags;
>  } cond_node_t;
>
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index 6976ef48..4bf9f05d 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -251,9 +251,9 @@ typedef struct class_perm_node {
>         struct class_perm_node *next;
>  } class_perm_node_t;
>
> -#define xperm_test(x, p) (1 & (p[x >> 5] >> (x & 0x1f)))
> -#define xperm_set(x, p) (p[x >> 5] |= (1 << (x & 0x1f)))
> -#define xperm_clear(x, p) (p[x >> 5] &= ~(1 << (x & 0x1f)))
> +#define xperm_test(x, p) (UINT32_C(1) & (p[x >> 5] >> (x & 0x1f)))
> +#define xperm_set(x, p) (p[x >> 5] |= (UINT32_C(1) << (x & 0x1f)))
> +#define xperm_clear(x, p) (p[x >> 5] &= ~(UINT32_C(1) << (x & 0x1f)))
>  #define EXTENDED_PERMS_LEN 8
>
>  typedef struct av_extended_perms {
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 84bfcfa3..aac5b35f 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1641,7 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
>                  * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
>                  * others. Initialize the data accordingly.
>                  */
> -               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
> +               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~UINT32_C(0) : UINT32_C(0);
>                 /* this is used to get the node - insertion is actually unique */
>                 node = avtab_insert_nonunique(avtab, key, &avdatum);
>                 if (!node) {
> --
> 2.32.0
>

Re: [PATCH v2 1/3] libsepol: ignore UBSAN false-positives

[Nicolas Iooss]

On Mon, Jul 12, 2021 at 9:34 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Thu, Jul 1, 2021 at 8:06 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Unsigned integer overflow is well-defined and not undefined behavior.
> > But it is still useful to enable undefined behavior sanitizer checks on
> > unsigned arithmetic to detect possible issues on counters or variables
> > with similar purpose.
> >
> > Annotate functions, in which unsigned overflows are expected to happen,
> > with the respective Clang function attribute[1].
> > GCC does not support sanitizing unsigned integer arithmetic[2].
> >
> >     avtab.c:76:2: runtime error: unsigned integer overflow: 6 * 3432918353 cannot be represented in type 'unsigned int'
> >     policydb.c:795:42: runtime error: unsigned integer overflow: 8160943042179512010 * 11 cannot be represented in type 'unsigned long'
> >     symtab.c:25:12: runtime error: left shift of 1766601759 by 4 places cannot be represented in type 'unsigned int'
> >
> > [1]: https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
> > [2]: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Thanks,
> Nicolas

Merged.
Thanks!
Nicolas

> > ---
> > v2:
> >   - use a common macro as suggested by Ondrej Mosnacek
> >   - mention that GCC does not support unsigned integer sanitation
> >
> >  libsepol/src/avtab.c    |  1 +
> >  libsepol/src/policydb.c |  1 +
> >  libsepol/src/private.h  | 11 +++++++++++
> >  libsepol/src/symtab.c   |  4 ++++
> >  4 files changed, 17 insertions(+)
> >
> > diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
> > index 88e9d510..37e8af07 100644
> > --- a/libsepol/src/avtab.c
> > +++ b/libsepol/src/avtab.c
> > @@ -52,6 +52,7 @@
> >  /* Based on MurmurHash3, written by Austin Appleby and placed in the
> >   * public domain.
> >   */
> > +ignore_unsigned_overflow_
> >  static inline int avtab_hash(struct avtab_key *keyp, uint32_t mask)
> >  {
> >         static const uint32_t c1 = 0xcc9e2d51;
> > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> > index ef2217c2..0721c81e 100644
> > --- a/libsepol/src/policydb.c
> > +++ b/libsepol/src/policydb.c
> > @@ -789,6 +789,7 @@ static int roles_init(policydb_t * p)
> >         goto out;
> >  }
> >
> > +ignore_unsigned_overflow_
> >  static inline unsigned long
> >  partial_name_hash(unsigned long c, unsigned long prevhash)
> >  {
> > diff --git a/libsepol/src/private.h b/libsepol/src/private.h
> > index 72f21262..e54cefdb 100644
> > --- a/libsepol/src/private.h
> > +++ b/libsepol/src/private.h
> > @@ -47,6 +47,17 @@
> >  #define is_saturated(x) (x == (typeof(x))-1)
> >  #define zero_or_saturated(x) ((x == 0) || is_saturated(x))
> >
> > +/* Use to ignore intentional unsigned under- and overflows while running under UBSAN. */
> > +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> > +#if (__clang_major__ >= 12)
> > +#define ignore_unsigned_overflow_        __attribute__((no_sanitize("unsigned-integer-overflow", "unsigned-shift-base")))
> > +#else
> > +#define ignore_unsigned_overflow_        __attribute__((no_sanitize("unsigned-integer-overflow")))
> > +#endif
> > +#else
> > +#define ignore_unsigned_overflow_
> > +#endif
> > +
> >  /* Policy compatibility information. */
> >  struct policydb_compat_info {
> >         unsigned int type;
> > diff --git a/libsepol/src/symtab.c b/libsepol/src/symtab.c
> > index 9a417ca2..a6061851 100644
> > --- a/libsepol/src/symtab.c
> > +++ b/libsepol/src/symtab.c
> > @@ -8,9 +8,13 @@
> >   */
> >
> >  #include <string.h>
> > +
> > +#include "private.h"
> > +
> >  #include <sepol/policydb/hashtab.h>
> >  #include <sepol/policydb/symtab.h>
> >
> > +ignore_unsigned_overflow_
> >  static unsigned int symhash(hashtab_t h, const_hashtab_key_t key)
> >  {
> >         const char *p, *keyp;
> > --
> > 2.32.0
> >

Re: [PATCH v2 3/3] libsepol: assure string NUL-termination of ibdev_name

[Nicolas Iooss]

On Mon, Jul 12, 2021 at 9:35 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Thu, Jul 1, 2021 at 8:07 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Clang complains:
> >
> >     ibendport_record.c: In function ‘sepol_ibendport_get_ibdev_name’:
> >     ibendport_record.c:169:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
> >       169 |  strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
> >           |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >     ibendport_record.c: In function ‘sepol_ibendport_set_ibdev_name’:
> >     ibendport_record.c:189:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
> >       189 |  strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
> >           |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > strncpy(3) does not NUL-terminate the destination if the source is of
> > the same length or longer then the specified size.
> > The source of these copies are retrieved from
> > sepol_ibendport_alloc_ibdev_name(), which allocates a fixed amount of
> > IB_DEVICE_NAME_MAX bytes.
> > Reduce the size to copy by 1 of all memory regions allocated by
> > sepol_ibendport_alloc_ibdev_name().
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Thanks,
> Nicolas

Merged.
Thanks!
Nicolas

> > ---
> > v2:
> >   - use at all affected places as pointed out by James Carter
> >
> >  libsepol/src/ibendport_record.c | 8 ++++----
> >  libsepol/src/ibendports.c       | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/libsepol/src/ibendport_record.c b/libsepol/src/ibendport_record.c
> > index adf67161..1eb50914 100644
> > --- a/libsepol/src/ibendport_record.c
> > +++ b/libsepol/src/ibendport_record.c
> > @@ -62,7 +62,7 @@ int sepol_ibendport_key_create(sepol_handle_t *handle,
> >         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_key->ibdev_name) < 0)
> >                 goto err;
> >
> > -       strncpy(tmp_key->ibdev_name, ibdev_name, IB_DEVICE_NAME_MAX);
> > +       strncpy(tmp_key->ibdev_name, ibdev_name, IB_DEVICE_NAME_MAX - 1);
> >         tmp_key->port = port;
> >
> >         *key_ptr = tmp_key;
> > @@ -166,7 +166,7 @@ int sepol_ibendport_get_ibdev_name(sepol_handle_t *handle,
> >         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_ibdev_name) < 0)
> >                 goto err;
> >
> > -       strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
> > +       strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
> >         *ibdev_name = tmp_ibdev_name;
> >         return STATUS_SUCCESS;
> >
> > @@ -186,7 +186,7 @@ int sepol_ibendport_set_ibdev_name(sepol_handle_t *handle,
> >         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp) < 0)
> >                 goto err;
> >
> > -       strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
> > +       strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX - 1);
> >         free(ibendport->ibdev_name);
> >         ibendport->ibdev_name = tmp;
> >         return STATUS_SUCCESS;
> > @@ -230,7 +230,7 @@ int sepol_ibendport_clone(sepol_handle_t *handle,
> >         if (sepol_ibendport_alloc_ibdev_name(handle, &new_ibendport->ibdev_name) < 0)
> >                 goto omem;
> >
> > -       strncpy(new_ibendport->ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
> > +       strncpy(new_ibendport->ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
> >         new_ibendport->port = ibendport->port;
> >
> >         if (ibendport->con &&
> > diff --git a/libsepol/src/ibendports.c b/libsepol/src/ibendports.c
> > index 6d56c9a1..ee5cb193 100644
> > --- a/libsepol/src/ibendports.c
> > +++ b/libsepol/src/ibendports.c
> > @@ -34,7 +34,7 @@ static int ibendport_from_record(sepol_handle_t *handle,
> >                                            &ibdev_name) < 0)
> >                 goto err;
> >
> > -       strncpy(tmp_ibendport->u.ibendport.dev_name, ibdev_name, IB_DEVICE_NAME_MAX);
> > +       strncpy(tmp_ibendport->u.ibendport.dev_name, ibdev_name, IB_DEVICE_NAME_MAX - 1);
> >
> >         free(ibdev_name);
> >         ibdev_name = NULL;
> > --
> > 2.32.0
> >

Re: [PATCH v2 2/3] libsepol: avoid implicit conversions

[Nicolas Iooss]

On Mon, Jul 12, 2021 at 9:36 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Thu, Jul 1, 2021 at 8:06 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Avoid implicit conversions from signed to unsigned values, found by
> > UB sanitizers, by using unsigned values in the first place.
> >
> >     expand.c:1644:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
> >
> >     expand.c:2892:24: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned int' changed the value to 4294967294 (32-bit, unsigned)
> >
> >     policy_define.c:2344:4: runtime error: implicit conversion from type 'int' of value -1048577 (32-bit, signed) to type 'unsigned int' changed the value to 4293918719 (32-bit, unsigned)
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Thanks,
> Nicolas

Merged.
Thanks!
Nicolas

> > ---
> > v2:
> >   - use UINT32_C as suggested by Ondrej Mosnacek
> >
> >  libsepol/include/sepol/policydb/conditional.h | 2 +-
> >  libsepol/include/sepol/policydb/policydb.h    | 6 +++---
> >  libsepol/src/expand.c                         | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/libsepol/include/sepol/policydb/conditional.h b/libsepol/include/sepol/policydb/conditional.h
> > index 9c3df3ef..49c0d766 100644
> > --- a/libsepol/include/sepol/policydb/conditional.h
> > +++ b/libsepol/include/sepol/policydb/conditional.h
> > @@ -90,7 +90,7 @@ typedef struct cond_node {
> >         uint32_t expr_pre_comp;
> >         struct cond_node *next;
> >         /* a tunable conditional, calculated and used at expansion */
> > -#define        COND_NODE_FLAGS_TUNABLE 0x01
> > +#define        COND_NODE_FLAGS_TUNABLE UINT32_C(0x01)
> >         uint32_t flags;
> >  } cond_node_t;
> >
> > diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> > index 6976ef48..4bf9f05d 100644
> > --- a/libsepol/include/sepol/policydb/policydb.h
> > +++ b/libsepol/include/sepol/policydb/policydb.h
> > @@ -251,9 +251,9 @@ typedef struct class_perm_node {
> >         struct class_perm_node *next;
> >  } class_perm_node_t;
> >
> > -#define xperm_test(x, p) (1 & (p[x >> 5] >> (x & 0x1f)))
> > -#define xperm_set(x, p) (p[x >> 5] |= (1 << (x & 0x1f)))
> > -#define xperm_clear(x, p) (p[x >> 5] &= ~(1 << (x & 0x1f)))
> > +#define xperm_test(x, p) (UINT32_C(1) & (p[x >> 5] >> (x & 0x1f)))
> > +#define xperm_set(x, p) (p[x >> 5] |= (UINT32_C(1) << (x & 0x1f)))
> > +#define xperm_clear(x, p) (p[x >> 5] &= ~(UINT32_C(1) << (x & 0x1f)))
> >  #define EXTENDED_PERMS_LEN 8
> >
> >  typedef struct av_extended_perms {
> > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> > index 84bfcfa3..aac5b35f 100644
> > --- a/libsepol/src/expand.c
> > +++ b/libsepol/src/expand.c
> > @@ -1641,7 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
> >                  * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
> >                  * others. Initialize the data accordingly.
> >                  */
> > -               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
> > +               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~UINT32_C(0) : UINT32_C(0);
> >                 /* this is used to get the node - insertion is actually unique */
> >                 node = avtab_insert_nonunique(avtab, key, &avdatum);
> >                 if (!node) {
> > --
> > 2.32.0
> >