| Message ID | 20210830182442.491886-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed | expand |
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 >
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 > > >
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 > > > > >
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 > > > > > > > >
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; }
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(-)