Feature/file context duplicate entry check - #183
Conversation
0c29cb5 to
412a52a
Compare
| E_ID_UNKNOWN_PERM = 7, | ||
| E_ID_UNKNOWN_CLASS = 8, | ||
| E_ID_EMPTY_BLOCK = 9, | ||
| E_ID_FC_DUPLICATE_ENTRY = 10, |
There was a problem hiding this comment.
I just merged #179 which also added an error check and took E-010. Can you switch to E-011, please?
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
| node->data.fc_data->path); | ||
| struct check_result *ret = NULL; | ||
|
|
||
| /*********************************************************************************** |
There was a problem hiding this comment.
Rather than the comment, just do:
if(!out) {
alloc_internal_error("message here");
}
| * 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 * |
There was a problem hiding this comment.
And "by" is the first word in a sentence and not capitalized.
| * 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 * |
| } 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) |
There was a problem hiding this comment.
Please align multiple arguments to a function call together. You can tab up to the previous indentation and then use spaces for alignment
| 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); |
| entry->val = info; | ||
| HASH_ADD_KEYPTR(hh_fc_entry, fcs_entry_map, entry->key, strlen(entry->key), entry); | ||
| } | ||
|
|
There was a problem hiding this comment.
eliminate both blank lines here please
cgzones
left a comment
cgzones
left a comment
There was a problem hiding this comment.
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| token = strtok(NULL, "`"); | ||
| token = strtok(token, "'"); | ||
| ifdef_condition = malloc(strlen(token) + 1); | ||
| strcpy(ifdef_condition, token); |
There was a problem hiding this comment.
Maybe use strdup? (strcpy smells)
|
@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 |
|
@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. |
|
@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. |
|
@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. |
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.
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. |
a149031 to
a06992d
Compare
|
@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? |
|
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. |
|
Thanks @stevedlawrence |
| 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; |
There was a problem hiding this comment.
Maybe I'm just missing something, but is there a reason this is named "fcs_entry" instead of "fc_entries"?
There was a problem hiding this comment.
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
| * 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 * |
There was a problem hiding this comment.
Can you reword to eliminate the use of first person singular in comments?
| token = strtok(line, "`"); | ||
| if (token) { | ||
| token = strtok(NULL, "`"); | ||
| token = strtok(token, "'"); |
There was a problem hiding this comment.
Shouldn't this be strtok(NULL, "'");" since you're continuing in the same string?
| token = strtok(line, "`"); | ||
| if (token) { | ||
| token = strtok(NULL, "`"); | ||
| token = strtok(token, "'"); |
| ifndef_condition = strdup(token); | ||
| } | ||
| continue; | ||
| } else if (!strncmp(line, "')", 2)) { |
There was a problem hiding this comment.
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.
| ifndef_condition = NULL; | ||
| } | ||
| continue; | ||
| } else if (!strncmp(line, "', `", 4) || !strncmp(line, "',`", 3)) { // Skip over m4 constructs |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
that is correct. I failed to take this situation into account. so I'll need to rework this
| } | ||
|
|
||
| struct fc_entry *entry = parse_fc_line(line, conditional); | ||
| free(conditional); |
There was a problem hiding this comment.
Given that you're freeing it immediately after the call, why not allocate on the stack rather than the heap?
There was a problem hiding this comment.
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?
| } else if (is_within_ifndef) { | ||
| conditional = malloc(sizeof(struct conditional_data)); | ||
| conditional->flavor = CONDITION_IFNDEF; | ||
| conditional->condition = strdup(ifndef_condition); |
There was a problem hiding this comment.
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.
| if (token) { | ||
| token = strtok(NULL, "`"); | ||
| token = strtok(token, "'"); | ||
| ifdef_condition = malloc(strlen(token) + 1); |
There was a problem hiding this comment.
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.
a06992d to
0fdb8ff
Compare
|
Got crashes with clang sanitizers with the functional bats tests: 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 ] |
|
@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. |
works fine for me on Debian sid. There a re actually two testsuites: |
|
@cgzones here is what I'm seeing. I'm running on a centos8 make all-recursive |
|
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. |
|
@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) I tried running that with valgrind I couldn't get that kind of output |
|
|
Thanks @cgzones |
5 participants
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: