* [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed
@ 2021-08-30 18:24 James Carter
2021-08-30 18:24 ` [PATCH 2/3] libsepol/cil: Refactor the function __cil_build_ast_node_helper() James Carter
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: James Carter @ 2021-08-30 18:24 UTC (permalink / raw)
To: selinux; +Cc: James Carter
If an optional that is to be disabled is the child of an optional that
is going to be disabled, then there is no reason to add that optional
to the stack of disabled optionals, because it is going to be destroyed
anyways. This means that there is no reason to maintain a stack of
disabled optionals at all.
Instead of using a stack to track disabled optionals, use a pointer
that points to the top-most optional that is to be disabled. When a
rule fails to resolve in an optional, if the disabled optional pointer
has not been set, then set it to that optional. If the pointer has
been set already, then the optional is already going to be destroyed,
so nothing else needs to be done. The resolution failure and the fact
that the optional is being disabled is reported in either case.
Signed-off-by: James Carter <jwcart2@gmail.com>
---
libsepol/cil/src/cil_resolve_ast.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 18007324..aeedc7dd 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -56,6 +56,7 @@ struct cil_args_resolve {
struct cil_tree_node *block;
struct cil_tree_node *macro;
struct cil_tree_node *optional;
+ struct cil_tree_node *disabled_optional;
struct cil_tree_node *boolif;
struct cil_list *sidorder_lists;
struct cil_list *classorder_lists;
@@ -63,7 +64,6 @@ struct cil_args_resolve {
struct cil_list *catorder_lists;
struct cil_list *sensitivityorder_lists;
struct cil_list *in_list;
- struct cil_stack *disabled_optionals;
};
static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key, struct cil_tree_node *ast_node)
@@ -3873,7 +3873,6 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
struct cil_tree_node *macro = args->macro;
struct cil_tree_node *optional = args->optional;
struct cil_tree_node *boolif = args->boolif;
- struct cil_stack *disabled_optionals = args->disabled_optionals;
if (node == NULL) {
goto exit;
@@ -3956,7 +3955,9 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
if (optional == NULL) {
cil_tree_log(node, CIL_ERR, "Failed to resolve %s statement", cil_node_to_string(node));
} else {
- cil_stack_push(disabled_optionals, CIL_NODE, optional);
+ if (!args->disabled_optional) {
+ args->disabled_optional = optional;
+ }
cil_tree_log(node, CIL_INFO, "Failed to resolve %s statement", cil_node_to_string(node));
cil_tree_log(optional, CIL_INFO, "Disabling optional '%s'", DATUM(optional->data)->name);
rc = SEPOL_OK;
@@ -4003,7 +4004,6 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
{
int rc = SEPOL_ERR;
struct cil_args_resolve *args = extra_args;
- struct cil_stack *disabled_optionals = args->disabled_optionals;
struct cil_tree_node *parent = NULL;
if (current == NULL || extra_args == NULL) {
@@ -4026,11 +4026,10 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
args->macro = NULL;
} else if (parent->flavor == CIL_OPTIONAL) {
struct cil_tree_node *n = parent->parent;
- struct cil_stack_item *item = cil_stack_peek(disabled_optionals);
- if (item && item->data == parent) {
- cil_stack_pop(disabled_optionals);
+ if (args->disabled_optional == parent) {
*(args->changed) = CIL_TRUE;
cil_list_append(args->to_destroy, CIL_NODE, parent);
+ args->disabled_optional = NULL;
}
args->optional = NULL;
while (n && n->flavor != CIL_ROOT) {
@@ -4067,6 +4066,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
extra_args.block = NULL;
extra_args.macro = NULL;
extra_args.optional = NULL;
+ extra_args.disabled_optional = NULL;
extra_args.boolif= NULL;
extra_args.sidorder_lists = NULL;
extra_args.classorder_lists = NULL;
@@ -4074,7 +4074,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
extra_args.catorder_lists = NULL;
extra_args.sensitivityorder_lists = NULL;
extra_args.in_list = NULL;
- extra_args.disabled_optionals = NULL;
cil_list_init(&extra_args.to_destroy, CIL_NODE);
cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
@@ -4083,7 +4082,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
cil_list_init(&extra_args.in_list, CIL_IN);
- cil_stack_init(&extra_args.disabled_optionals);
for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
extra_args.pass = pass;
@@ -4218,7 +4216,6 @@ exit:
__cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
cil_list_destroy(&extra_args.in_list, CIL_FALSE);
- cil_stack_destroy(&extra_args.disabled_optionals);
return rc;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] libsepol/cil: Refactor the function __cil_build_ast_node_helper()
2021-08-30 18:24 [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed James Carter
@ 2021-08-30 18:24 ` James Carter
2021-08-30 18:24 ` [PATCH 3/3] libsepol/cil: Simplify cil_tree_children_destroy() James Carter
2021-09-01 19:25 ` [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed Nicolas Iooss
2 siblings, 0 replies; 7+ messages in thread
From: James Carter @ 2021-08-30 18:24 UTC (permalink / raw)
To: selinux; +Cc: James Carter
Refactor the function __cil_build_ast_node_helper() by moving the
check for illegal statements and the large if-then-else statement
to determine which function to call to parse the policy statements
to different functions.
There is no need to keep walking the nodes of a policy statement
that has already been completely parsed. This means that the
remaining nodes of any policy statement that does not contain a list
of policy statements can be skipped. This was done inconsistently
before. The following policy statements now have all nodes after
the first one skipped: blockinherit, blockabstract, classcommon,
user, userattribute, userbounds, userprefix, type, typeattribute,
typealias, typealiasactual, typebounds, typepermissive, role,
userrole, roletype, roletransition, roleallow, roleattribute,
rolebounds, bool, tunable, typetransition, typechange, typemember,
sensitivity, sensitivityalias, senistivityaliasactual, category,
categoryalias, categoryaliasactual, and ipaddr. The only policy
statements that do contain a list of policy statements are:
block, in, tunableif, booleanif, true (conditional block), false
(conditional block), macro, optional, and src_info.
Signed-off-by: James Carter <jwcart2@gmail.com>
---
libsepol/cil/src/cil_build_ast.c | 416 ++++++++++++++-----------------
1 file changed, 184 insertions(+), 232 deletions(-)
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index a5afc267..521c3012 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -6108,77 +6108,47 @@ void cil_destroy_src_info(struct cil_src_info *info)
free(info);
}
-int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *finished, void *extra_args)
+static int check_for_illegal_statement(struct cil_tree_node *parse_current, struct cil_args_build *args)
{
- struct cil_args_build *args = extra_args;
- struct cil_db *db = args->db;
- struct cil_tree_node *ast_current = args->ast;
- struct cil_tree_node *tunif = args->tunif;
- struct cil_tree_node *in = args->in;
- struct cil_tree_node *macro = args->macro;
- struct cil_tree_node *optional = args->optional;
- struct cil_tree_node *boolif = args->boolif;
- struct cil_tree_node *ast_node = NULL;
- int rc = SEPOL_ERR;
-
- if (parse_current->parent->cl_head != parse_current) {
- /* ignore anything that isn't following a parenthesis */
- rc = SEPOL_OK;
- goto exit;
- } else if (parse_current->data == NULL) {
- /* the only time parenthesis can immediately following parenthesis is if
- * the parent is the root node */
- if (parse_current->parent->parent == NULL) {
- rc = SEPOL_OK;
- } else {
- cil_tree_log(parse_current, CIL_ERR, "Keyword expected after open parenthesis");
- }
- goto exit;
- }
-
- if (tunif != NULL) {
+ if (args->tunif != NULL) {
if (parse_current->data == CIL_KEY_TUNABLE) {
- rc = SEPOL_ERR;
cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in tunableif", (char *)parse_current->data);
- goto exit;
+ return SEPOL_ERR;
}
}
- if (in != NULL) {
+ if (args->in != NULL) {
if (parse_current->data == CIL_KEY_TUNABLE ||
parse_current->data == CIL_KEY_IN) {
- rc = SEPOL_ERR;
cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in in-statement", (char *)parse_current->data);
- goto exit;
+ return SEPOL_ERR;
}
}
- if (macro != NULL) {
+ if (args->macro != NULL) {
if (parse_current->data == CIL_KEY_TUNABLE ||
parse_current->data == CIL_KEY_IN ||
parse_current->data == CIL_KEY_BLOCK ||
parse_current->data == CIL_KEY_BLOCKINHERIT ||
parse_current->data == CIL_KEY_BLOCKABSTRACT ||
parse_current->data == CIL_KEY_MACRO) {
- rc = SEPOL_ERR;
cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in macro", (char *)parse_current->data);
- goto exit;
+ return SEPOL_ERR;
}
}
- if (optional != NULL) {
+ if (args->optional != NULL) {
if (parse_current->data == CIL_KEY_TUNABLE ||
parse_current->data == CIL_KEY_IN ||
parse_current->data == CIL_KEY_BLOCK ||
parse_current->data == CIL_KEY_BLOCKABSTRACT ||
parse_current->data == CIL_KEY_MACRO) {
- rc = SEPOL_ERR;
cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in optional", (char *)parse_current->data);
- goto exit;
+ return SEPOL_ERR;
}
}
- if (boolif != NULL) {
+ if (args->boolif != NULL) {
if (parse_current->data != CIL_KEY_TUNABLEIF &&
parse_current->data != CIL_KEY_CALL &&
parse_current->data != CIL_KEY_CONDTRUE &&
@@ -6189,338 +6159,320 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
parse_current->data != CIL_KEY_TYPETRANSITION &&
parse_current->data != CIL_KEY_TYPECHANGE &&
parse_current->data != CIL_KEY_TYPEMEMBER) {
- rc = SEPOL_ERR;
- if (((struct cil_booleanif*)boolif->data)->preserved_tunable) {
+ if (((struct cil_booleanif*)args->boolif->data)->preserved_tunable) {
cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in tunableif being treated as a booleanif", (char *)parse_current->data);
} else {
cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in booleanif", (char *)parse_current->data);
}
- goto exit;
+ return SEPOL_ERR;
}
}
- cil_tree_node_init(&ast_node);
+ return SEPOL_OK;
+}
- ast_node->parent = ast_current;
- ast_node->line = parse_current->line;
- ast_node->hll_offset = parse_current->hll_offset;
+static struct cil_tree_node * parse_statement(struct cil_db *db, struct cil_tree_node *parse_current, struct cil_tree_node *ast_parent)
+{
+ struct cil_tree_node *new_ast_node = NULL;
+ int rc = SEPOL_ERR;
+
+ cil_tree_node_init(&new_ast_node);
+ new_ast_node->parent = ast_parent;
+ new_ast_node->line = parse_current->line;
+ new_ast_node->hll_offset = parse_current->hll_offset;
if (parse_current->data == CIL_KEY_BLOCK) {
- rc = cil_gen_block(db, parse_current, ast_node, 0);
+ rc = cil_gen_block(db, parse_current, new_ast_node, 0);
} else if (parse_current->data == CIL_KEY_BLOCKINHERIT) {
- rc = cil_gen_blockinherit(db, parse_current, ast_node);
+ rc = cil_gen_blockinherit(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_BLOCKABSTRACT) {
- rc = cil_gen_blockabstract(db, parse_current, ast_node);
+ rc = cil_gen_blockabstract(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_IN) {
- rc = cil_gen_in(db, parse_current, ast_node);
+ rc = cil_gen_in(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CLASS) {
- rc = cil_gen_class(db, parse_current, ast_node);
- // To avoid parsing list of perms again
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_class(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CLASSORDER) {
- rc = cil_gen_classorder(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_classorder(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_MAP_CLASS) {
- rc = cil_gen_map_class(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_map_class(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CLASSMAPPING) {
- rc = cil_gen_classmapping(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_classmapping(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CLASSPERMISSION) {
- rc = cil_gen_classpermission(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_classpermission(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CLASSPERMISSIONSET) {
- rc = cil_gen_classpermissionset(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_classpermissionset(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_COMMON) {
- rc = cil_gen_common(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_common(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CLASSCOMMON) {
- rc = cil_gen_classcommon(db, parse_current, ast_node);
+ rc = cil_gen_classcommon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SID) {
- rc = cil_gen_sid(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_sid(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SIDCONTEXT) {
- rc = cil_gen_sidcontext(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_sidcontext(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SIDORDER) {
- rc = cil_gen_sidorder(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_sidorder(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_USER) {
- rc = cil_gen_user(db, parse_current, ast_node);
+ rc = cil_gen_user(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_USERATTRIBUTE) {
- rc = cil_gen_userattribute(db, parse_current, ast_node);
+ rc = cil_gen_userattribute(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_USERATTRIBUTESET) {
- rc = cil_gen_userattributeset(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_userattributeset(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_USERLEVEL) {
- rc = cil_gen_userlevel(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_userlevel(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_USERRANGE) {
- rc = cil_gen_userrange(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_userrange(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_USERBOUNDS) {
- rc = cil_gen_bounds(db, parse_current, ast_node, CIL_USER);
+ rc = cil_gen_bounds(db, parse_current, new_ast_node, CIL_USER);
} else if (parse_current->data == CIL_KEY_USERPREFIX) {
- rc = cil_gen_userprefix(db, parse_current, ast_node);
+ rc = cil_gen_userprefix(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SELINUXUSER) {
- rc = cil_gen_selinuxuser(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_selinuxuser(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SELINUXUSERDEFAULT) {
- rc = cil_gen_selinuxuserdefault(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_selinuxuserdefault(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_TYPE) {
- rc = cil_gen_type(db, parse_current, ast_node);
+ rc = cil_gen_type(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_TYPEATTRIBUTE) {
- rc = cil_gen_typeattribute(db, parse_current, ast_node);
+ rc = cil_gen_typeattribute(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_TYPEATTRIBUTESET) {
- rc = cil_gen_typeattributeset(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_typeattributeset(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_EXPANDTYPEATTRIBUTE) {
- rc = cil_gen_expandtypeattribute(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_expandtypeattribute(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_TYPEALIAS) {
- rc = cil_gen_alias(db, parse_current, ast_node, CIL_TYPEALIAS);
+ rc = cil_gen_alias(db, parse_current, new_ast_node, CIL_TYPEALIAS);
} else if (parse_current->data == CIL_KEY_TYPEALIASACTUAL) {
- rc = cil_gen_aliasactual(db, parse_current, ast_node, CIL_TYPEALIASACTUAL);
+ rc = cil_gen_aliasactual(db, parse_current, new_ast_node, CIL_TYPEALIASACTUAL);
} else if (parse_current->data == CIL_KEY_TYPEBOUNDS) {
- rc = cil_gen_bounds(db, parse_current, ast_node, CIL_TYPE);
+ rc = cil_gen_bounds(db, parse_current, new_ast_node, CIL_TYPE);
} else if (parse_current->data == CIL_KEY_TYPEPERMISSIVE) {
- rc = cil_gen_typepermissive(db, parse_current, ast_node);
+ rc = cil_gen_typepermissive(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_RANGETRANSITION) {
- rc = cil_gen_rangetransition(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_rangetransition(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_ROLE) {
- rc = cil_gen_role(db, parse_current, ast_node);
+ rc = cil_gen_role(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_USERROLE) {
- rc = cil_gen_userrole(db, parse_current, ast_node);
+ rc = cil_gen_userrole(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_ROLETYPE) {
- rc = cil_gen_roletype(db, parse_current, ast_node);
+ rc = cil_gen_roletype(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_ROLETRANSITION) {
- rc = cil_gen_roletransition(parse_current, ast_node);
+ rc = cil_gen_roletransition(parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_ROLEALLOW) {
- rc = cil_gen_roleallow(db, parse_current, ast_node);
+ rc = cil_gen_roleallow(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_ROLEATTRIBUTE) {
- rc = cil_gen_roleattribute(db, parse_current, ast_node);
+ rc = cil_gen_roleattribute(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_ROLEATTRIBUTESET) {
- rc = cil_gen_roleattributeset(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_roleattributeset(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_ROLEBOUNDS) {
- rc = cil_gen_bounds(db, parse_current, ast_node, CIL_ROLE);
+ rc = cil_gen_bounds(db, parse_current, new_ast_node, CIL_ROLE);
} else if (parse_current->data == CIL_KEY_BOOL) {
- rc = cil_gen_bool(db, parse_current, ast_node, CIL_FALSE);
+ rc = cil_gen_bool(db, parse_current, new_ast_node, CIL_FALSE);
} else if (parse_current->data == CIL_KEY_BOOLEANIF) {
- rc = cil_gen_boolif(db, parse_current, ast_node, CIL_FALSE);
+ rc = cil_gen_boolif(db, parse_current, new_ast_node, CIL_FALSE);
} else if(parse_current->data == CIL_KEY_TUNABLE) {
if (db->preserve_tunables) {
- rc = cil_gen_bool(db, parse_current, ast_node, CIL_TRUE);
+ rc = cil_gen_bool(db, parse_current, new_ast_node, CIL_TRUE);
} else {
- rc = cil_gen_tunable(db, parse_current, ast_node);
+ rc = cil_gen_tunable(db, parse_current, new_ast_node);
}
} else if (parse_current->data == CIL_KEY_TUNABLEIF) {
if (db->preserve_tunables) {
- rc = cil_gen_boolif(db, parse_current, ast_node, CIL_TRUE);
+ rc = cil_gen_boolif(db, parse_current, new_ast_node, CIL_TRUE);
} else {
- rc = cil_gen_tunif(db, parse_current, ast_node);
+ rc = cil_gen_tunif(db, parse_current, new_ast_node);
}
} else if (parse_current->data == CIL_KEY_CONDTRUE) {
- rc = cil_gen_condblock(db, parse_current, ast_node, CIL_CONDTRUE);
+ rc = cil_gen_condblock(db, parse_current, new_ast_node, CIL_CONDTRUE);
} else if (parse_current->data == CIL_KEY_CONDFALSE) {
- rc = cil_gen_condblock(db, parse_current, ast_node, CIL_CONDFALSE);
+ rc = cil_gen_condblock(db, parse_current, new_ast_node, CIL_CONDFALSE);
} else if (parse_current->data == CIL_KEY_ALLOW) {
- rc = cil_gen_avrule(parse_current, ast_node, CIL_AVRULE_ALLOWED);
- // So that the object and perms lists do not get parsed again
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_avrule(parse_current, new_ast_node, CIL_AVRULE_ALLOWED);
} else if (parse_current->data == CIL_KEY_AUDITALLOW) {
- rc = cil_gen_avrule(parse_current, ast_node, CIL_AVRULE_AUDITALLOW);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_avrule(parse_current, new_ast_node, CIL_AVRULE_AUDITALLOW);
} else if (parse_current->data == CIL_KEY_DONTAUDIT) {
- rc = cil_gen_avrule(parse_current, ast_node, CIL_AVRULE_DONTAUDIT);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_avrule(parse_current, new_ast_node, CIL_AVRULE_DONTAUDIT);
} else if (parse_current->data == CIL_KEY_NEVERALLOW) {
- rc = cil_gen_avrule(parse_current, ast_node, CIL_AVRULE_NEVERALLOW);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_avrule(parse_current, new_ast_node, CIL_AVRULE_NEVERALLOW);
} else if (parse_current->data == CIL_KEY_ALLOWX) {
- rc = cil_gen_avrulex(parse_current, ast_node, CIL_AVRULE_ALLOWED);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_avrulex(parse_current, new_ast_node, CIL_AVRULE_ALLOWED);
} else if (parse_current->data == CIL_KEY_AUDITALLOWX) {
- rc = cil_gen_avrulex(parse_current, ast_node, CIL_AVRULE_AUDITALLOW);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_avrulex(parse_current, new_ast_node, CIL_AVRULE_AUDITALLOW);
} else if (parse_current->data == CIL_KEY_DONTAUDITX) {
- rc = cil_gen_avrulex(parse_current, ast_node, CIL_AVRULE_DONTAUDIT);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_avrulex(parse_current, new_ast_node, CIL_AVRULE_DONTAUDIT);
} else if (parse_current->data == CIL_KEY_NEVERALLOWX) {
- rc = cil_gen_avrulex(parse_current, ast_node, CIL_AVRULE_NEVERALLOW);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_avrulex(parse_current, new_ast_node, CIL_AVRULE_NEVERALLOW);
} else if (parse_current->data == CIL_KEY_PERMISSIONX) {
- rc = cil_gen_permissionx(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_permissionx(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_TYPETRANSITION) {
- rc = cil_gen_typetransition(db, parse_current, ast_node);
+ rc = cil_gen_typetransition(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_TYPECHANGE) {
- rc = cil_gen_type_rule(parse_current, ast_node, CIL_TYPE_CHANGE);
+ rc = cil_gen_type_rule(parse_current, new_ast_node, CIL_TYPE_CHANGE);
} else if (parse_current->data == CIL_KEY_TYPEMEMBER) {
- rc = cil_gen_type_rule(parse_current, ast_node, CIL_TYPE_MEMBER);
+ rc = cil_gen_type_rule(parse_current, new_ast_node, CIL_TYPE_MEMBER);
} else if (parse_current->data == CIL_KEY_SENSITIVITY) {
- rc = cil_gen_sensitivity(db, parse_current, ast_node);
+ rc = cil_gen_sensitivity(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SENSALIAS) {
- rc = cil_gen_alias(db, parse_current, ast_node, CIL_SENSALIAS);
+ rc = cil_gen_alias(db, parse_current, new_ast_node, CIL_SENSALIAS);
} else if (parse_current->data == CIL_KEY_SENSALIASACTUAL) {
- rc = cil_gen_aliasactual(db, parse_current, ast_node, CIL_SENSALIASACTUAL);
+ rc = cil_gen_aliasactual(db, parse_current, new_ast_node, CIL_SENSALIASACTUAL);
} else if (parse_current->data == CIL_KEY_CATEGORY) {
- rc = cil_gen_category(db, parse_current, ast_node);
+ rc = cil_gen_category(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CATALIAS) {
- rc = cil_gen_alias(db, parse_current, ast_node, CIL_CATALIAS);
+ rc = cil_gen_alias(db, parse_current, new_ast_node, CIL_CATALIAS);
} else if (parse_current->data == CIL_KEY_CATALIASACTUAL) {
- rc = cil_gen_aliasactual(db, parse_current, ast_node, CIL_CATALIASACTUAL);
+ rc = cil_gen_aliasactual(db, parse_current, new_ast_node, CIL_CATALIASACTUAL);
} else if (parse_current->data == CIL_KEY_CATSET) {
- rc = cil_gen_catset(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_catset(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CATORDER) {
- rc = cil_gen_catorder(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_catorder(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SENSITIVITYORDER) {
- rc = cil_gen_sensitivityorder(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_sensitivityorder(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SENSCAT) {
- rc = cil_gen_senscat(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_senscat(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_LEVEL) {
- rc = cil_gen_level(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_level(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_LEVELRANGE) {
- rc = cil_gen_levelrange(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_levelrange(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CONSTRAIN) {
- rc = cil_gen_constrain(db, parse_current, ast_node, CIL_CONSTRAIN);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_constrain(db, parse_current, new_ast_node, CIL_CONSTRAIN);
} else if (parse_current->data == CIL_KEY_MLSCONSTRAIN) {
- rc = cil_gen_constrain(db, parse_current, ast_node, CIL_MLSCONSTRAIN);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_constrain(db, parse_current, new_ast_node, CIL_MLSCONSTRAIN);
} else if (parse_current->data == CIL_KEY_VALIDATETRANS) {
- rc = cil_gen_validatetrans(db, parse_current, ast_node, CIL_VALIDATETRANS);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_validatetrans(db, parse_current, new_ast_node, CIL_VALIDATETRANS);
} else if (parse_current->data == CIL_KEY_MLSVALIDATETRANS) {
- rc = cil_gen_validatetrans(db, parse_current, ast_node, CIL_MLSVALIDATETRANS);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_validatetrans(db, parse_current, new_ast_node, CIL_MLSVALIDATETRANS);
} else if (parse_current->data == CIL_KEY_CONTEXT) {
- rc = cil_gen_context(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_context(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_FILECON) {
- rc = cil_gen_filecon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_filecon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_IBPKEYCON) {
- rc = cil_gen_ibpkeycon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_ibpkeycon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_IBENDPORTCON) {
- rc = cil_gen_ibendportcon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_ibendportcon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_PORTCON) {
- rc = cil_gen_portcon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_portcon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_NODECON) {
- rc = cil_gen_nodecon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_nodecon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_GENFSCON) {
- rc = cil_gen_genfscon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_genfscon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_NETIFCON) {
- rc = cil_gen_netifcon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_netifcon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_PIRQCON) {
- rc = cil_gen_pirqcon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_pirqcon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_IOMEMCON) {
- rc = cil_gen_iomemcon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_iomemcon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_IOPORTCON) {
- rc = cil_gen_ioportcon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_ioportcon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_PCIDEVICECON) {
- rc = cil_gen_pcidevicecon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_pcidevicecon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_DEVICETREECON) {
- rc = cil_gen_devicetreecon(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_devicetreecon(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_FSUSE) {
- rc = cil_gen_fsuse(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_fsuse(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_MACRO) {
- rc = cil_gen_macro(db, parse_current, ast_node);
+ rc = cil_gen_macro(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_CALL) {
- rc = cil_gen_call(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_call(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_POLICYCAP) {
- rc = cil_gen_policycap(db, parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_policycap(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_OPTIONAL) {
- rc = cil_gen_optional(db, parse_current, ast_node);
+ rc = cil_gen_optional(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_IPADDR) {
- rc = cil_gen_ipaddr(db, parse_current, ast_node);
+ rc = cil_gen_ipaddr(db, parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_DEFAULTUSER) {
- rc = cil_gen_default(parse_current, ast_node, CIL_DEFAULTUSER);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_default(parse_current, new_ast_node, CIL_DEFAULTUSER);
} else if (parse_current->data == CIL_KEY_DEFAULTROLE) {
- rc = cil_gen_default(parse_current, ast_node, CIL_DEFAULTROLE);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_default(parse_current, new_ast_node, CIL_DEFAULTROLE);
} else if (parse_current->data == CIL_KEY_DEFAULTTYPE) {
- rc = cil_gen_default(parse_current, ast_node, CIL_DEFAULTTYPE);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_default(parse_current, new_ast_node, CIL_DEFAULTTYPE);
} else if (parse_current->data == CIL_KEY_DEFAULTRANGE) {
- rc = cil_gen_defaultrange(parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_defaultrange(parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_HANDLEUNKNOWN) {
- rc = cil_gen_handleunknown(parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_handleunknown(parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_MLS) {
- rc = cil_gen_mls(parse_current, ast_node);
- *finished = CIL_TREE_SKIP_NEXT;
+ rc = cil_gen_mls(parse_current, new_ast_node);
} else if (parse_current->data == CIL_KEY_SRC_INFO) {
- rc = cil_gen_src_info(parse_current, ast_node);
+ rc = cil_gen_src_info(parse_current, new_ast_node);
} else {
cil_log(CIL_ERR, "Error: Unknown keyword %s\n", (char *)parse_current->data);
rc = SEPOL_ERR;
}
if (rc == SEPOL_OK) {
- if (ast_current->cl_head == NULL) {
- ast_current->cl_head = ast_node;
+ if (ast_parent->cl_head == NULL) {
+ ast_parent->cl_head = new_ast_node;
} else {
- ast_current->cl_tail->next = ast_node;
+ ast_parent->cl_tail->next = new_ast_node;
}
- ast_current->cl_tail = ast_node;
- ast_current = ast_node;
- args->ast = ast_current;
+ ast_parent->cl_tail = new_ast_node;
} else {
- cil_tree_node_destroy(&ast_node);
+ cil_tree_node_destroy(&new_ast_node);
+ new_ast_node = NULL;
}
-exit:
- return rc;
+ return new_ast_node;
}
-int __cil_build_ast_first_child_helper(__attribute__((unused)) struct cil_tree_node *parse_current, void *extra_args)
+int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *finished, void *extra_args)
{
struct cil_args_build *args = extra_args;
- struct cil_tree_node *ast = args->ast;
+ struct cil_tree_node *new_ast_node = NULL;
+ int rc = SEPOL_ERR;
- if (ast->flavor == CIL_TUNABLEIF) {
- args->tunif = ast;
+ if (parse_current->parent->cl_head != parse_current) {
+ /* ignore anything that isn't following a parenthesis */
+ return SEPOL_OK;
+ } else if (parse_current->data == NULL) {
+ /* the only time parenthesis can immediately following parenthesis is if
+ * the parent is the root node */
+ if (parse_current->parent->parent == NULL) {
+ return SEPOL_OK;
+ } else {
+ cil_tree_log(parse_current, CIL_ERR, "Keyword expected after open parenthesis");
+ return SEPOL_ERR;
+ }
}
- if (ast->flavor == CIL_IN) {
- args->in = ast;
+ rc = check_for_illegal_statement(parse_current, args);
+ if (rc != SEPOL_OK) {
+ return SEPOL_ERR;
}
- if (ast->flavor == CIL_MACRO) {
- args->macro = ast;
+ new_ast_node = parse_statement(args->db, parse_current, args->ast);
+ if (!new_ast_node) {
+ return SEPOL_ERR;
}
- if (ast->flavor == CIL_OPTIONAL) {
- args->optional = ast;
+ args->ast = new_ast_node;
+
+ if (parse_current->data != CIL_KEY_BLOCK &&
+ parse_current->data != CIL_KEY_IN &&
+ parse_current->data != CIL_KEY_TUNABLEIF &&
+ parse_current->data != CIL_KEY_BOOLEANIF &&
+ parse_current->data != CIL_KEY_CONDTRUE &&
+ parse_current->data != CIL_KEY_CONDFALSE &&
+ parse_current->data != CIL_KEY_MACRO &&
+ parse_current->data != CIL_KEY_OPTIONAL &&
+ parse_current->data != CIL_KEY_SRC_INFO) {
+ /* Skip anything that does not contain a list of policy statements */
+ *finished = CIL_TREE_SKIP_NEXT;
}
- if (ast->flavor == CIL_BOOLEANIF) {
+ return SEPOL_OK;
+}
+
+int __cil_build_ast_first_child_helper(__attribute__((unused)) struct cil_tree_node *parse_current, void *extra_args)
+{
+ struct cil_args_build *args = extra_args;
+ struct cil_tree_node *ast = args->ast;
+
+ if (ast->flavor == CIL_TUNABLEIF) {
+ args->tunif = ast;
+ } else if (ast->flavor == CIL_IN) {
+ args->in = ast;
+ } else if (ast->flavor == CIL_MACRO) {
+ args->macro = ast;
+ } else if (ast->flavor == CIL_OPTIONAL) {
+ args->optional = ast;
+ } else if (ast->flavor == CIL_BOOLEANIF) {
args->boolif = ast;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] libsepol/cil: Simplify cil_tree_children_destroy()
2021-08-30 18:24 [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed James Carter
2021-08-30 18:24 ` [PATCH 2/3] libsepol/cil: Refactor the function __cil_build_ast_node_helper() James Carter
@ 2021-08-30 18:24 ` James Carter
2021-09-01 19:25 ` [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed Nicolas Iooss
2 siblings, 0 replies; 7+ messages in thread
From: James Carter @ 2021-08-30 18:24 UTC (permalink / raw)
To: selinux; +Cc: James Carter
Use a simpler recursive solution and set the head and tail pointers
of the starting node to NULL when done.
Remove the now uneeded setting of the head and tail pointers to NULL
in cil_resolve_in().
Signed-off-by: James Carter <jwcart2@gmail.com>
---
libsepol/cil/src/cil_resolve_ast.c | 2 --
libsepol/cil/src/cil_tree.c | 33 +++++++++---------------------
2 files changed, 10 insertions(+), 25 deletions(-)
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index aeedc7dd..60dbd163 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -2440,8 +2440,6 @@ int cil_resolve_in(struct cil_tree_node *current, void *extra_args)
}
cil_tree_children_destroy(current);
- current->cl_head = NULL;
- current->cl_tail = NULL;
return SEPOL_OK;
diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c
index 75293005..075b1eb5 100644
--- a/libsepol/cil/src/cil_tree.c
+++ b/libsepol/cil/src/cil_tree.c
@@ -201,34 +201,21 @@ void cil_tree_subtree_destroy(struct cil_tree_node *node)
void cil_tree_children_destroy(struct cil_tree_node *node)
{
- struct cil_tree_node *start_node = node;
- struct cil_tree_node *next = NULL;
+ struct cil_tree_node *curr, *next;
- if (node == NULL) {
+ if (!node) {
return;
}
- if (node->cl_head != NULL) {
- node = node->cl_head;
- }
-
- while (node != start_node) {
- if (node->cl_head != NULL){
- next = node->cl_head;
- } else {
- if (node->next == NULL) {
- next = node->parent;
- if (node->parent != NULL) {
- node->parent->cl_head = NULL;
- }
- cil_tree_node_destroy(&node);
- } else {
- next = node->next;
- cil_tree_node_destroy(&node);
- }
- }
- node = next;
+ curr = node->cl_head;
+ while (curr) {
+ next = curr->next;
+ cil_tree_children_destroy(curr);
+ cil_tree_node_destroy(&curr);
+ curr = next;
}
+ node->cl_head = NULL;
+ node->cl_tail = NULL;
}
void cil_tree_node_init(struct cil_tree_node **node)
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed
2021-08-30 18:24 [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed James Carter
2021-08-30 18:24 ` [PATCH 2/3] libsepol/cil: Refactor the function __cil_build_ast_node_helper() James Carter
2021-08-30 18:24 ` [PATCH 3/3] libsepol/cil: Simplify cil_tree_children_destroy() James Carter
@ 2021-09-01 19:25 ` Nicolas Iooss
2021-09-02 16:33 ` James Carter
2 siblings, 1 reply; 7+ messages in thread
From: Nicolas Iooss @ 2021-09-01 19:25 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list
On Mon, Aug 30, 2021 at 8:24 PM James Carter <jwcart2@gmail.com> wrote:
>
> If an optional that is to be disabled is the child of an optional that
> is going to be disabled, then there is no reason to add that optional
> to the stack of disabled optionals, because it is going to be destroyed
> anyways. This means that there is no reason to maintain a stack of
> disabled optionals at all.
>
> Instead of using a stack to track disabled optionals, use a pointer
> that points to the top-most optional that is to be disabled. When a
> rule fails to resolve in an optional, if the disabled optional pointer
> has not been set, then set it to that optional. If the pointer has
> been set already, then the optional is already going to be destroyed,
> so nothing else needs to be done. The resolution failure and the fact
> that the optional is being disabled is reported in either case.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
For the 3 patches:
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
(I liked these simplifications, and if someone encounters a stack
exhaustion issue because of changing cil_tree_children_destroy to a
recursive function, I guess a reasonable recursion limit could be
added where it would make sense)
Thanks,
Nicolas
> ---
> libsepol/cil/src/cil_resolve_ast.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 18007324..aeedc7dd 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -56,6 +56,7 @@ struct cil_args_resolve {
> struct cil_tree_node *block;
> struct cil_tree_node *macro;
> struct cil_tree_node *optional;
> + struct cil_tree_node *disabled_optional;
> struct cil_tree_node *boolif;
> struct cil_list *sidorder_lists;
> struct cil_list *classorder_lists;
> @@ -63,7 +64,6 @@ struct cil_args_resolve {
> struct cil_list *catorder_lists;
> struct cil_list *sensitivityorder_lists;
> struct cil_list *in_list;
> - struct cil_stack *disabled_optionals;
> };
>
> static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key, struct cil_tree_node *ast_node)
> @@ -3873,7 +3873,6 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> struct cil_tree_node *macro = args->macro;
> struct cil_tree_node *optional = args->optional;
> struct cil_tree_node *boolif = args->boolif;
> - struct cil_stack *disabled_optionals = args->disabled_optionals;
>
> if (node == NULL) {
> goto exit;
> @@ -3956,7 +3955,9 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> if (optional == NULL) {
> cil_tree_log(node, CIL_ERR, "Failed to resolve %s statement", cil_node_to_string(node));
> } else {
> - cil_stack_push(disabled_optionals, CIL_NODE, optional);
> + if (!args->disabled_optional) {
> + args->disabled_optional = optional;
> + }
> cil_tree_log(node, CIL_INFO, "Failed to resolve %s statement", cil_node_to_string(node));
> cil_tree_log(optional, CIL_INFO, "Disabling optional '%s'", DATUM(optional->data)->name);
> rc = SEPOL_OK;
> @@ -4003,7 +4004,6 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> {
> int rc = SEPOL_ERR;
> struct cil_args_resolve *args = extra_args;
> - struct cil_stack *disabled_optionals = args->disabled_optionals;
> struct cil_tree_node *parent = NULL;
>
> if (current == NULL || extra_args == NULL) {
> @@ -4026,11 +4026,10 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> args->macro = NULL;
> } else if (parent->flavor == CIL_OPTIONAL) {
> struct cil_tree_node *n = parent->parent;
> - struct cil_stack_item *item = cil_stack_peek(disabled_optionals);
> - if (item && item->data == parent) {
> - cil_stack_pop(disabled_optionals);
> + if (args->disabled_optional == parent) {
> *(args->changed) = CIL_TRUE;
> cil_list_append(args->to_destroy, CIL_NODE, parent);
> + args->disabled_optional = NULL;
> }
> args->optional = NULL;
> while (n && n->flavor != CIL_ROOT) {
> @@ -4067,6 +4066,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> extra_args.block = NULL;
> extra_args.macro = NULL;
> extra_args.optional = NULL;
> + extra_args.disabled_optional = NULL;
> extra_args.boolif= NULL;
> extra_args.sidorder_lists = NULL;
> extra_args.classorder_lists = NULL;
> @@ -4074,7 +4074,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> extra_args.catorder_lists = NULL;
> extra_args.sensitivityorder_lists = NULL;
> extra_args.in_list = NULL;
> - extra_args.disabled_optionals = NULL;
>
> cil_list_init(&extra_args.to_destroy, CIL_NODE);
> cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
> @@ -4083,7 +4082,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
> cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> cil_list_init(&extra_args.in_list, CIL_IN);
> - cil_stack_init(&extra_args.disabled_optionals);
>
> for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> extra_args.pass = pass;
> @@ -4218,7 +4216,6 @@ exit:
> __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
> cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> - cil_stack_destroy(&extra_args.disabled_optionals);
>
> return rc;
> }
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed
2021-09-01 19:25 ` [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed Nicolas Iooss
@ 2021-09-02 16:33 ` James Carter
2021-09-06 18:32 ` Nicolas Iooss
0 siblings, 1 reply; 7+ messages in thread
From: James Carter @ 2021-09-02 16:33 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
On Wed, Sep 1, 2021 at 3:26 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Mon, Aug 30, 2021 at 8:24 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > If an optional that is to be disabled is the child of an optional that
> > is going to be disabled, then there is no reason to add that optional
> > to the stack of disabled optionals, because it is going to be destroyed
> > anyways. This means that there is no reason to maintain a stack of
> > disabled optionals at all.
> >
> > Instead of using a stack to track disabled optionals, use a pointer
> > that points to the top-most optional that is to be disabled. When a
> > rule fails to resolve in an optional, if the disabled optional pointer
> > has not been set, then set it to that optional. If the pointer has
> > been set already, then the optional is already going to be destroyed,
> > so nothing else needs to be done. The resolution failure and the fact
> > that the optional is being disabled is reported in either case.
> >
> > Signed-off-by: James Carter <jwcart2@gmail.com>
>
> For the 3 patches:
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
This series has been merged.
> (I liked these simplifications, and if someone encounters a stack
> exhaustion issue because of changing cil_tree_children_destroy to a
> recursive function, I guess a reasonable recursion limit could be
> added where it would make sense)
>
Yes, my thought was that since cil_tree_walk() & cil_tree_walk_core()
have similar
limits, that it wouldn't be a problem. And, if it was, we could add a
recursion limit as you suggest.
Thanks,
Jim
> Thanks,
> Nicolas
>
> > ---
> > libsepol/cil/src/cil_resolve_ast.c | 17 +++++++----------
> > 1 file changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > index 18007324..aeedc7dd 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -56,6 +56,7 @@ struct cil_args_resolve {
> > struct cil_tree_node *block;
> > struct cil_tree_node *macro;
> > struct cil_tree_node *optional;
> > + struct cil_tree_node *disabled_optional;
> > struct cil_tree_node *boolif;
> > struct cil_list *sidorder_lists;
> > struct cil_list *classorder_lists;
> > @@ -63,7 +64,6 @@ struct cil_args_resolve {
> > struct cil_list *catorder_lists;
> > struct cil_list *sensitivityorder_lists;
> > struct cil_list *in_list;
> > - struct cil_stack *disabled_optionals;
> > };
> >
> > static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key, struct cil_tree_node *ast_node)
> > @@ -3873,7 +3873,6 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> > struct cil_tree_node *macro = args->macro;
> > struct cil_tree_node *optional = args->optional;
> > struct cil_tree_node *boolif = args->boolif;
> > - struct cil_stack *disabled_optionals = args->disabled_optionals;
> >
> > if (node == NULL) {
> > goto exit;
> > @@ -3956,7 +3955,9 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> > if (optional == NULL) {
> > cil_tree_log(node, CIL_ERR, "Failed to resolve %s statement", cil_node_to_string(node));
> > } else {
> > - cil_stack_push(disabled_optionals, CIL_NODE, optional);
> > + if (!args->disabled_optional) {
> > + args->disabled_optional = optional;
> > + }
> > cil_tree_log(node, CIL_INFO, "Failed to resolve %s statement", cil_node_to_string(node));
> > cil_tree_log(optional, CIL_INFO, "Disabling optional '%s'", DATUM(optional->data)->name);
> > rc = SEPOL_OK;
> > @@ -4003,7 +4004,6 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> > {
> > int rc = SEPOL_ERR;
> > struct cil_args_resolve *args = extra_args;
> > - struct cil_stack *disabled_optionals = args->disabled_optionals;
> > struct cil_tree_node *parent = NULL;
> >
> > if (current == NULL || extra_args == NULL) {
> > @@ -4026,11 +4026,10 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> > args->macro = NULL;
> > } else if (parent->flavor == CIL_OPTIONAL) {
> > struct cil_tree_node *n = parent->parent;
> > - struct cil_stack_item *item = cil_stack_peek(disabled_optionals);
> > - if (item && item->data == parent) {
> > - cil_stack_pop(disabled_optionals);
> > + if (args->disabled_optional == parent) {
> > *(args->changed) = CIL_TRUE;
> > cil_list_append(args->to_destroy, CIL_NODE, parent);
> > + args->disabled_optional = NULL;
> > }
> > args->optional = NULL;
> > while (n && n->flavor != CIL_ROOT) {
> > @@ -4067,6 +4066,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > extra_args.block = NULL;
> > extra_args.macro = NULL;
> > extra_args.optional = NULL;
> > + extra_args.disabled_optional = NULL;
> > extra_args.boolif= NULL;
> > extra_args.sidorder_lists = NULL;
> > extra_args.classorder_lists = NULL;
> > @@ -4074,7 +4074,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > extra_args.catorder_lists = NULL;
> > extra_args.sensitivityorder_lists = NULL;
> > extra_args.in_list = NULL;
> > - extra_args.disabled_optionals = NULL;
> >
> > cil_list_init(&extra_args.to_destroy, CIL_NODE);
> > cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
> > @@ -4083,7 +4082,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
> > cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> > cil_list_init(&extra_args.in_list, CIL_IN);
> > - cil_stack_init(&extra_args.disabled_optionals);
> >
> > for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> > extra_args.pass = pass;
> > @@ -4218,7 +4216,6 @@ exit:
> > __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
> > cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> > cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> > - cil_stack_destroy(&extra_args.disabled_optionals);
> >
> > return rc;
> > }
> > --
> > 2.31.1
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed
2021-09-02 16:33 ` James Carter
@ 2021-09-06 18:32 ` Nicolas Iooss
2021-09-07 13:40 ` James Carter
0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Iooss @ 2021-09-06 18:32 UTC (permalink / raw)
To: James Carter, SElinux list
On Thu, Sep 2, 2021 at 6:33 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Wed, Sep 1, 2021 at 3:26 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > On Mon, Aug 30, 2021 at 8:24 PM James Carter <jwcart2@gmail.com> wrote:
> > >
> > > If an optional that is to be disabled is the child of an optional that
> > > is going to be disabled, then there is no reason to add that optional
> > > to the stack of disabled optionals, because it is going to be destroyed
> > > anyways. This means that there is no reason to maintain a stack of
> > > disabled optionals at all.
> > >
> > > Instead of using a stack to track disabled optionals, use a pointer
> > > that points to the top-most optional that is to be disabled. When a
> > > rule fails to resolve in an optional, if the disabled optional pointer
> > > has not been set, then set it to that optional. If the pointer has
> > > been set already, then the optional is already going to be destroyed,
> > > so nothing else needs to be done. The resolution failure and the fact
> > > that the optional is being disabled is reported in either case.
> > >
> > > Signed-off-by: James Carter <jwcart2@gmail.com>
> >
> > For the 3 patches:
> >
> > Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> >
>
> This series has been merged.
>
> > (I liked these simplifications, and if someone encounters a stack
> > exhaustion issue because of changing cil_tree_children_destroy to a
> > recursive function, I guess a reasonable recursion limit could be
> > added where it would make sense)
> >
> Yes, my thought was that since cil_tree_walk() & cil_tree_walk_core()
> have similar
> limits, that it wouldn't be a problem. And, if it was, we could add a
> recursion limit as you suggest.
>
> Thanks,
> Jim
Hi,
I did not take long for OSS-Fuzz to find a crash due to the
cil_tree_children_destroy change:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=38110 (this link
will be publicly open on 2021-12-03 at the latest).
Here is a simple reproducer, in shell:
(echo '(' ; for i in $(seq 300000) ; do echo ';;*lmx 3 r' ; done) >
test.cil && secilc test.cil
On my system (x86_64 Arch Linux), this triggers a segmentation fault,
probably due to stack exhaustion, with maaaaany recursive calls to
cil_tree_children_destroy. Using the fuzzer, the limit is smaller
(with "seq 50000" I manage to trigger a segfault). So I guess the
number of successive line markers should be limited too.
Thanks,
Nicolas
> > Thanks,
> > Nicolas
> >
> > > ---
> > > libsepol/cil/src/cil_resolve_ast.c | 17 +++++++----------
> > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > > index 18007324..aeedc7dd 100644
> > > --- a/libsepol/cil/src/cil_resolve_ast.c
> > > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > > @@ -56,6 +56,7 @@ struct cil_args_resolve {
> > > struct cil_tree_node *block;
> > > struct cil_tree_node *macro;
> > > struct cil_tree_node *optional;
> > > + struct cil_tree_node *disabled_optional;
> > > struct cil_tree_node *boolif;
> > > struct cil_list *sidorder_lists;
> > > struct cil_list *classorder_lists;
> > > @@ -63,7 +64,6 @@ struct cil_args_resolve {
> > > struct cil_list *catorder_lists;
> > > struct cil_list *sensitivityorder_lists;
> > > struct cil_list *in_list;
> > > - struct cil_stack *disabled_optionals;
> > > };
> > >
> > > static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key, struct cil_tree_node *ast_node)
> > > @@ -3873,7 +3873,6 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> > > struct cil_tree_node *macro = args->macro;
> > > struct cil_tree_node *optional = args->optional;
> > > struct cil_tree_node *boolif = args->boolif;
> > > - struct cil_stack *disabled_optionals = args->disabled_optionals;
> > >
> > > if (node == NULL) {
> > > goto exit;
> > > @@ -3956,7 +3955,9 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> > > if (optional == NULL) {
> > > cil_tree_log(node, CIL_ERR, "Failed to resolve %s statement", cil_node_to_string(node));
> > > } else {
> > > - cil_stack_push(disabled_optionals, CIL_NODE, optional);
> > > + if (!args->disabled_optional) {
> > > + args->disabled_optional = optional;
> > > + }
> > > cil_tree_log(node, CIL_INFO, "Failed to resolve %s statement", cil_node_to_string(node));
> > > cil_tree_log(optional, CIL_INFO, "Disabling optional '%s'", DATUM(optional->data)->name);
> > > rc = SEPOL_OK;
> > > @@ -4003,7 +4004,6 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> > > {
> > > int rc = SEPOL_ERR;
> > > struct cil_args_resolve *args = extra_args;
> > > - struct cil_stack *disabled_optionals = args->disabled_optionals;
> > > struct cil_tree_node *parent = NULL;
> > >
> > > if (current == NULL || extra_args == NULL) {
> > > @@ -4026,11 +4026,10 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> > > args->macro = NULL;
> > > } else if (parent->flavor == CIL_OPTIONAL) {
> > > struct cil_tree_node *n = parent->parent;
> > > - struct cil_stack_item *item = cil_stack_peek(disabled_optionals);
> > > - if (item && item->data == parent) {
> > > - cil_stack_pop(disabled_optionals);
> > > + if (args->disabled_optional == parent) {
> > > *(args->changed) = CIL_TRUE;
> > > cil_list_append(args->to_destroy, CIL_NODE, parent);
> > > + args->disabled_optional = NULL;
> > > }
> > > args->optional = NULL;
> > > while (n && n->flavor != CIL_ROOT) {
> > > @@ -4067,6 +4066,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > > extra_args.block = NULL;
> > > extra_args.macro = NULL;
> > > extra_args.optional = NULL;
> > > + extra_args.disabled_optional = NULL;
> > > extra_args.boolif= NULL;
> > > extra_args.sidorder_lists = NULL;
> > > extra_args.classorder_lists = NULL;
> > > @@ -4074,7 +4074,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > > extra_args.catorder_lists = NULL;
> > > extra_args.sensitivityorder_lists = NULL;
> > > extra_args.in_list = NULL;
> > > - extra_args.disabled_optionals = NULL;
> > >
> > > cil_list_init(&extra_args.to_destroy, CIL_NODE);
> > > cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
> > > @@ -4083,7 +4082,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > > cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
> > > cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> > > cil_list_init(&extra_args.in_list, CIL_IN);
> > > - cil_stack_init(&extra_args.disabled_optionals);
> > >
> > > for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> > > extra_args.pass = pass;
> > > @@ -4218,7 +4216,6 @@ exit:
> > > __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
> > > cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> > > cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> > > - cil_stack_destroy(&extra_args.disabled_optionals);
> > >
> > > return rc;
> > > }
> > > --
> > > 2.31.1
> > >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed
2021-09-06 18:32 ` Nicolas Iooss
@ 2021-09-07 13:40 ` James Carter
0 siblings, 0 replies; 7+ messages in thread
From: James Carter @ 2021-09-07 13:40 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
On Mon, Sep 6, 2021 at 2:33 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Thu, Sep 2, 2021 at 6:33 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > On Wed, Sep 1, 2021 at 3:26 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > >
> > > On Mon, Aug 30, 2021 at 8:24 PM James Carter <jwcart2@gmail.com> wrote:
> > > >
> > > > If an optional that is to be disabled is the child of an optional that
> > > > is going to be disabled, then there is no reason to add that optional
> > > > to the stack of disabled optionals, because it is going to be destroyed
> > > > anyways. This means that there is no reason to maintain a stack of
> > > > disabled optionals at all.
> > > >
> > > > Instead of using a stack to track disabled optionals, use a pointer
> > > > that points to the top-most optional that is to be disabled. When a
> > > > rule fails to resolve in an optional, if the disabled optional pointer
> > > > has not been set, then set it to that optional. If the pointer has
> > > > been set already, then the optional is already going to be destroyed,
> > > > so nothing else needs to be done. The resolution failure and the fact
> > > > that the optional is being disabled is reported in either case.
> > > >
> > > > Signed-off-by: James Carter <jwcart2@gmail.com>
> > >
> > > For the 3 patches:
> > >
> > > Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > >
> >
> > This series has been merged.
> >
> > > (I liked these simplifications, and if someone encounters a stack
> > > exhaustion issue because of changing cil_tree_children_destroy to a
> > > recursive function, I guess a reasonable recursion limit could be
> > > added where it would make sense)
> > >
> > Yes, my thought was that since cil_tree_walk() & cil_tree_walk_core()
> > have similar
> > limits, that it wouldn't be a problem. And, if it was, we could add a
> > recursion limit as you suggest.
> >
> > Thanks,
> > Jim
>
> Hi,
> I did not take long for OSS-Fuzz to find a crash due to the
> cil_tree_children_destroy change:
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=38110 (this link
> will be publicly open on 2021-12-03 at the latest).
>
> Here is a simple reproducer, in shell:
>
> (echo '(' ; for i in $(seq 300000) ; do echo ';;*lmx 3 r' ; done) >
> test.cil && secilc test.cil
>
> On my system (x86_64 Arch Linux), this triggers a segmentation fault,
> probably due to stack exhaustion, with maaaaany recursive calls to
> cil_tree_children_destroy. Using the fuzzer, the limit is smaller
> (with "seq 50000" I manage to trigger a segfault). So I guess the
> number of successive line markers should be limited too.
>
I saw the OSS-Fuzz message, but it didn't cause a crash on my Fedora system.
There is already a limit of 4,096 open parenthesis, but the line mark
stuff does not get included in that count. I think that it would make
sense for it to get included.
Jim
> Thanks,
> Nicolas
>
> > > Thanks,
> > > Nicolas
> > >
> > > > ---
> > > > libsepol/cil/src/cil_resolve_ast.c | 17 +++++++----------
> > > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > > > index 18007324..aeedc7dd 100644
> > > > --- a/libsepol/cil/src/cil_resolve_ast.c
> > > > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > > > @@ -56,6 +56,7 @@ struct cil_args_resolve {
> > > > struct cil_tree_node *block;
> > > > struct cil_tree_node *macro;
> > > > struct cil_tree_node *optional;
> > > > + struct cil_tree_node *disabled_optional;
> > > > struct cil_tree_node *boolif;
> > > > struct cil_list *sidorder_lists;
> > > > struct cil_list *classorder_lists;
> > > > @@ -63,7 +64,6 @@ struct cil_args_resolve {
> > > > struct cil_list *catorder_lists;
> > > > struct cil_list *sensitivityorder_lists;
> > > > struct cil_list *in_list;
> > > > - struct cil_stack *disabled_optionals;
> > > > };
> > > >
> > > > static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key, struct cil_tree_node *ast_node)
> > > > @@ -3873,7 +3873,6 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> > > > struct cil_tree_node *macro = args->macro;
> > > > struct cil_tree_node *optional = args->optional;
> > > > struct cil_tree_node *boolif = args->boolif;
> > > > - struct cil_stack *disabled_optionals = args->disabled_optionals;
> > > >
> > > > if (node == NULL) {
> > > > goto exit;
> > > > @@ -3956,7 +3955,9 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> > > > if (optional == NULL) {
> > > > cil_tree_log(node, CIL_ERR, "Failed to resolve %s statement", cil_node_to_string(node));
> > > > } else {
> > > > - cil_stack_push(disabled_optionals, CIL_NODE, optional);
> > > > + if (!args->disabled_optional) {
> > > > + args->disabled_optional = optional;
> > > > + }
> > > > cil_tree_log(node, CIL_INFO, "Failed to resolve %s statement", cil_node_to_string(node));
> > > > cil_tree_log(optional, CIL_INFO, "Disabling optional '%s'", DATUM(optional->data)->name);
> > > > rc = SEPOL_OK;
> > > > @@ -4003,7 +4004,6 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> > > > {
> > > > int rc = SEPOL_ERR;
> > > > struct cil_args_resolve *args = extra_args;
> > > > - struct cil_stack *disabled_optionals = args->disabled_optionals;
> > > > struct cil_tree_node *parent = NULL;
> > > >
> > > > if (current == NULL || extra_args == NULL) {
> > > > @@ -4026,11 +4026,10 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> > > > args->macro = NULL;
> > > > } else if (parent->flavor == CIL_OPTIONAL) {
> > > > struct cil_tree_node *n = parent->parent;
> > > > - struct cil_stack_item *item = cil_stack_peek(disabled_optionals);
> > > > - if (item && item->data == parent) {
> > > > - cil_stack_pop(disabled_optionals);
> > > > + if (args->disabled_optional == parent) {
> > > > *(args->changed) = CIL_TRUE;
> > > > cil_list_append(args->to_destroy, CIL_NODE, parent);
> > > > + args->disabled_optional = NULL;
> > > > }
> > > > args->optional = NULL;
> > > > while (n && n->flavor != CIL_ROOT) {
> > > > @@ -4067,6 +4066,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > > > extra_args.block = NULL;
> > > > extra_args.macro = NULL;
> > > > extra_args.optional = NULL;
> > > > + extra_args.disabled_optional = NULL;
> > > > extra_args.boolif= NULL;
> > > > extra_args.sidorder_lists = NULL;
> > > > extra_args.classorder_lists = NULL;
> > > > @@ -4074,7 +4074,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > > > extra_args.catorder_lists = NULL;
> > > > extra_args.sensitivityorder_lists = NULL;
> > > > extra_args.in_list = NULL;
> > > > - extra_args.disabled_optionals = NULL;
> > > >
> > > > cil_list_init(&extra_args.to_destroy, CIL_NODE);
> > > > cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
> > > > @@ -4083,7 +4082,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > > > cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
> > > > cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> > > > cil_list_init(&extra_args.in_list, CIL_IN);
> > > > - cil_stack_init(&extra_args.disabled_optionals);
> > > >
> > > > for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> > > > extra_args.pass = pass;
> > > > @@ -4218,7 +4216,6 @@ exit:
> > > > __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
> > > > cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> > > > cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> > > > - cil_stack_destroy(&extra_args.disabled_optionals);
> > > >
> > > > return rc;
> > > > }
> > > > --
> > > > 2.31.1
> > > >
> > >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-07 13:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-30 18:24 [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed James Carter
2021-08-30 18:24 ` [PATCH 2/3] libsepol/cil: Refactor the function __cil_build_ast_node_helper() James Carter
2021-08-30 18:24 ` [PATCH 3/3] libsepol/cil: Simplify cil_tree_children_destroy() James Carter
2021-09-01 19:25 ` [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed Nicolas Iooss
2021-09-02 16:33 ` James Carter
2021-09-06 18:32 ` Nicolas Iooss
2021-09-07 13:40 ` James Carter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.