[PATCH 00/25] libselinux: misc compiler and static analyzer findings

[PATCH 00/25] libselinux: misc compiler and static analyzer findings

[Christian Göttsche]

Several changes found by compilers and static analyzer regarding const
qualifiers, dead code, code smells and resource cleanup.

Contains no changes with regard to libselinux callers.

Christian Göttsche (25):
  libselinux: sidtab_hash(): do not discard const qualifier
  libselinux: selinux_file_context_cmp(): do not discard const qualifier
  libselinux: label_common(): do not discard const qualifier
  libselinux: Sha1Finalise(): do not discard const qualifier
  libselinux: sefcontext_compile: mark local variable static
  libselinux: avcstat: use standard length modifier for unsigned long
    long
  libselinux: selinux_restorecon: mark local variable static
  libselinux: selabel_get_digests_all_partial_matches: free memory after
    FTS_D block
  libselinux: getconlist: free memory on multiple level arguments
  libselinux: exclude_non_seclabel_mounts(): drop unused variable
  libselinux: context_new(): drop dead assignment
  libselinux: label_x::init(): drop dead assignment
  libselinux: label_media::init(): drop dead assignment
  libselinux: setexecfilecon(): drop dead assignment
  libselinux: getdefaultcon: free memory on multiple same arguments
  libselinux: store_stem(): do not free possible non-heap object
  libselinux: matchmediacon(): close file on error
  libselinux: init_selinux_config(): free resources on error
  libselinux: label_file::init(): do not pass NULL to strdup
  libselinux: matchpathcon: free memory on realloc failure
  libselinux: label_db::db_init(): open file with CLOEXEC mode
  libselinux: drop redundant casts to the same type
  libselinux: sidtab_sid_stats(): unify parameter name
  libselinux: regex: unify parameter names
  libselinux: label_file.c: fix indent

 libselinux/src/audit2why.c                    |  2 +-
 libselinux/src/avc_sidtab.c                   | 15 ++++-----
 libselinux/src/context.c                      |  2 +-
 libselinux/src/is_customizable_type.c         |  2 +-
 libselinux/src/label_db.c                     |  2 +-
 libselinux/src/label_file.c                   | 16 ++++++----
 libselinux/src/label_file.h                   |  8 +++--
 libselinux/src/label_media.c                  |  1 -
 libselinux/src/label_x.c                      |  1 -
 libselinux/src/matchmediacon.c                |  1 +
 libselinux/src/matchpathcon.c                 | 32 +++++++++++--------
 libselinux/src/regex.c                        |  4 +--
 libselinux/src/selinux_config.c               |  7 +++-
 libselinux/src/selinux_restorecon.c           |  7 ++--
 libselinux/src/setexecfilecon.c               |  1 -
 libselinux/src/sha1.c                         | 10 +++---
 libselinux/src/sha1.h                         |  2 +-
 libselinux/utils/avcstat.c                    |  4 +--
 libselinux/utils/getconlist.c                 |  1 +
 libselinux/utils/getdefaultcon.c              |  3 ++
 libselinux/utils/sefcontext_compile.c         |  2 +-
 .../selabel_get_digests_all_partial_matches.c | 10 +++---
 22 files changed, 76 insertions(+), 57 deletions(-)

-- 
2.31.1

[PATCH 01/25] libselinux: sidtab_hash(): do not discard const qualifier

[Christian Göttsche]

Do not discard the const qualifier of the function argument, and drop
the redundant local variable `keyp`.

avc_sidtab.c: In function ‘sidtab_hash’:
avc_sidtab.c:23:9: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
   23 |  keyp = (char *)key;
      |         ^

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

diff --git a/libselinux/src/avc_sidtab.c b/libselinux/src/avc_sidtab.c
index 9669264d..8dc87560 100644
--- a/libselinux/src/avc_sidtab.c
+++ b/libselinux/src/avc_sidtab.c
@@ -15,14 +15,13 @@
 
 static inline unsigned sidtab_hash(const char * key)
 {
-	char *p, *keyp;
+	const char *p;
 	unsigned int size;
 	unsigned int val;
 
 	val = 0;
-	keyp = (char *)key;
-	size = strlen(keyp);
-	for (p = keyp; (unsigned int)(p - keyp) < size; p++)
+	size = strlen(key);
+	for (p = key; (unsigned int)(p - key) < size; p++)
 		val =
 		    (val << 4 | (val >> (8 * sizeof(unsigned int) - 4))) ^ (*p);
 	return val & (SIDTAB_SIZE - 1);
-- 
2.31.1

[PATCH 02/25] libselinux: selinux_file_context_cmp(): do not discard const qualifier

[Christian Göttsche]

matchpathcon.c: In function ‘selinux_file_context_cmp’:
matchpathcon.c:487:18: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
  487 |  rest_a = strchr((char *)a, ':');
      |                  ^
matchpathcon.c:488:18: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
  488 |  rest_b = strchr((char *)b, ':');
      |                  ^

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

diff --git a/libselinux/src/matchpathcon.c b/libselinux/src/matchpathcon.c
index 2ec66650..9e1fab59 100644
--- a/libselinux/src/matchpathcon.c
+++ b/libselinux/src/matchpathcon.c
@@ -477,15 +477,15 @@ void matchpathcon_checkmatches(char *str __attribute__((unused)))
 int selinux_file_context_cmp(const char * a,
 			     const char * b)
 {
-	char *rest_a, *rest_b;	/* Rest of the context after the user */
+	const char *rest_a, *rest_b;	/* Rest of the context after the user */
 	if (!a && !b)
 		return 0;
 	if (!a)
 		return -1;
 	if (!b)
 		return 1;
-	rest_a = strchr((char *)a, ':');
-	rest_b = strchr((char *)b, ':');
+	rest_a = strchr(a, ':');
+	rest_b = strchr(b, ':');
 	if (!rest_a && !rest_b)
 		return 0;
 	if (!rest_a)
-- 
2.31.1

[PATCH 03/25] libselinux: label_common(): do not discard const qualifier

[Christian Göttsche]

As the const qualifier is discarded in label_common(), do not return a
const qualified pointer pointer from the local function `lookup_all()`.

label_file.c: In function ‘lookup_common’:
label_file.c:994:24: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
  994 |  struct spec *result = (struct spec*)matches[0];
      |                        ^

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

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index 726394ca..4d4e3a76 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -845,7 +845,7 @@ static void closef(struct selabel_handle *rec)
 // Finds all the matches of |key| in the given context. Returns the result in
 // the allocated array and updates the match count. If match_count is NULL,
 // stops early once the 1st match is found.
-static const struct spec **lookup_all(struct selabel_handle *rec,
+static struct spec **lookup_all(struct selabel_handle *rec,
                                       const char *key,
                                       int type,
                                       bool partial,
@@ -861,7 +861,7 @@ static const struct spec **lookup_all(struct selabel_handle *rec,
 	unsigned int sofar = 0;
 	char *sub = NULL;
 
-	const struct spec **result = NULL;
+	struct spec **result = NULL;
 	if (match_count) {
 		*match_count = 0;
 		result = calloc(data->nspec, sizeof(struct spec*));
@@ -987,11 +987,11 @@ static struct spec *lookup_common(struct selabel_handle *rec,
                                   const char *key,
                                   int type,
                                   bool partial) {
-	const struct spec **matches = lookup_all(rec, key, type, partial, NULL);
+	struct spec **matches = lookup_all(rec, key, type, partial, NULL);
 	if (!matches) {
 		return NULL;
 	}
-	struct spec *result = (struct spec*)matches[0];
+	struct spec *result = matches[0];
 	free(matches);
 	return result;
 }
@@ -1054,7 +1054,7 @@ static bool hash_all_partial_matches(struct selabel_handle *rec, const char *key
 	assert(digest);
 
 	size_t total_matches;
-	const struct spec **matches = lookup_all(rec, key, 0, true, &total_matches);
+	struct spec **matches = lookup_all(rec, key, 0, true, &total_matches);
 	if (!matches) {
 		return false;
 	}
-- 
2.31.1

[PATCH 04/25] libselinux: Sha1Finalise(): do not discard const qualifier

[Christian Göttsche]

Mark the argument `Buffer` of `Sha1Update()` const, since it is not
modified.

sha1.c: In function ‘Sha1Finalise’:
sha1.c:208:25: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
  208 |     Sha1Update(Context, (uint8_t*)"\x80", 1);
      |                         ^
sha1.c:211:29: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
  211 |         Sha1Update(Context, (uint8_t*)"\0", 1);
      |                             ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/sha1.c | 10 +++++-----
 libselinux/src/sha1.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/libselinux/src/sha1.c b/libselinux/src/sha1.c
index 9a8ce01d..664bbcf2 100644
--- a/libselinux/src/sha1.c
+++ b/libselinux/src/sha1.c
@@ -151,7 +151,7 @@ void
     Sha1Update
     (
         Sha1Context*        Context,
-        void*               Buffer,
+        const void*         Buffer,
         uint32_t            BufferSize
     )
 {
@@ -172,7 +172,7 @@ void
         TransformFunction(Context->State, Context->Buffer);
         for (; i + 63 < BufferSize; i += 64)
         {
-            TransformFunction(Context->State, (uint8_t*)Buffer + i);
+            TransformFunction(Context->State, (const uint8_t*)Buffer + i);
         }
         j = 0;
     }
@@ -181,7 +181,7 @@ void
         i = 0;
     }
 
-    memcpy(&Context->Buffer[j], &((uint8_t*)Buffer)[i], BufferSize - i);
+    memcpy(&Context->Buffer[j], &((const uint8_t*)Buffer)[i], BufferSize - i);
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -205,10 +205,10 @@ void
         finalcount[i] = (unsigned char)((Context->Count[(i >= 4 ? 0 : 1)]
          >> ((3-(i & 3)) * 8) ) & 255);  // Endian independent
     }
-    Sha1Update(Context, (uint8_t*)"\x80", 1);
+    Sha1Update(Context, (const uint8_t*)"\x80", 1);
     while ((Context->Count[0] & 504) != 448)
     {
-        Sha1Update(Context, (uint8_t*)"\0", 1);
+        Sha1Update(Context, (const uint8_t*)"\0", 1);
     }
 
     Sha1Update(Context, finalcount, 8);  // Should cause a Sha1TransformFunction()
diff --git a/libselinux/src/sha1.h b/libselinux/src/sha1.h
index eac3c195..f83a6e7e 100644
--- a/libselinux/src/sha1.h
+++ b/libselinux/src/sha1.h
@@ -64,7 +64,7 @@ void
     Sha1Update
     (
         Sha1Context*        Context,
-        void*               Buffer,
+        const void*         Buffer,
         uint32_t            BufferSize
     );
 
-- 
2.31.1

[PATCH 05/25] libselinux: sefcontext_compile: mark local variable static

[Christian Göttsche]

The variable `policy_file` is only used in sefcontext_compile.c.

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

diff --git a/libselinux/utils/sefcontext_compile.c b/libselinux/utils/sefcontext_compile.c
index dcb0085a..6c32172d 100644
--- a/libselinux/utils/sefcontext_compile.c
+++ b/libselinux/utils/sefcontext_compile.c
@@ -14,7 +14,7 @@
 #include "../src/label_file.h"
 #include "../src/regex.h"
 
-const char *policy_file;
+static const char *policy_file;
 static int ctx_err;
 
 static int validate_context(char **ctxp)
-- 
2.31.1

[PATCH 06/25] libselinux: avcstat: use standard length modifier for unsigned long long

[Christian Göttsche]

The format width specifier `L` is only standardized for floating point
types. Use `ll` for fixed-width data types.

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

diff --git a/libselinux/utils/avcstat.c b/libselinux/utils/avcstat.c
index da239287..cc9a48dd 100644
--- a/libselinux/utils/avcstat.c
+++ b/libselinux/utils/avcstat.c
@@ -205,7 +205,7 @@ int main(int argc, char **argv)
 			die("unable to parse \'%s\': no data", avcstatfile);
 
 		if (cumulative || !i)
-			printf("%10Lu %10Lu %10Lu %10Lu %10Lu %10Lu\n",
+			printf("%10llu %10llu %10llu %10llu %10llu %10llu\n",
 			       tot.lookups, tot.hits, tot.misses,
 			       tot.allocations, tot.reclaims, tot.frees);
 		else {
@@ -215,7 +215,7 @@ int main(int argc, char **argv)
 			rel.allocations = tot.allocations - last.allocations;
 			rel.reclaims = tot.reclaims - last.reclaims;
 			rel.frees = tot.frees - last.frees;
-			printf("%10Lu %10Lu %10Lu %10Lu %10Lu %10Lu\n",
+			printf("%10llu %10llu %10llu %10llu %10llu %10llu\n",
 			       rel.lookups, rel.hits, rel.misses,
 			       rel.allocations, rel.reclaims, rel.frees);
 		}
-- 
2.31.1

[PATCH 07/25] libselinux: selinux_restorecon: mark local variable static

[Christian Göttsche]

The variable `dir_xattr_list` is only used inside `selinux_restorecon.c`.

selinux_restorecon.c:65:19: warning: no previous extern declaration for non-static variable 'dir_xattr_list' [-Wmissing-variable-declarations]
struct dir_xattr *dir_xattr_list;
                  ^
selinux_restorecon.c:65:1: note: declare 'static' if the variable is not intended to be used outside of this translation unit
struct dir_xattr *dir_xattr_list;
^

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

diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 63fb8dc5..249c361f 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -62,7 +62,7 @@ static uint64_t fc_count = 0;	/* Number of files processed so far */
 static uint64_t efile_count;	/* Estimated total number of files */
 
 /* Store information on directories with xattr's. */
-struct dir_xattr *dir_xattr_list;
+static struct dir_xattr *dir_xattr_list;
 static struct dir_xattr *dir_xattr_last;
 
 /* restorecon_flags for passing to restorecon_sb() */
-- 
2.31.1

[PATCH 08/25] libselinux: selabel_get_digests_all_partial_matches: free memory after FTS_D block

[Christian Göttsche]

Free all memory from `selabel_get_digests_all_partial_matches()` in case
of success and failure.

Found by clang-analyzer.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 .../utils/selabel_get_digests_all_partial_matches.c    | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/libselinux/utils/selabel_get_digests_all_partial_matches.c b/libselinux/utils/selabel_get_digests_all_partial_matches.c
index 0c2edc67..e28833d2 100644
--- a/libselinux/utils/selabel_get_digests_all_partial_matches.c
+++ b/libselinux/utils/selabel_get_digests_all_partial_matches.c
@@ -128,7 +128,7 @@ int main(int argc, char **argv)
 					printf("No SHA1 digest available for: %s\n",
 					       ftsent->fts_path);
 					printf("as file_context entry is \"<<none>>\"\n");
-					break;
+					goto cleanup;
 				}
 
 				printf("The file_context entries for: %s\n",
@@ -149,11 +149,11 @@ int main(int argc, char **argv)
 							xattr_digest[i]);
 					printf("%s\n", sha1_buf);
 				}
-
-				free(xattr_digest);
-				free(calculated_digest);
-				free(sha1_buf);
 			}
+			cleanup:
+			free(xattr_digest);
+			free(calculated_digest);
+			free(sha1_buf);
 			break;
 		}
 		default:
-- 
2.31.1

[PATCH 09/25] libselinux: getconlist: free memory on multiple level arguments

[Christian Göttsche]

Do not leak memory if the program argument `l` got passed more than
once.

Found by clang-analyzer.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/utils/getconlist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index 76654b75..0bb28469 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -26,6 +26,7 @@ int main(int argc, char **argv)
 	while ((opt = getopt(argc, argv, "l:")) > 0) {
 		switch (opt) {
 		case 'l':
+			free(level);
 			level = strdup(optarg);
 			if (!level) {
 				fprintf(stderr, "memory allocation failure: %d(%s)\n",
-- 
2.31.1

[PATCH 10/25] libselinux: exclude_non_seclabel_mounts(): drop unused variable

[Christian Göttsche]

The variable `num` is never read from.

Found by clang-analyer.

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

diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 249c361f..6fb9e1ff 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -230,7 +230,6 @@ static int exclude_non_seclabel_mounts(void)
 	struct utsname uts;
 	FILE *fp;
 	size_t len;
-	ssize_t num;
 	int index = 0, found = 0, nfile = 0;
 	char *mount_info[4];
 	char *buf = NULL, *item;
@@ -245,7 +244,7 @@ static int exclude_non_seclabel_mounts(void)
 	if (!fp)
 		return 0;
 
-	while ((num = getline(&buf, &len, fp)) != -1) {
+	while (getline(&buf, &len, fp) != -1) {
 		found = 0;
 		index = 0;
 		item = strtok(buf, " ");
-- 
2.31.1

[PATCH 11/25] libselinux: context_new(): drop dead assignment

[Christian Göttsche]

The variable `i` is not used inside this loop, and it later
unconditionally set to 0.

Found by clang-analyzer.

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

diff --git a/libselinux/src/context.c b/libselinux/src/context.c
index ce425880..b2144c7c 100644
--- a/libselinux/src/context.c
+++ b/libselinux/src/context.c
@@ -37,7 +37,7 @@ context_t context_new(const char *str)
 	}
 	n->current_str = n->component[0] = n->component[1] = n->component[2] =
 	    n->component[3] = 0;
-	for (i = count = 0, p = str; *p; p++) {
+	for (count = 0, p = str; *p; p++) {
 		switch (*p) {
 		case ':':
 			count++;
-- 
2.31.1

[PATCH 12/25] libselinux: label_x::init(): drop dead assignment

[Christian Göttsche]

The variable `lineno` is only used in the preceding loop and is always
set prior that to 0.

Found by clang-analyzer.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/label_x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libselinux/src/label_x.c b/libselinux/src/label_x.c
index 96745299..e9fa063f 100644
--- a/libselinux/src/label_x.c
+++ b/libselinux/src/label_x.c
@@ -146,7 +146,6 @@ static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
 			if (process_line(path, line_buf, pass, ++lineno, rec))
 				goto finish;
 		}
-		lineno = 0;
 
 		if (pass == 0) {
 			if (data->nspec == 0) {
-- 
2.31.1

[PATCH 13/25] libselinux: label_media::init(): drop dead assignment

[Christian Göttsche]

The variable `lineno` is only used in the preceding loop and it always
set prior that to 0.

Found by clang-analyzer.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/label_media.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libselinux/src/label_media.c b/libselinux/src/label_media.c
index d202e5d5..eb27deaf 100644
--- a/libselinux/src/label_media.c
+++ b/libselinux/src/label_media.c
@@ -119,7 +119,6 @@ static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
 			if (process_line(path, line_buf, pass, ++lineno, rec))
 				goto finish;
 		}
-		lineno = 0;
 
 		if (pass == 0) {
 			if (data->nspec == 0) {
-- 
2.31.1

[PATCH 14/25] libselinux: setexecfilecon(): drop dead assignment

[Christian Göttsche]

The variable `rc` is always unconditionally assigned by the next call of
`setexeccon()` and never read in between.

Found by clang-analyzer.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/setexecfilecon.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libselinux/src/setexecfilecon.c b/libselinux/src/setexecfilecon.c
index e72ba0d9..2c6505a9 100644
--- a/libselinux/src/setexecfilecon.c
+++ b/libselinux/src/setexecfilecon.c
@@ -37,7 +37,6 @@ int setexecfilecon(const char *filename, const char *fallback_type)
 		newcon = strdup(context_str(con));
 		if (!newcon)
 			goto out;
-		rc = 0;
 	}
 
 	rc = setexeccon(newcon);
-- 
2.31.1

[PATCH 15/25] libselinux: getdefaultcon: free memory on multiple same arguments

[Christian Göttsche]

Do not leak memory if program arguments get specified more than once.

Found by clang-anlyzer.

getdefaultcon.c:52:3: warning: Potential leak of memory pointed to by 'level' [unix.Malloc]
                fprintf(stderr,
                ^~~~~~~~~~~~~~~
getdefaultcon.c:52:3: warning: Potential leak of memory pointed to by 'role' [unix.Malloc]
                fprintf(stderr,
                ^~~~~~~~~~~~~~~
getdefaultcon.c:52:3: warning: Potential leak of memory pointed to by 'service' [unix.Malloc]
                fprintf(stderr,
                ^~~~~~~~~~~~~~~

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/utils/getdefaultcon.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libselinux/utils/getdefaultcon.c b/libselinux/utils/getdefaultcon.c
index 96a5a8c2..957c1cb2 100644
--- a/libselinux/utils/getdefaultcon.c
+++ b/libselinux/utils/getdefaultcon.c
@@ -28,12 +28,15 @@ int main(int argc, char **argv)
 	while ((opt = getopt(argc, argv, "l:r:s:v")) > 0) {
 		switch (opt) {
 		case 'l':
+			free(level);
 			level = strdup(optarg);
 			break;
 		case 'r':
+			free(role);
 			role = strdup(optarg);
 			break;
 		case 's':
+			free(service);
 			service = strdup(optarg);
 			break;
 		case 'v':
-- 
2.31.1

[PATCH 16/25] libselinux: store_stem(): do not free possible non-heap object

[Christian Göttsche]

GCC 11 complains:

In file included from label_file.c:24:
In function ‘store_stem’,
    inlined from ‘load_mmap’ at label_file.c:277:12,
    inlined from ‘process_file’ at label_file.c:551:5:
label_file.h:289:25: error: ‘free’ called on pointer ‘*mmap_area.next_addr’ with nonzero offset 4 [-Werror=free-nonheap-object]
  289 |                         free(buf);
      |                         ^~~~~~~~~

Free the pointer on failure at the caller instead of inside `store_stem()`.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/label_file.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index baed3341..9f633701 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -286,7 +286,6 @@ static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
 		tmp_arr = realloc(data->stem_arr,
 				  sizeof(*tmp_arr) * alloc_stems);
 		if (!tmp_arr) {
-			free(buf);
 			return -1;
 		}
 		data->alloc_stems = alloc_stems;
@@ -308,6 +307,7 @@ static inline int find_stem_from_spec(struct saved_data *data, const char *buf)
 	int stem_len = get_stem_from_spec(buf);
 	int stemid;
 	char *stem;
+	int r;
 
 	if (!stem_len)
 		return -1;
@@ -321,7 +321,11 @@ static inline int find_stem_from_spec(struct saved_data *data, const char *buf)
 	if (!stem)
 		return -1;
 
-	return store_stem(data, stem, stem_len);
+	r = store_stem(data, stem, stem_len);
+	if (r < 0)
+		free(stem);
+
+	return r;
 }
 
 /* This will always check for buffer over-runs and either read the next entry
-- 
2.31.1

[PATCH 17/25] libselinux: matchmediacon(): close file on error

[Christian Göttsche]

Found by Infer.

matchmediacon.c:25: error: Resource Leak
  resource of type `_IO_FILE` acquired to `return` by call to `fopen()` at line 21, column 16 is not released after line 25, column 4.
  23. 	while (!feof_unlocked(infile)) {
  24. 		if (!fgets_unlocked(current_line, sizeof(current_line), infile)) {
  25. 			return -1;
         ^
  26. 		}
  27. 		if (current_line[strlen(current_line) - 1])

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

diff --git a/libselinux/src/matchmediacon.c b/libselinux/src/matchmediacon.c
index 23d01af4..d3d95043 100644
--- a/libselinux/src/matchmediacon.c
+++ b/libselinux/src/matchmediacon.c
@@ -22,6 +22,7 @@ int matchmediacon(const char *media, char ** con)
 		return -1;
 	while (!feof_unlocked(infile)) {
 		if (!fgets_unlocked(current_line, sizeof(current_line), infile)) {
+			fclose(infile);
 			return -1;
 		}
 		if (current_line[strlen(current_line) - 1])
-- 
2.31.1

[PATCH 18/25] libselinux: init_selinux_config(): free resources on error

[Christian Göttsche]

Found by Infer.

selinux_config.c:181: error: Resource Leak
  resource of type `_IO_FILE` acquired by call to `fopen()` at line 165, column 7 is not released after line 181, column 6.
  179. 				type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
  180. 				if (!type)
  181. 					return;
            ^
  182. 				end = type + strlen(type) - 1;
  183. 				while ((end > type) &&

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/selinux_config.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c
index 6c523895..97f81a8b 100644
--- a/libselinux/src/selinux_config.c
+++ b/libselinux/src/selinux_config.c
@@ -177,8 +177,11 @@ static void init_selinux_config(void)
 			if (!strncasecmp(buf_p, SELINUXTYPETAG,
 					 sizeof(SELINUXTYPETAG) - 1)) {
 				type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
-				if (!type)
+				if (!type) {
+					free(line_buf);
+					fclose(fp);
 					return;
+				}
 				end = type + strlen(type) - 1;
 				while ((end > type) &&
 				       (isspace(*end) || iscntrl(*end))) {
@@ -187,6 +190,8 @@ static void init_selinux_config(void)
 				}
 				if (setpolicytype(type) != 0) {
 					free(type);
+					free(line_buf);
+					fclose(fp);
 					return;
 				}
 				free(type);
-- 
2.31.1

[PATCH 19/25] libselinux: label_file::init(): do not pass NULL to strdup

[Christian Göttsche]

If any of the build flags `BUILD_HOST` or `ANDROID` is set and the
caller did not pass an option of type `SELABEL_OPT_PATH`, the variable
`path` might be not set.
Add a check to avoid calling `strdup()` with a NULL pointer.

Found by cppcheck.

src/label_file.c:759:26: warning: Possible null pointer dereference: path [nullPointer]
 rec->spec_file = strdup(path);
                         ^
src/label_file.c:713:21: note: Assignment 'path=NULL', assigned value is 0
 const char *path = NULL;
                    ^
src/label_file.c:759:26: note: Null pointer dereference
 rec->spec_file = strdup(path);
                         ^

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

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index 4d4e3a76..39a56133 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -756,6 +756,10 @@ static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
 	}
 
 #endif
+
+	if (!path)
+		goto finish;
+
 	rec->spec_file = strdup(path);
 
 	/*
-- 
2.31.1

[PATCH 20/25] libselinux: matchpathcon: free memory on realloc failure

[Christian Göttsche]

In case `realloc()` fails and returns NULL, free the passed array,
instead of just setting the size helper variables to 0.

Also free the string contents in `free_array_elts()` of the array
`con_array`, instead of just the array of pointers.

Found by cppcheck.

src/matchpathcon.c:86:4: error: Common realloc mistake: 'con_array' nulled but not freed upon failure [memleakOnRealloc]
   con_array = (char **)realloc(con_array, sizeof(char*) *
   ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/matchpathcon.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/libselinux/src/matchpathcon.c b/libselinux/src/matchpathcon.c
index 9e1fab59..075a3fb3 100644
--- a/libselinux/src/matchpathcon.c
+++ b/libselinux/src/matchpathcon.c
@@ -78,17 +78,30 @@ static pthread_once_t once = PTHREAD_ONCE_INIT;
 static pthread_key_t destructor_key;
 static int destructor_key_initialized = 0;
 
+static void free_array_elts(void)
+{
+	int i;
+	for (i = 0; i < con_array_used; i++)
+		free(con_array[i]);
+	free(con_array);
+
+	con_array_size = con_array_used = 0;
+	con_array = NULL;
+}
+
 static int add_array_elt(char *con)
 {
+	char **tmp;
 	if (con_array_size) {
 		while (con_array_used >= con_array_size) {
 			con_array_size *= 2;
-			con_array = (char **)realloc(con_array, sizeof(char*) *
+			tmp = (char **)realloc(con_array, sizeof(char*) *
 						     con_array_size);
-			if (!con_array) {
-				con_array_size = con_array_used = 0;
+			if (!tmp) {
+				free_array_elts();
 				return -1;
 			}
+			con_array = tmp;
 		}
 	} else {
 		con_array_size = 1000;
@@ -105,13 +118,6 @@ static int add_array_elt(char *con)
 	return con_array_used++;
 }
 
-static void free_array_elts(void)
-{
-	con_array_size = con_array_used = 0;
-	free(con_array);
-	con_array = NULL;
-}
-
 void set_matchpathcon_invalidcon(int (*f) (const char *p, unsigned l, char *c))
 {
 	myinvalidcon = f;
-- 
2.31.1

[PATCH 21/25] libselinux: label_db::db_init(): open file with CLOEXEC mode

[Christian Göttsche]

Open the file stream with the `e` flag, so that the underlying file
descriptor gets closed on an exec in a potential sibling thread.

Also drop the flag `b`, since it is ignored on POSIX systems.

Found by clang-tidy.

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

diff --git a/libselinux/src/label_db.c b/libselinux/src/label_db.c
index fba96c92..94c05c6d 100644
--- a/libselinux/src/label_db.c
+++ b/libselinux/src/label_db.c
@@ -277,7 +277,7 @@ db_init(const struct selinux_opt *opts, unsigned nopts,
 	if (!path)
 		path = selinux_sepgsql_context_path();
 
-	if ((filp = fopen(path, "rb")) == NULL) {
+	if ((filp = fopen(path, "re")) == NULL) {
 		free(catalog);
 		return NULL;
 	}
-- 
2.31.1

[PATCH 22/25] libselinux: drop redundant casts to the same type

[Christian Göttsche]

Found by clang-tidy.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/audit2why.c            | 2 +-
 libselinux/src/avc_sidtab.c           | 2 +-
 libselinux/src/is_customizable_type.c | 2 +-
 libselinux/src/selinux_restorecon.c   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libselinux/src/audit2why.c b/libselinux/src/audit2why.c
index d56b56eb..029f874f 100644
--- a/libselinux/src/audit2why.c
+++ b/libselinux/src/audit2why.c
@@ -275,7 +275,7 @@ static int __policy_init(const char *init_path)
 	}
 
 	sepol_bool_iterate(avc->handle, avc->policydb,
-			   load_booleans, (void *)NULL);
+			   load_booleans, NULL);
 
 	/* Initialize the sidtab for subsequent use by sepol_context_to_sid
 	   and sepol_compute_av_reason. */
diff --git a/libselinux/src/avc_sidtab.c b/libselinux/src/avc_sidtab.c
index 8dc87560..8c81cf65 100644
--- a/libselinux/src/avc_sidtab.c
+++ b/libselinux/src/avc_sidtab.c
@@ -56,7 +56,7 @@ int sidtab_insert(struct sidtab *s, const char * ctx)
 		rc = -1;
 		goto out;
 	}
-	newctx = (char *) strdup(ctx);
+	newctx = strdup(ctx);
 	if (!newctx) {
 		rc = -1;
 		avc_free(newnode);
diff --git a/libselinux/src/is_customizable_type.c b/libselinux/src/is_customizable_type.c
index 92876f4d..1b17860c 100644
--- a/libselinux/src/is_customizable_type.c
+++ b/libselinux/src/is_customizable_type.c
@@ -38,7 +38,7 @@ static int get_customizable_type_list(char *** retlist)
 			while (fgets_unlocked(buf, selinux_page_size, fp)
 			       && i < ctr) {
 				buf[strlen(buf) - 1] = 0;
-				list[i] = (char *) strdup(buf);
+				list[i] = strdup(buf);
 				if (!list[i]) {
 					unsigned int j;
 					for (j = 0; j < i; j++)
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 6fb9e1ff..999aa924 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -1152,7 +1152,7 @@ void selinux_restorecon_set_sehandle(struct selabel_handle *hndl)
 	unsigned char *fc_digest;
 	size_t num_specfiles, fc_digest_len;
 
-	fc_sehandle = (struct selabel_handle *) hndl;
+	fc_sehandle = hndl;
 	if (!fc_sehandle)
 		return;
 
-- 
2.31.1

[PATCH 23/25] libselinux: sidtab_sid_stats(): unify parameter name

[Christian Göttsche]

Found by clang-tidy.

libselinux/src/avc_sidtab.h:32:6: warning: function 'sidtab_sid_stats' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
void sidtab_sid_stats(struct sidtab *s, char *buf, int buflen) ;
     ^
libselinux/src/avc_sidtab.c:103:6: note: the definition seen here
void sidtab_sid_stats(struct sidtab *h, char *buf, int buflen)
     ^
libselinux/src/avc_sidtab.h:32:6: note: differing parameters are named here: ('s'), in definition: ('h')
void sidtab_sid_stats(struct sidtab *s, char *buf, int buflen) ;
     ^                               ~
                                     h

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

diff --git a/libselinux/src/avc_sidtab.c b/libselinux/src/avc_sidtab.c
index 8c81cf65..f179d855 100644
--- a/libselinux/src/avc_sidtab.c
+++ b/libselinux/src/avc_sidtab.c
@@ -100,7 +100,7 @@ sidtab_context_to_sid(struct sidtab *s,
 	return rc;
 }
 
-void sidtab_sid_stats(struct sidtab *h, char *buf, int buflen)
+void sidtab_sid_stats(struct sidtab *s, char *buf, int buflen)
 {
 	int i, chain_len, slots_used, max_chain_len;
 	struct sidtab_node *cur;
@@ -108,7 +108,7 @@ void sidtab_sid_stats(struct sidtab *h, char *buf, int buflen)
 	slots_used = 0;
 	max_chain_len = 0;
 	for (i = 0; i < SIDTAB_SIZE; i++) {
-		cur = h->htable[i];
+		cur = s->htable[i];
 		if (cur) {
 			slots_used++;
 			chain_len = 0;
@@ -124,7 +124,7 @@ void sidtab_sid_stats(struct sidtab *h, char *buf, int buflen)
 
 	snprintf(buf, buflen,
 		 "%s:  %u SID entries and %d/%d buckets used, longest "
-		 "chain length %d\n", avc_prefix, h->nel, slots_used,
+		 "chain length %d\n", avc_prefix, s->nel, slots_used,
 		 SIDTAB_SIZE, max_chain_len);
 }
 
-- 
2.31.1

[PATCH 24/25] libselinux: regex: unify parameter names

[Christian Göttsche]

Use the same parameter names as in the header `regex.h`.

Found by clang-tidy.

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

diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
index 770bc3ea..73987d9f 100644
--- a/libselinux/src/regex.c
+++ b/libselinux/src/regex.c
@@ -319,7 +319,7 @@ char const *regex_version(void)
 }
 
 int regex_load_mmap(struct mmap_area *mmap_area, struct regex_data **regex,
-		    int unused __attribute__((unused)), bool *regex_compiled)
+		    int do_load_precompregex __attribute__((unused)), bool *regex_compiled)
 {
 	int rc;
 	uint32_t entry_len;
@@ -387,7 +387,7 @@ static inline pcre_extra *get_pcre_extra(struct regex_data *regex)
 }
 
 int regex_writef(struct regex_data *regex, FILE *fp,
-		 int unused __attribute__((unused)))
+		 int do_write_precompregex __attribute__((unused)))
 {
 	int rc;
 	size_t len;
-- 
2.31.1

[PATCH 25/25] libselinux: label_file.c: fix indent

[Christian Göttsche]

Found by clang-tidy.

libselinux/src/label_file.c:374:4: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
                 else
                 ^

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

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index 39a56133..8139b38c 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -371,7 +371,7 @@ end_arch_check:
 
 		if (stem_id < 0 || stem_id >= (int32_t)stem_map_len)
 			spec->stem_id = -1;
-		 else
+		else
 			spec->stem_id = stem_map[stem_id];
 
 		/* retrieve the hasMetaChars bit */
-- 
2.31.1

Re: [PATCH 00/25] libselinux: misc compiler and static analyzer findings

[Petr Lautrbach]

Christian Göttsche <cgzones@googlemail.com> writes:

> Several changes found by compilers and static analyzer regarding const
> qualifiers, dead code, code smells and resource cleanup.
>
> Contains no changes with regard to libselinux callers.

Acked-by: Petr Lautrbach <plautrba@redhat.com>

All merged.

Thanks!


>
> Christian Göttsche (25):
>   libselinux: sidtab_hash(): do not discard const qualifier
>   libselinux: selinux_file_context_cmp(): do not discard const qualifier
>   libselinux: label_common(): do not discard const qualifier
>   libselinux: Sha1Finalise(): do not discard const qualifier
>   libselinux: sefcontext_compile: mark local variable static
>   libselinux: avcstat: use standard length modifier for unsigned long
>     long
>   libselinux: selinux_restorecon: mark local variable static
>   libselinux: selabel_get_digests_all_partial_matches: free memory after
>     FTS_D block
>   libselinux: getconlist: free memory on multiple level arguments
>   libselinux: exclude_non_seclabel_mounts(): drop unused variable
>   libselinux: context_new(): drop dead assignment
>   libselinux: label_x::init(): drop dead assignment
>   libselinux: label_media::init(): drop dead assignment
>   libselinux: setexecfilecon(): drop dead assignment
>   libselinux: getdefaultcon: free memory on multiple same arguments
>   libselinux: store_stem(): do not free possible non-heap object
>   libselinux: matchmediacon(): close file on error
>   libselinux: init_selinux_config(): free resources on error
>   libselinux: label_file::init(): do not pass NULL to strdup
>   libselinux: matchpathcon: free memory on realloc failure
>   libselinux: label_db::db_init(): open file with CLOEXEC mode
>   libselinux: drop redundant casts to the same type
>   libselinux: sidtab_sid_stats(): unify parameter name
>   libselinux: regex: unify parameter names
>   libselinux: label_file.c: fix indent
>
>  libselinux/src/audit2why.c                    |  2 +-
>  libselinux/src/avc_sidtab.c                   | 15 ++++-----
>  libselinux/src/context.c                      |  2 +-
>  libselinux/src/is_customizable_type.c         |  2 +-
>  libselinux/src/label_db.c                     |  2 +-
>  libselinux/src/label_file.c                   | 16 ++++++----
>  libselinux/src/label_file.h                   |  8 +++--
>  libselinux/src/label_media.c                  |  1 -
>  libselinux/src/label_x.c                      |  1 -
>  libselinux/src/matchmediacon.c                |  1 +
>  libselinux/src/matchpathcon.c                 | 32 +++++++++++--------
>  libselinux/src/regex.c                        |  4 +--
>  libselinux/src/selinux_config.c               |  7 +++-
>  libselinux/src/selinux_restorecon.c           |  7 ++--
>  libselinux/src/setexecfilecon.c               |  1 -
>  libselinux/src/sha1.c                         | 10 +++---
>  libselinux/src/sha1.h                         |  2 +-
>  libselinux/utils/avcstat.c                    |  4 +--
>  libselinux/utils/getconlist.c                 |  1 +
>  libselinux/utils/getdefaultcon.c              |  3 ++
>  libselinux/utils/sefcontext_compile.c         |  2 +-
>  .../selabel_get_digests_all_partial_matches.c | 10 +++---
>  22 files changed, 76 insertions(+), 57 deletions(-)
>
> -- 
> 2.31.1