On Tue, May 26, 2020 at 11:47 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> On Tue, May 19, 2020 at 10:30 AM James Carter <jwcart2@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> > ---
> > 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
