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
> 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
> >
>
