Feature/file context duplicate entry check #183

gdesrosi · 2020-12-11T00:02:25Z

Adding check “E-010” to check for multiple specification of the same entry which setfiles
would report as an error during the policy build. Even if those two entries were identical,
setfiles would still report that as an error during the policy build. For instance,
'/foo – foo_context' and '/foo – foo_context' would be considered multiple specification
for the same entry just like '/foo – foo_context' and '/foo – bar_context' would be
considered multiple specification. This check helps detecting that kind of problem by
parsing all the fc entries and looking for multiple specifications across all other file
contexts instead of just comparing entries within each file since it is possible to have
multiple specifications across file contexts and that would also cause issues.

All findings are returned as selint errors with the file context names and line numbers
involved in the result of the violation.

Other changes to support addition of file context duplicate entry check as well, which include:

1)Addition of an fc_entry map to map.c to store all the fc entries
  across every file context.

2)Addition of the fc_entry_map_info and fc_entry_hash structs to
  store the relevant information into that map that will later be
  used in the file context duplicate entry check.

3)Modification of the fc_entry struct to take into account when an
  entry is within and ifdef/ifndef macro.

4)Modification of the parse_fc_file method to parse and store ifdef/ifndef
  definitions to relevant fc entry as well as storing those entry into the
  fc_entry_map which again will be used later in the file context
  duplicate entry check.

dburgener · 2020-12-12T20:31:32Z

src/check_hooks.h

@@ -67,6 +67,7 @@ enum error_ids {
 E_ID_UNKNOWN_PERM = 7,
 E_ID_UNKNOWN_CLASS = 8,
 E_ID_EMPTY_BLOCK = 9,
+ E_ID_FC_DUPLICATE_ENTRY = 10,


I just merged #179 which also added an error check and took E-010. Can you switch to E-011, please?

dburgener · 2020-12-12T20:33:43Z

src/fc_checks.c

+static bool is_same_fc_entry(const struct fc_entry *entry_one,
+ const struct fc_entry *entry_two) {
+
+ return !strcmp(entry_one->path, entry_two->path)


This is a ton of indentation. I get that you're trying to make the complicated block clear, but can you cut back to one tab at each level instead of two?

@dburgener I will address all the formatting issues that you've pointed out, but I had a question. So I'm using the eclipse c/c++ plugin and generally when I'm done with a function i use their default formatting template and I know it does all sorts of things like limiting number of letters per line, tab formatting and what not. I was wondering if you were using any sort of formatting template to format your code before submitting PRs or if there is a particular guideline that you use that maybe I can modify the formatting template to follow.

dburgener · 2020-12-12T20:38:59Z

src/fc_checks.c

+ node->data.fc_data->path);
+ struct check_result *ret = NULL;
+
+ /***********************************************************************************


Rather than the comment, just do:

if(!out) { alloc_internal_error("message here"); }

dburgener · 2020-12-12T20:47:33Z

src/fc_checks.c

+ * 2)two duplicates whereby one of them is within an ifdef/ifndef and the other *
+ * one is not within any. *
+ * *
+ * 3)two two duplicates whereby both are within the same ifdef/ifndef. by both *


repeated word

And "by" is the first word in a sentence and not capitalized.

dburgener · 2020-12-12T20:48:41Z

src/fc_checks.c

+ * here I'm not referring to literally be defined under the same conditional *
+ * although it would also return true in this case, rather I'm referring to the *
+ * parameters or conditions being the same. so if you had the same specification *
+ * under distro_redhat in two different files,it would treat them as being under *


missing space in "files,it"

dburgener · 2020-12-12T20:58:37Z

src/fc_checks.c

+ } else if (entry_one->conditional && entry_two->conditional) {
+ if (entry_one->conditional->flavor == entry_two->conditional->flavor
+ && (!strcmp(entry_one->conditional->condition,
+ entry_two->conditional->condition)


Please align multiple arguments to a function call together. You can tab up to the previous indentation and then use spaces for alignment

dburgener · 2020-12-12T21:49:38Z

src/maps.c

+ entry = malloc(sizeof(struct fc_entry_hash));
+ entry->key = strdup(info->entry->path);
+ entry->val = info;
+ HASH_ADD_KEYPTR(hh_fc_entry, fcs_entry_map, entry->key, strlen(entry->key), entry);


spaces instead of tabs

dburgener · 2020-12-12T21:50:03Z

src/maps.c

+ entry->val = info;
+ HASH_ADD_KEYPTR(hh_fc_entry, fcs_entry_map, entry->key, strlen(entry->key), entry);
+ }
+


eliminate both blank lines here please

cgzones

Some memory leaks in the test suite:

diff --git a/tests/check_fc_checks.c b/tests/check_fc_checks.c
index c61953c..8d6461b 100644
--- a/tests/check_fc_checks.c
+++ b/tests/check_fc_checks.c
@@ -422,9 +422,11 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 **************************************************/
 	node_entry->conditional = malloc(sizeof(struct conditional_data));
 	memset(node_entry->conditional, 0, sizeof(struct conditional_data));
+	free(node_entry->conditional->condition);
 	node_entry->conditional->condition = strdup("distro_gentoo");
 	node_entry->conditional->flavor = CONDITION_IFDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -437,6 +439,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 **************************************************/
 	node_entry->conditional->flavor = CONDITION_IFNDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -454,6 +457,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	map_entry->conditional->condition = strdup("distro_gentoo");
 	node_entry->conditional->flavor = CONDITION_IFDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -467,6 +471,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	node_entry->conditional->flavor = CONDITION_IFNDEF;
 	map_entry->conditional->flavor = CONDITION_IFNDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -480,6 +485,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 **************************************************/
 	node_entry->conditional->flavor = CONDITION_IFDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_null(res);
 
@@ -488,6 +494,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 * and the other one within ifndef and the        *
 	 * conditions differ                              *
 	 **************************************************/
+	free(node_entry->conditional->condition);
 	node_entry->conditional->condition = strdup("distro_redhat");
 
 	res = check_file_contexts_duplicate_entry(data, node);
@@ -502,6 +509,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 **************************************************/
 	map_entry->conditional->flavor = CONDITION_IFDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_null(res);
 
@@ -522,8 +530,10 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 * checking to find problematic duplicate          *
 	 * across different file contexts.                 *
 	 **************************************************/
+	free(data->filename);
 	data->filename = strdup("bar.fc");
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -531,8 +541,10 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	ck_assert_ptr_nonnull(res->message);
 
 	free_check_result(res);
+	free(data->filename);
 	free(data);
 	free_policy_node(node);
+	free_fc_entry(map_entry);
 	free_all_maps();
 }
 END_TEST

cgzones · 2020-12-13T11:37:12Z

src/parse_fc.c

+ token = strtok(NULL, "`");
+ token = strtok(token, "'");
+ ifdef_condition = malloc(strlen(token) + 1);
+ strcpy(ifdef_condition, token);


Maybe use strdup? (strcpy smells)

gdesrosi · 2020-12-13T14:11:23Z

@cgzones yes I'm aware of the memory leaks. I actually fixed those last night, but I'm waiting to get those code review comments fix addressed so i can do another push with everything included

gdesrosi · 2020-12-13T14:48:15Z

@dburgener in the free_check_result function there is no NULL check on res->message before attempting to free it. I'm wondering if there is a specific reason for that since we seem to consistently do that pretty much everywhere else. I ran into issues last night working on the test suite with that because i mistakenly called free_check_result on a NULL check_result, but then I thought it should have still checked that this was a valid pointer before attempting to free it, ignore it it it isn't, or error out with a message indicating that you are trying to free an invalid pointer.

dburgener · 2020-12-13T14:51:25Z

@gdesrosi The lack of a NULL check on res is an oversight, feel free to add it (and that's what I presume you're hitting). The lack of NULL check on res->message is intentional, since free(NULL); is a no-op.

gdesrosi · 2020-12-13T16:05:10Z

@dburgener yeah you're right the issue is res. I can fix that as part of this PR, but I was thinking maybe for traceability you may have wanted that to be done separately? It's small enough that it probably is not necessary to open a PR for that. I also asked you a question regarding whether or not we use formatting template for selint, or if we even have specific guidelines that we use that maybe I can use to tweak my eclipse so that it follows those. I'm not sure if you saw that already.

dburgener · 2020-12-13T17:39:46Z

I can fix that as part of this PR, but I was thinking maybe for traceability you may have wanted that to be done separately? It's small enough that it probably is not necessary to open a PR for that.

A separate commit would be good. Nothing is too small for its own PR, although I'm fine with putting this in its own commit in this PR.

I was wondering if you were using any sort of formatting template to format your code before submitting PRs or if there is a particular guideline that you use that maybe I can modify the formatting template to follow.

Unfortunately nothing documented. I'd like to run an automated checker, but unfortunately last time I looked I couldn't find one that did what I want. The main things I see in this PR are indentation issues. We want tabs for indentation and spaces for alignment (tab to the indent level then space to the alignment point). One tab per indentation level. Align multiple boolean expressions so its clear where the nesting is.

The best general rule I can give you is to try and follow the style the code is already doing, but I recognize that that's less helpful than a tool or document.

gdesrosi · 2020-12-14T13:59:52Z

@dburgener @stevedlawrence @cgzones Does anyone know what could cause "make check-valgrind" to not return any memory leaks for me locally, but failing on the automated PR checker?

stevedlawrence · 2020-12-14T14:12:57Z

Seems to have to do with the options you provide when you run ./configure. The github actions runs the following when configuring with gcc:

./configure --enable-werror CFLAGS="-Wno-error=conversion -Wno-error=sign-conversion --coverage"

With my usual ./configure options I wasn't getting any errors like you, but with the above I can dupliate the leak.

gdesrosi · 2020-12-14T14:26:38Z

Thanks @stevedlawrence

dburgener · 2020-12-14T16:24:01Z

src/maps.c

@@ -27,6 +27,7 @@ static struct hash_elem *perm_map = NULL;
 static struct hash_elem *mods_map = NULL;
 static struct hash_elem *mod_layers_map = NULL;
 static struct hash_elem *ifs_map = NULL;
+static struct fc_entry_hash *fcs_entry_map = NULL;


Maybe I'm just missing something, but is there a reason this is named "fcs_entry" instead of "fc_entries"?

No reason other than me being bad at my variable naming. Well I guess I can also blame that on English not being my first language lol

dburgener · 2020-12-14T16:25:37Z

src/fc_checks.c

+ * one is not within any. *
+ * *
+ * 3)Two duplicates whereby both are within the same ifdef/ifndef. By both *
+ * here I'm not referring to literally be defined under the same conditional *


Can you reword to eliminate the use of first person singular in comments?

dburgener · 2020-12-14T16:27:41Z

src/parse_fc.c

+ token = strtok(line, "`");
+ if (token) {
+ token = strtok(NULL, "`");
+ token = strtok(token, "'");


Shouldn't this be strtok(NULL, "'");" since you're continuing in the same string?

yes that is a mistake.

dburgener · 2020-12-14T16:27:53Z

src/parse_fc.c

+ token = strtok(line, "`");
+ if (token) {
+ token = strtok(NULL, "`");
+ token = strtok(token, "'");


Same question as above

dburgener · 2020-12-14T16:30:28Z

src/parse_fc.c

+ ifndef_condition = strdup(token);
+ }
+ continue;
+ } else if (!strncmp(line, "')", 2)) {


The lack of durability here scares me. Ending an ifdef block with eg ' ) (note space) would be legal, and missed by this.

The durable solution is probably a fairly large and high risk change, so maybe for now, just a TODO comment noting the issue? SELint does generally assume reasonable style, but this one seems pretty annoying if it were to hit.

dburgener · 2020-12-14T17:21:05Z

src/parse_fc.c

+ ifndef_condition = NULL;
+ }
+ continue;
+ } else if (!strncmp(line, "', `", 4) || !strncmp(line, "',`", 3)) { // Skip over m4 constructs


This looks broken. This would typically flip the ifdef. These are for situations like:

ifdef(`foo',` /some/path label1 ',` /some/path label2 ') In order to assign label1 if foo was set and label2 if it's not. If I'm reading this right, this would treat both as being in the ifdef and result in a false positive in this scenario.

that is correct. I failed to take this situation into account. so I'll need to rework this

dburgener · 2020-12-14T17:23:20Z

src/parse_fc.c

+ }
+
+ struct fc_entry *entry = parse_fc_line(line, conditional);
+ free(conditional);


Given that you're freeing it immediately after the call, why not allocate on the stack rather than the heap?

On second thought, you allocate a copy of it in parse_fc_line(). Why not allocate it here and then take ownership of that copy in parse_fc_line?

dburgener · 2020-12-14T17:28:45Z

src/parse_fc.c

+ } else if (is_within_ifndef) {
+ conditional = malloc(sizeof(struct conditional_data));
+ conditional->flavor = CONDITION_IFNDEF;
+ conditional->condition = strdup(ifndef_condition);


These strings will get leaked if there is a parse error in the line (the fc_entry node will never take ownership of them.

You either need to check for (and free) them in the cleanup tag in parse_fc_line(), or sidestep all the memory management issues here by just passing the flavor and string to parse_fc_line directly.

dburgener · 2020-12-14T17:32:57Z

src/parse_fc.c

+ if (token) {
+ token = strtok(NULL, "`");
+ token = strtok(token, "'");
+ ifdef_condition = malloc(strlen(token) + 1);


No need for malloc now that you're strduping. malloc will leak memory now.

…ich include: 1)Addition of an fc_entry map to map.c to store all the fc entries across every file context. 2)Addition of the fc_entry_map_info and fc_entry_hash structs to store the relevant information into that map that will later be used in the file context duplicate entry check. 3)Modification of the fc_entry struct to take into account when an entry is within and ifdef/ifndef macro. 4)Modification of the parse_fc_file method to parse and store ifdef/ifndef definitions to relevant fc entry as well as storing those entry into the fc_entry_map which again will be used later in the file context duplicate entry check.

…entry which setfiles would report as an error during the policy build. Even if those two entries were identical, setfiles would still report that as an error during the policy build. For instance, '/foo – foo_context' and '/foo – foo_context' would be considered multiple specification for the same entry just like '/foo – foo_context' and '/foo – bar_context' would be considered multiple specification. This check helps detecting that kind of problem by parsing all the fc entries and looking for multiple specifications across all other file contexts instead of just comparing entries within each file since it is possible to have multiple specifications across file contexts and that would also cause issues. All findings are returned as selint errors with the file context names and line numbers involve in the result of the violation.

cgzones · 2020-12-15T13:55:29Z

Got crashes with clang sanitizers with the functional bats tests:

typfc_checks.c:248:16: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:138:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior fc_checks.c:248:16 in AddressSanitizer:DEADLYSIGNAL =================================================================
==12920==ERROR:
AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004325e5 bp 0x7ffd4d3b9740 sp 0x7ffd4d3b8ee0 T0)
==12920==The signal is caused by a READ memory access.
==12920==Hint: address points to the zero page. #0 0x4325e5 in strcmp (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5)
 #1 0x60e1a5 in is_same_fc_entry /home/christian/Coding/workspaces/selint/src/fc_checks.c:247:9
 #2 0x60ce27 in check_file_contexts_duplicate_entry /home/christian/Coding/workspaces/selint/src/fc_checks.c:376:8
 #3 0x608c72 in call_checks_for_node_type /home/christian/Coding/workspaces/selint/src/check_hooks.c:77:30
 #4 0x608a56 in call_checks /home/christian/Coding/workspaces/selint/src/check_hooks.c:62:9
 #5 0x602887 in run_checks_on_one_file /home/christian/Coding/workspaces/selint/src/runner.c:350:27
 #6 0x602c92 in run_all_checks /home/christian/Coding/workspaces/selint/src/runner.c:391:4
 #7 0x603307 in run_analysis /home/christian/Coding/workspaces/selint/src/runner.c:477:8
 #8 0x4cbc99 in main /home/christian/Coding/workspaces/selint/src/main.c:597:26
 #9 0x7fd288442d09 in __libc_start_main csu/../csu/libc-start.c:308:16
 #10 0x41f3e9 in _start (/home/christian/Coding/workspaces/selint/src/selint+0x41f3e9)
AddressSanitizer can not provide additional info. 
SUMMARY: AddressSanitizer: SEGV (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5) in strcmp
==12920==ABORTING

typfc_checks.c:284:42: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:138:33: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior fc_checks.c:284:42 in AddressSanitizer:
DEADLYSIGNAL =================================================================
==16367==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004325e5 bp 0x7ffc5307b810 sp 0x7ffc5307afb0 T0)
==16367==The signal is caused by a READ memory access.
==16367==Hint: address points to the zero page. #0 0x4325e5 in strcmp (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5)
 #1 0x60fdf9 in is_multiple_fc_entry_spec /home/christian/Coding/workspaces/selint/src/fc_checks.c:284:8
 #2 0x60d05e in check_file_contexts_duplicate_entry /home/christian/Coding/workspaces/selint/src/fc_checks.c:404:15
 #3 0x608c72 in call_checks_for_node_type /home/christian/Coding/workspaces/selint/src/check_hooks.c:77:30
 #4 0x608a56 in call_checks /home/christian/Coding/workspaces/selint/src/check_hooks.c:62:9
 #5 0x602887 in run_checks_on_one_file /home/christian/Coding/workspaces/selint/src/runner.c:350:27
 #6 0x602c92 in run_all_checks /home/christian/Coding/workspaces/selint/src/runner.c:391:4
 #7 0x603307 in run_analysis /home/christian/Coding/workspaces/selint/src/runner.c:477:8
 #8 0x4cbc99 in main /home/christian/Coding/workspaces/selint/src/main.c:597:26
 #9 0x7f495c3d0d09 in __libc_start_main csu/../csu/libc-start.c:308:16
 #10 0x41f3e9 in _start (/home/christian/Coding/workspaces/selint/src/selint+0x41f3e9)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5) in strcmp
==16367==ABORTING

suggested change:

diff --git a/src/fc_checks.c b/src/fc_checks.c
index eb726bf..d37cdf6 100644
--- a/src/fc_checks.c
+++ b/src/fc_checks.c
@@ -233,6 +233,19 @@ struct check_result *check_file_context_types_exist(__attribute__((unused)) cons
        return NULL;
 }
 
+static bool null_str_eq(const char *a, const char *b)
+{
+       if (a == b) {
+               return true;
+       }
+
+       if (!a || !b) {
+               return false;
+       }
+
+       return strcmp(a, b) == 0;
+}
+
 /**********************************
  * Return true if the two fc_entry nodes are the same
  * and false otherwise
@@ -243,15 +256,15 @@ static bool is_same_fc_entry(const struct fc_entry *entry_one,
        return !strcmp(entry_one->path, entry_two->path)
                && ((!entry_one->context && !entry_two->context)        //when <<none>>
                        || (entry_one->obj == entry_two->obj
-                               && !strcmp(entry_one->path, entry_two->path)
-                               && !strcmp(entry_one->context->range,
-                                          entry_two->context->range)
-                               && !strcmp(entry_one->context->role,
-                                          entry_two->context->role)
-                               && !strcmp(entry_one->context->type,
-                                          entry_two->context->type)
-                               && !strcmp(entry_one->context->user,
-                                          entry_two->context->user)));
+                               && null_str_eq(entry_one->path, entry_two->path)
+                               && null_str_eq(entry_one->context->range,
+                                              entry_two->context->range)
+                               && null_str_eq(entry_one->context->role,
+                                              entry_two->context->role)
+                               && null_str_eq(entry_one->context->type,
+                                              entry_two->context->type)
+                               && null_str_eq(entry_one->context->user,
+                                              entry_two->context->user)));
 }
 
 /**********************************
@@ -268,13 +281,13 @@ static bool is_multiple_fc_entry_spec(const struct fc_entry *entry_one,
                return !strcmp(entry_one->path, entry_two->path)
                        && entry_one->obj == entry_two->obj
                        && !strcmp(entry_one->path, entry_two->path)
-                       && (strcmp(entry_one->context->range, entry_two->context->range)
-                               || strcmp(entry_one->context->role,
-                                         entry_two->context->role)
-                               || strcmp(entry_one->context->type,
-                                         entry_two->context->type)
-                               || strcmp(entry_one->context->user,
-                                         entry_two->context->user));
+                       && (!null_str_eq(entry_one->context->range, entry_two->context->range)
+                               || !null_str_eq(entry_one->context->role,
+                                               entry_two->context->role)
+                               || !null_str_eq(entry_one->context->type,
+                                               entry_two->context->type)
+                               || !null_str_eq(entry_one->context->user,
+                                               entry_two->context->user));
        }
 }
 
diff --git a/tests/functional/end-to-end.bats b/tests/functional/end-to-end.bats
index b66cf5a..a9a5b12 100644
--- a/tests/functional/end-to-end.bats
+++ b/tests/functional/end-to-end.bats
@@ -312,6 +312,7 @@ test_parse_error_impl() {
 
 @test "Enable/disable" {
        run ${SELINT_PATH} -c configs/empty.conf -e W-002 -e W-003 -d S-002 -d C-002 -r -s policies/check_triggers
+       echo ${output}
        [ "$status" -eq 0 ]
        count=$(echo ${output} | grep -o "S-002" | wc -l)
        [ "$count" -eq 0 ]

gdesrosi · 2020-12-15T14:19:20Z

@cgzones Im still trying to figure out how to configure my environment so that it runs all the checks that the automated PR checker checks for. I tried running configure with those flags ./configure --enable-werror CFLAGS="-Wno-error=conversion -Wno-error=sign-conversion --coverage" which @stevedlawrence had suggested yesterday but I was getting build error with those flags on lex even on a clean checkout of master. I emailed @dburgener about that yesterday,but since I finished addressing those code review comments from yesterday I did a push, but I knew there were likely issues that the automated PR checker would detect.

cgzones · 2020-12-15T14:28:08Z

./configure --enable-werror CFLAGS="-Wno-error=conversion -Wno-error=sign-conversion --coverage"
make check -j

works fine for me on Debian sid.
Can you share your build failures?

There a re actually two testsuites:
I. a libcheck based (tests/check_*.c), run by make check
II. a bats based (tests/functional/end-to-end.bats), run by cd tests/functional/ && bats end-to-end.bats

gdesrosi · 2020-12-15T14:40:36Z

@cgzones here is what I'm seeing. I'm running on a centos8

make all-recursive
make[1]: Entering directory '/home/gdesrosiers/SELINT_COPY/selint'
Making all in src
make[2]: Entering directory '/home/gdesrosiers/SELINT_COPY/selint/src'
make all-am
make[3]: Entering directory '/home/gdesrosiers/SELINT_COPY/selint/src'
depbase=echo lex.o | sed 's|[^/]*$|.deps/&|;s|\.o$||';
gcc -DHAVE_CONFIG_H -I. -I.. -Wall -Wextra -Wcast-qual -Wconversion -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Werror -DSYSCONFDIR='"/usr/local/etc"' -Wno-error=conversion -Wno-error=sign-conversion --coverage -MT lex.o -MD -MP -MF $depbase.Tpo -c -o lex.o lex.c &&
mv -f $depbase.Tpo $depbase.Po
lex.c: In function ‘yylex’:
lex.c:1203:23: error: comparison of integer expressions of different signedness: ‘yy_size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
for ( yyl = 0; yyl < yyleng; ++yyl )
^
lex.c: In function ‘yy_get_next_buffer’:
lex.c:1867:42: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
YY_CURRENT_BUFFER_LVALUE->yy_buf_size - number_to_move - 1;
^
lex.c:294:34: warning: conversion from ‘yy_size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
#define YY_CURRENT_BUFFER_LVALUE yyg->yy_buffer_stack[yyg->yy_buffer_stack_top]
^~~
lex.c:1867:4: note: in expansion of macro ‘YY_CURRENT_BUFFER_LVALUE’
YY_CURRENT_BUFFER_LVALUE->yy_buf_size - number_to_move - 1;
^~~~~~~~~~~~~~~~~~~~~~~~
lex.c:1901:56: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
num_to_read = YY_CURRENT_BUFFER_LVALUE->yy_buf_size -
^
lex.c:294:34: warning: conversion from ‘yy_size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
#define YY_CURRENT_BUFFER_LVALUE yyg->yy_buffer_stack[yyg->yy_buffer_stack_top]
^~~
lex.c:1901:18: note: in expansion of macro ‘YY_CURRENT_BUFFER_LVALUE’
num_to_read = YY_CURRENT_BUFFER_LVALUE->yy_buf_size -
^~~~~~~~~~~~~~~~~~~~~~~~
lex.c:1935:29: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
if ((int) (yyg->yy_n_chars + number_to_move) > YY_CURRENT_BUFFER_LVALUE->yy_buf_size) {
^
lex.c:1937:34: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
int new_size = yyg->yy_n_chars + number_to_move + (yyg->yy_n_chars >> 1);
^
lex.c:1937:51: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
int new_size = yyg->yy_n_chars + number_to_move + (yyg->yy_n_chars >> 1);
^
lex.c:1937:18: warning: conversion from ‘yy_size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
int new_size = yyg->yy_n_chars + number_to_move + (yyg->yy_n_chars >> 1);
^~~
lex.c:1943:18: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
yyg->yy_n_chars += number_to_move;
^~
lex.c:1943:21: warning: conversion from ‘yy_size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
yyg->yy_n_chars += number_to_move;
^~~~~~~~~~~~~~
lex.c: In function ‘yyensure_buffer_stack’:
lex.c:2339:23: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
(num_to_alloc * sizeof(struct yy_buffer_state*)
^
lex.c:2344:48: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
memset(yyg->yy_buffer_stack, 0, num_to_alloc * sizeof(struct yy_buffer_state*));
^
lex.c:2346:30: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
yyg->yy_buffer_stack_max = num_to_alloc;
^~~~~~~~~~~~
lex.c:2356:18: warning: conversion from ‘long unsigned int’ to ‘int’ may change value [-Wconversion]
num_to_alloc = yyg->yy_buffer_stack_max + grow_size;
^~~
lex.c:2359:22: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
num_to_alloc * sizeof(struct yy_buffer_state*)
^
lex.c:2366:30: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
yyg->yy_buffer_stack_max = num_to_alloc;
^~~~~~~~~~~~

dburgener · 2020-12-15T14:56:04Z

I think this is your problem:

lex.c:1203:23: error: comparison of integer expressions of different signedness: ‘yy_size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]

The rest of the warnings do seem to be getting treated as (EDIT: NOT) errors. Possibly an older version of lex than we've seen?

Try adding -Wno-error=sign-compare in your ./configure with the other -Wno-error flags and see if that resolves it. You should expect to still see the warnings, but they shouldn't break your build.

gdesrosi · 2020-12-15T20:28:36Z

@cgzones what are you running bats with to get output like this?

==16367==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004325e5 bp 0x7ffc5307b810 sp 0x7ffc5307afb0 T0)
==16367==The signal is caused by a READ memory access.
==16367==Hint: address points to the zero page. #0 0x4325e5 in strcmp (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5)
#1 0x60fdf9 in is_multiple_fc_entry_spec /home/christian/Coding/workspaces/selint/src/fc_checks.c:284:8
#2 0x60d05e in check_file_contexts_duplicate_entry /home/christian/Coding/workspaces/selint/src/fc_checks.c:404:15
#3 0x608c72 in call_checks_for_node_type /home/christian/Coding/workspaces/selint/src/check_hooks.c:77:30
#4 0x608a56 in call_checks /home/christian/Coding/workspaces/selint/src/check_hooks.c:62:9
#5 0x602887 in run_checks_on_one_file /home/christian/Coding/workspaces/selint/src/runner.c:350:27
#6 0x602c92 in run_all_checks /home/christian/Coding/workspaces/selint/src/runner.c:391:4

I tried running that with valgrind I couldn't get that kind of output

cgzones · 2020-12-15T20:39:17Z

./configure CC=clang-11 CFLAGS="-O1 -g -fsanitize=address -fsanitize-address-use-after-scope -fno-omit-frame-pointer -fsanitize=undefined -fsanitize=nullability -fsanitize=implicit-conversion -fsanitize=integer -fsanitize=float-divide-by-zero -fsanitize=local-bounds"
make
cd tests/functional/ && bats end-to-end.bats

gdesrosi · 2020-12-15T20:43:07Z

Thanks @cgzones

gdesrosi force-pushed the feature/file-context-duplicate-entry-check branch 3 times, most recently from 0c29cb5 to 412a52a Compare December 12, 2020 05:38

dburgener reviewed Dec 12, 2020

View reviewed changes

cgzones reviewed Dec 13, 2020

View reviewed changes

gdesrosi force-pushed the feature/file-context-duplicate-entry-check branch 5 times, most recently from a149031 to a06992d Compare December 14, 2020 04:41

dburgener reviewed Dec 14, 2020

View reviewed changes

gdesrosi added 3 commits December 14, 2020 23:18

Ensuring res is a valid pointer before attempting to free res->message

6405cb6

gdesrosi force-pushed the feature/file-context-duplicate-entry-check branch from a06992d to 0fdb8ff Compare December 15, 2020 04:23

Feature/file context duplicate entry check #183

Are you sure you want to change the base?

Feature/file context duplicate entry check #183

Conversation

gdesrosi commented Dec 11, 2020

Choose a reason for hiding this comment

dburgener Dec 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgzones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdesrosi commented Dec 13, 2020 • edited

gdesrosi commented Dec 13, 2020 • edited

dburgener commented Dec 13, 2020

gdesrosi commented Dec 13, 2020

dburgener commented Dec 13, 2020

gdesrosi commented Dec 14, 2020

stevedlawrence commented Dec 14, 2020 • edited

gdesrosi commented Dec 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgzones commented Dec 15, 2020

gdesrosi commented Dec 15, 2020

cgzones commented Dec 15, 2020

gdesrosi commented Dec 15, 2020

dburgener commented Dec 15, 2020 • edited

gdesrosi commented Dec 15, 2020

cgzones commented Dec 15, 2020

gdesrosi commented Dec 15, 2020

dburgener Dec 12, 2020 •

edited

gdesrosi commented Dec 13, 2020 •

edited

gdesrosi commented Dec 13, 2020 •

edited

stevedlawrence commented Dec 14, 2020 •

edited

dburgener commented Dec 15, 2020 •

edited