On Mon, Sep 6, 2021 at 2:33 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > On Thu, Sep 2, 2021 at 6:33 PM James Carter <jwcart2@xxxxxxxxx> wrote: > > > > On Wed, Sep 1, 2021 at 3:26 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > > > > > On Mon, Aug 30, 2021 at 8:24 PM James Carter <jwcart2@xxxxxxxxx> 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@xxxxxxxxx> > > > > > > For the 3 patches: > > > > > > Acked-by: Nicolas Iooss <nicolas.iooss@xxxxxxx> > > > > > > > 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 > > > > > > > >
