Re: [PATCH v2 2/2] libsepol/cil: Return error when identifier declared as both type and attribute

[James Carter <jwcart2@gmail.com>]

On Tue, May 26, 2020 at 11:47 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, May 19, 2020 at 10:30 AM James Carter <jwcart2@gmail.com> wrote:
> >
> > CIL allows a type to be redeclared when using the multiple declarations
> > option ("-m" or "--muliple-decls"), but make it an error for an identifier
> > to be declared as both a type and an attribute.
> >
> > Change the error message so that it always gives the location and flavor
> > of both declarations. The flavors will be the same in all other cases,
> > but in this case they explain why there is an error even if multiple
> > declartions are allowed.
> >
> > Issue reported by: Topi Miettinen <toiwoton@gmail.com>
>
> Normally just Reported-by:...
>
> > Fixes: Commit fafe4c212bf6c32c ("libsepol: cil: Add ability to redeclare
> >        types[attributes]")
>
> Normally just "Fixes: <hash> ("one-liner")", no "Commit".
>
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >  libsepol/cil/src/cil_build_ast.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > index fcecdc4f..ce2499a1 100644
> > --- a/libsepol/cil/src/cil_build_ast.c
> > +++ b/libsepol/cil/src/cil_build_ast.c
> > @@ -87,7 +87,7 @@ exit:
> >   * datum, given the new datum and the one already present in a given symtab.
> >   */
> >  int cil_is_datum_multiple_decl(__attribute__((unused)) struct cil_symtab_datum *cur,
> > -                               __attribute__((unused)) struct cil_symtab_datum *old,
> > +                               struct cil_symtab_datum *old,
> >                                 enum cil_flavor f)
> >  {
> >         int rc = CIL_FALSE;
> > @@ -95,8 +95,12 @@ int cil_is_datum_multiple_decl(__attribute__((unused)) struct cil_symtab_datum *
> >         switch (f) {
> >         case CIL_TYPE:
> >         case CIL_TYPEATTRIBUTE:
> > -               /* type and typeattribute statements insert empty datums, ret true */
> > -               rc = CIL_TRUE;
> > +               if (!old || f != FLAVOR(old)) {
> > +                       rc = CIL_FALSE;
> > +               } else {
> > +                       /* type and typeattribute statements insert empty datums */
> > +                       rc = CIL_TRUE;
> > +               }
> >                 break;
> >         default:
> >                 break;
> > @@ -126,19 +130,20 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
> >         if (symtab != NULL) {
> >                 rc = cil_symtab_insert(symtab, (hashtab_key_t)key, datum, ast_node);
> >                 if (rc == SEPOL_EEXIST) {
> > +                       rc = cil_symtab_get_datum(symtab, (hashtab_key_t)key, &prev);
> > +                       if (rc != SEPOL_OK) {
> > +                               cil_log(CIL_ERR, "Re-declaration of %s %s, but previous declaration could not be found\n",cil_node_to_string(ast_node), key);
> > +                               goto exit;
> > +                       }
> >                         if (!db->multiple_decls ||
> > -                           cil_symtab_get_datum(symtab, (hashtab_key_t)key, &prev) != SEPOL_OK ||
> >                             !cil_is_datum_multiple_decl(datum, prev, nflavor)) {
> > -
> >                                 /* multiple_decls not ok, ret error */
> > +                               struct cil_tree_node *node = NODE(prev);
> >                                 cil_log(CIL_ERR, "Re-declaration of %s %s\n",
> >                                         cil_node_to_string(ast_node), key);
> > -                               if (cil_symtab_get_datum(symtab, key, &datum) == SEPOL_OK) {
> > -                                       if (sflavor == CIL_SYM_BLOCKS) {
> > -                                               struct cil_tree_node *node = datum->nodes->head->data;
> > -                                               cil_tree_log(node, CIL_ERR, "Previous declaration");
> > -                                       }
> > -                               }
> > +                               cil_tree_log(node, CIL_ERR, "Previous declaration of %s",
> > +                                       cil_node_to_string(node));
> > +                               rc = SEPOL_ERR;
> >                                 goto exit;
> >                         }
> >                         /* multiple_decls is enabled and works for this datum type, add node */
> > @@ -169,7 +174,7 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
> >         return SEPOL_OK;
> >
> >  exit:
> > -       cil_log(CIL_ERR, "Failed to create node\n");
> > +       cil_log(CIL_INFO, "Failed to create node\n");
>
> Is this useful/meaningful to retain?  Seems odd to have an
> informational message about a failure to create a node.

I definitely did not think that there was any value in reporting it as
an error and I agree with you that it is not very useful at all.

Jim