[PATCH 0/5 v2] secilc/cil: Fix name resolution for macro calls

[PATCH 0/5 v2] secilc/cil: Fix name resolution for macro calls

[James Carter]

This patch series fixes name resolution within expanded macro calls and
with call arguments.

The first patch is same as in v1.
Patches 2-4 replace patch 2 in v1. The original way of fixing name
resolution of call arguments did not work if a disabled optional
forced a reset.
Patch 5 is the same as patch 3 in v1.

James Carter (5):
  libsepol/cil: Make name resolution in macros work as documented
  libsepol/cil: Do not add NULL node when inserting key into symtab
  libsepo/cil: Refactor macro call resolution
  libsepol/cil: Do not resolve arguments to declarations in the call
  secilc/docs: Relocate and reword macro call name resolution order

 libsepol/cil/src/cil_internal.h          |   1 -
 libsepol/cil/src/cil_resolve_ast.c       | 635 ++++++++++++-----------
 libsepol/cil/src/cil_symtab.c            |   8 +-
 secilc/docs/cil_call_macro_statements.md |  24 +-
 4 files changed, 354 insertions(+), 314 deletions(-)

-- 
2.26.3

[PATCH 1/5 v2] libsepol/cil: Make name resolution in macros work as documented

[James Carter]

The CIL Reference Guide specifies how name resolution is suppose
to work within an expanded macro.
  1. Items defined inside the macro
  2. Items passed into the macro as arguments
  3. Items defined in the same namespace of the macro
  4. Items defined in the caller's namespace
  5. Items defined in the global namespace

But Lorenzo Ceragioli <lorenzo.ceragioli@phd.unipi.it> found
that the first step is not done.

So the following policy:
  (block A
    (type a)
    (macro m ()
      (type a)
      (allow a self (CLASS (PERM)))
    )
  )
  (block B
    (call A.m)
  )
will result in:
  (allow A.a self (CLASS (PERM)))
instead of the expected:
  (allow B.a self (CLASS (PERM)))

Now when an expanded call is found, the macro's namespace is
checked first. If the name is found, then the name was declared
in the macro and it is declared in the expanded call, so only the
namespace of the call up to and including the global namespace
will be searched. If the name is not found in the macro's namespace
then name resolution continues with steps 2-5 above.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_resolve_ast.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index f251ed15..bbe86e22 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -4224,10 +4224,18 @@ static int __cil_resolve_name_with_parents(struct cil_tree_node *node, char *nam
 			break;
 		case CIL_CALL: {
 			struct cil_call *call = node->data;
-			rc = cil_resolve_name_call_args(call, name, sym_index, datum);
-			if (rc != SEPOL_OK) {
-				/* Continue search in macro's parent */
-				rc = __cil_resolve_name_with_parents(NODE(call->macro)->parent, name, sym_index, datum);
+			struct cil_macro *macro = call->macro;
+			symtab = &macro->symtab[sym_index];
+			rc = cil_symtab_get_datum(symtab, name, datum);
+			if (rc == SEPOL_OK) {
+				/* If the name was declared in the macro, just look on the call side */
+				rc = SEPOL_ERR;
+			} else {
+				rc = cil_resolve_name_call_args(call, name, sym_index, datum);
+				if (rc != SEPOL_OK) {
+					/* Continue search in macro's parent */
+					rc = __cil_resolve_name_with_parents(NODE(call->macro)->parent, name, sym_index, datum);
+				}
 			}
 		}
 			break;
-- 
2.26.3

[PATCH 2/5 v2] libsepol/cil: Do not add NULL node when inserting key into symtab

[James Carter]

Allow inserting a key without providing a node.

This will make it easier to properly resolve call arguments where
a key might need to be temporarily removed to search for a datum
that is not declared within the call. Since the node is already
in the node list, re-inserting the key without this option would
add another link to the node and cause problems.

Also, do not add the node to the datum's node list if the result
of the call to hashtab_insert() is SEPOL_EEXIST because the datum
is a duplicate and will be destroyed.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_symtab.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libsepol/cil/src/cil_symtab.c b/libsepol/cil/src/cil_symtab.c
index 579a888e..c1951560 100644
--- a/libsepol/cil/src/cil_symtab.c
+++ b/libsepol/cil/src/cil_symtab.c
@@ -93,10 +93,10 @@ int cil_symtab_insert(symtab_t *symtab, hashtab_key_t key, struct cil_symtab_dat
 		datum->fqn = key;
 		datum->symtab = symtab;
 		symtab->nprim++;
-		cil_list_append(datum->nodes, CIL_NODE, node);
-	} else if (rc == SEPOL_EEXIST) {
-		cil_list_append(datum->nodes, CIL_NODE, node);
-	} else {
+		if (node) {
+			cil_list_append(datum->nodes, CIL_NODE, node);
+		}
+	} else if (rc != SEPOL_EEXIST) {
 		cil_symtab_error("Failed to insert datum into hashtab\n");
 	}
 
-- 
2.26.3

[PATCH 3/5 v2] libsepo/cil: Refactor macro call resolution

[James Carter]

Rename cil_resolve_call1() as cil resolve_call() and rename
cil_resolve_call2() as cil_resolve_call_args() to make it clearer
what is being done in each function.

Move code to build call arguments out of cil_resolve_call() and into
the new function called cil_build_call_args() so that the logic of
cil_resolve_call() can be seen.

Exit cil_resolve_call() immediately if the call has already been
copied.

In __cil_resolve_ast_node(), only resolve calls outside of macros.
This results in more calls to cil_copy_ast(), but slightly less
rules copied overall (since no rules are copied into a macro). This
also means that the CIL_PASS_MACRO pass is not needed and can be
eliminated.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_internal.h    |   1 -
 libsepol/cil/src/cil_resolve_ast.c | 599 +++++++++++++++--------------
 2 files changed, 303 insertions(+), 297 deletions(-)

diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
index 74e0b34d..a77c9520 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -59,7 +59,6 @@ enum cil_pass {
 	CIL_PASS_BLKIN_LINK,
 	CIL_PASS_BLKIN_COPY,
 	CIL_PASS_BLKABS,
-	CIL_PASS_MACRO,
 	CIL_PASS_CALL1,
 	CIL_PASS_CALL2,
 	CIL_PASS_ALIAS1,
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index bbe86e22..b96118ce 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -2840,359 +2840,371 @@ exit:
 	return rc;
 }
 
-int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
+static int cil_build_call_args(struct cil_tree_node *call_node, struct cil_call *call, struct cil_macro *macro, void *extra_args)
 {
-	struct cil_call *new_call = current->data;
 	struct cil_args_resolve *args = extra_args;
-	struct cil_db *db = NULL;
-	struct cil_tree_node *macro_node = NULL;
-	struct cil_symtab_datum *macro_datum = NULL;
+	struct cil_list_item *item;
+	struct cil_args *arg = NULL;
+	struct cil_tree_node *arg_node = NULL;
 	int rc = SEPOL_ERR;
 
-	if (args != NULL) {
-		db = args->db;
+	if (macro->params == NULL) {
+		if (call->args_tree == NULL) {
+			return SEPOL_OK;
+		} else {
+			cil_tree_log(call_node, CIL_ERR, "Unexpected arguments");
+			return SEPOL_ERR;;
+		}
 	}
-
-	rc = cil_resolve_name(current, new_call->macro_str, CIL_SYM_BLOCKS, extra_args, &macro_datum);
-	if (rc != SEPOL_OK) {
-		goto exit;
+	if (call->args_tree == NULL) {
+		cil_tree_log(call_node, CIL_ERR, "Missing arguments");
+		return SEPOL_ERR;
 	}
 
-	macro_node = NODE(macro_datum);
+	arg_node = call->args_tree->root->cl_head;
 
-	if (macro_node->flavor != CIL_MACRO) {
-		cil_tree_log(current, CIL_ERR, "Failed to resolve %s to a macro", new_call->macro_str);
-		rc = SEPOL_ERR;
-		goto exit;
-	}
-	new_call->macro = (struct cil_macro*)macro_datum;
+	cil_list_init(&call->args, CIL_LIST_ITEM);
 
-	if (new_call->macro->params != NULL ) {
+	cil_list_for_each(item, macro->params) {
+		enum cil_flavor flavor = ((struct cil_param*)item->data)->flavor;
 
-		struct cil_list_item *item;
-		struct cil_args *new_arg = NULL;
-		struct cil_tree_node *pc = NULL;
-
-		if (new_call->args_tree == NULL) {
-			cil_tree_log(current, CIL_ERR, "Missing arguments");
+		if (arg_node == NULL) {
+			cil_tree_log(call_node, CIL_ERR, "Missing arguments");
+			rc = SEPOL_ERR;
+			goto exit;
+		}
+		if (item->flavor != CIL_PARAM) {
 			rc = SEPOL_ERR;
 			goto exit;
 		}
 
-		pc = new_call->args_tree->root->cl_head;
-
-		cil_list_init(&new_call->args, CIL_LIST_ITEM);
-
-		cil_list_for_each(item, new_call->macro->params) {
-			enum cil_flavor flavor = ((struct cil_param*)item->data)->flavor;
+		cil_args_init(&arg);
 
-			if (pc == NULL) {
-				cil_tree_log(current, CIL_ERR, "Missing arguments");
+		switch (flavor) {
+		case CIL_NAME: {
+			struct cil_name *name;
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
 				rc = SEPOL_ERR;
 				goto exit;
 			}
-			if (item->flavor != CIL_PARAM) {
+			name = __cil_insert_name(args->db, arg_node->data, call_node);
+			if (name != NULL) {
+				arg->arg = (struct cil_symtab_datum *)name;
+			} else {
+				arg->arg_str = arg_node->data;
+			}
+		}
+			break;
+		case CIL_TYPE:
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
 				rc = SEPOL_ERR;
 				goto exit;
 			}
-
-			cil_args_init(&new_arg);
-
-			switch (flavor) {
-			case CIL_NAME: {
-				struct cil_name *name;
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				name = __cil_insert_name(args->db, pc->data, current);
-				if (name != NULL) {
-					new_arg->arg = (struct cil_symtab_datum *)name;
-				} else {
-					new_arg->arg_str = pc->data;
-				}
+			arg->arg_str = arg_node->data;
+			break;
+		case CIL_ROLE:
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
 			}
-				break;
-			case CIL_TYPE:
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				new_arg->arg_str = pc->data;
-				break;
-			case CIL_ROLE:
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				new_arg->arg_str = pc->data;
-				break;
-			case CIL_USER:
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				new_arg->arg_str = pc->data;
-				break;
-			case CIL_SENS:
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				new_arg->arg_str = pc->data;
-				break;
-			case CIL_CAT:
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				new_arg->arg_str = pc->data;
-				break;
-			case CIL_BOOL:
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				new_arg->arg_str = pc->data;
-				break;
-			case CIL_CATSET: {
-				if (pc->cl_head != NULL) {
-					struct cil_catset *catset = NULL;
-					struct cil_tree_node *cat_node = NULL;
-					cil_catset_init(&catset);
-					rc = cil_fill_cats(pc, &catset->cats);
-					if (rc != SEPOL_OK) {
-						cil_destroy_catset(catset);
-						cil_destroy_args(new_arg);
-						goto exit;
-					}
-					cil_tree_node_init(&cat_node);
-					cat_node->flavor = CIL_CATSET;
-					cat_node->data = catset;
-					cil_list_append(((struct cil_symtab_datum*)catset)->nodes,
-									CIL_LIST_ITEM, cat_node);
-					new_arg->arg = (struct cil_symtab_datum*)catset;
-				} else if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
+			arg->arg_str = arg_node->data;
+			break;
+		case CIL_USER:
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			arg->arg_str = arg_node->data;
+			break;
+		case CIL_SENS:
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			arg->arg_str = arg_node->data;
+			break;
+		case CIL_CAT:
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			arg->arg_str = arg_node->data;
+			break;
+		case CIL_BOOL:
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			arg->arg_str = arg_node->data;
+			break;
+		case CIL_CATSET: {
+			if (arg_node->cl_head != NULL) {
+				struct cil_catset *catset = NULL;
+				struct cil_tree_node *cat_node = NULL;
+				cil_catset_init(&catset);
+				rc = cil_fill_cats(arg_node, &catset->cats);
+				if (rc != SEPOL_OK) {
+					cil_destroy_catset(catset);
+					cil_destroy_args(arg);
 					goto exit;
-				} else {
-					new_arg->arg_str = pc->data;
 				}
-
-				break;
+				cil_tree_node_init(&cat_node);
+				cat_node->flavor = CIL_CATSET;
+				cat_node->data = catset;
+				cil_list_append(((struct cil_symtab_datum*)catset)->nodes,
+								CIL_LIST_ITEM, cat_node);
+				arg->arg = (struct cil_symtab_datum*)catset;
+			} else if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			} else {
+				arg->arg_str = arg_node->data;
 			}
-			case CIL_LEVEL: {
-				if (pc->cl_head != NULL) {
-					struct cil_level *level = NULL;
-					struct cil_tree_node *lvl_node = NULL;
-					cil_level_init(&level);
-
-					rc = cil_fill_level(pc->cl_head, level);
-					if (rc != SEPOL_OK) {
-						cil_log(CIL_ERR, "Failed to create anonymous level, rc: %d\n", rc);
-						cil_destroy_level(level);
-						cil_destroy_args(new_arg);
-						goto exit;
-					}
-					cil_tree_node_init(&lvl_node);
-					lvl_node->flavor = CIL_LEVEL;
-					lvl_node->data = level;
-					cil_list_append(((struct cil_symtab_datum*)level)->nodes, 
-									CIL_LIST_ITEM, lvl_node);
-					new_arg->arg = (struct cil_symtab_datum*)level;
-				} else if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
+
+			break;
+		}
+		case CIL_LEVEL: {
+			if (arg_node->cl_head != NULL) {
+				struct cil_level *level = NULL;
+				struct cil_tree_node *lvl_node = NULL;
+				cil_level_init(&level);
+
+				rc = cil_fill_level(arg_node->cl_head, level);
+				if (rc != SEPOL_OK) {
+					cil_log(CIL_ERR, "Failed to create anonymous level, rc: %d\n", rc);
+					cil_destroy_level(level);
+					cil_destroy_args(arg);
 					goto exit;
-				} else {
-					new_arg->arg_str = pc->data;
 				}
-
-				break;
+				cil_tree_node_init(&lvl_node);
+				lvl_node->flavor = CIL_LEVEL;
+				lvl_node->data = level;
+				cil_list_append(((struct cil_symtab_datum*)level)->nodes,
+								CIL_LIST_ITEM, lvl_node);
+				arg->arg = (struct cil_symtab_datum*)level;
+			} else if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			} else {
+				arg->arg_str = arg_node->data;
 			}
-			case CIL_LEVELRANGE: {
-				if (pc->cl_head != NULL) {
-					struct cil_levelrange *range = NULL;
-					struct cil_tree_node *range_node = NULL;
-					cil_levelrange_init(&range);
-
-					rc = cil_fill_levelrange(pc->cl_head, range);
-					if (rc != SEPOL_OK) {
-						cil_log(CIL_ERR, "Failed to create anonymous levelrange, rc: %d\n", rc);
-						cil_destroy_levelrange(range);
-						cil_destroy_args(new_arg);
-						goto exit;
-					}
-					cil_tree_node_init(&range_node);
-					range_node->flavor = CIL_LEVELRANGE;
-					range_node->data = range;
-					cil_list_append(((struct cil_symtab_datum*)range)->nodes, 
-									CIL_LIST_ITEM, range_node);
-					new_arg->arg = (struct cil_symtab_datum*)range;
-				} else if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
+
+			break;
+		}
+		case CIL_LEVELRANGE: {
+			if (arg_node->cl_head != NULL) {
+				struct cil_levelrange *range = NULL;
+				struct cil_tree_node *range_node = NULL;
+				cil_levelrange_init(&range);
+
+				rc = cil_fill_levelrange(arg_node->cl_head, range);
+				if (rc != SEPOL_OK) {
+					cil_log(CIL_ERR, "Failed to create anonymous levelrange, rc: %d\n", rc);
+					cil_destroy_levelrange(range);
+					cil_destroy_args(arg);
 					goto exit;
-				} else {
-					new_arg->arg_str = pc->data;
 				}
-
-				break;
+				cil_tree_node_init(&range_node);
+				range_node->flavor = CIL_LEVELRANGE;
+				range_node->data = range;
+				cil_list_append(((struct cil_symtab_datum*)range)->nodes,
+								CIL_LIST_ITEM, range_node);
+				arg->arg = (struct cil_symtab_datum*)range;
+			} else if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			} else {
+				arg->arg_str = arg_node->data;
 			}
-			case CIL_IPADDR: {
-				if (pc->cl_head != NULL) {
-					struct cil_ipaddr *ipaddr = NULL;
-					struct cil_tree_node *addr_node = NULL;
-					cil_ipaddr_init(&ipaddr);
-
-					rc = cil_fill_ipaddr(pc->cl_head, ipaddr);
-					if (rc != SEPOL_OK) {
-						cil_log(CIL_ERR, "Failed to create anonymous ip address, rc: %d\n", rc);
-						cil_destroy_ipaddr(ipaddr);
-						cil_destroy_args(new_arg);
-						goto exit;
-					}
-					cil_tree_node_init(&addr_node);
-					addr_node->flavor = CIL_IPADDR;
-					addr_node->data = ipaddr;
-					cil_list_append(((struct cil_symtab_datum*)ipaddr)->nodes,
-									CIL_LIST_ITEM, addr_node);
-					new_arg->arg = (struct cil_symtab_datum*)ipaddr;
-				} else if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
+
+			break;
+		}
+		case CIL_IPADDR: {
+			if (arg_node->cl_head != NULL) {
+				struct cil_ipaddr *ipaddr = NULL;
+				struct cil_tree_node *addr_node = NULL;
+				cil_ipaddr_init(&ipaddr);
+
+				rc = cil_fill_ipaddr(arg_node->cl_head, ipaddr);
+				if (rc != SEPOL_OK) {
+					cil_log(CIL_ERR, "Failed to create anonymous ip address, rc: %d\n", rc);
+					cil_destroy_ipaddr(ipaddr);
+					cil_destroy_args(arg);
 					goto exit;
-				} else {
-					new_arg->arg_str = pc->data;
 				}
+				cil_tree_node_init(&addr_node);
+				addr_node->flavor = CIL_IPADDR;
+				addr_node->data = ipaddr;
+				cil_list_append(((struct cil_symtab_datum*)ipaddr)->nodes,
+								CIL_LIST_ITEM, addr_node);
+				arg->arg = (struct cil_symtab_datum*)ipaddr;
+			} else if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			} else {
+				arg->arg_str = arg_node->data;
+			}
 
-				break;
+			break;
+		}
+		case CIL_CLASS:
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
 			}
-			case CIL_CLASS:
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				new_arg->arg_str = pc->data;
-				break;
-			case CIL_MAP_CLASS:
-				if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
-					goto exit;
-				}
-				new_arg->arg_str = pc->data;
-				break;
-			case CIL_CLASSPERMISSION: {
-				if (pc->cl_head != NULL) {
-					struct cil_classpermission *cp = NULL;
-					struct cil_tree_node *cp_node = NULL;
-
-					cil_classpermission_init(&cp);
-					rc = cil_fill_classperms_list(pc, &cp->classperms);
-					if (rc != SEPOL_OK) {
-						cil_log(CIL_ERR, "Failed to create anonymous classpermission\n");
-						cil_destroy_classpermission(cp);
-						cil_destroy_args(new_arg);
-						goto exit;
-					}
-					cil_tree_node_init(&cp_node);
-					cp_node->flavor = CIL_CLASSPERMISSION;
-					cp_node->data = cp;
-					cil_list_append(cp->datum.nodes, CIL_LIST_ITEM, cp_node);
-					new_arg->arg = (struct cil_symtab_datum*)cp;
-				} else if (pc->data == NULL) {
-					cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
-					cil_destroy_args(new_arg);
-					rc = SEPOL_ERR;
+			arg->arg_str = arg_node->data;
+			break;
+		case CIL_MAP_CLASS:
+			if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			arg->arg_str = arg_node->data;
+			break;
+		case CIL_CLASSPERMISSION: {
+			if (arg_node->cl_head != NULL) {
+				struct cil_classpermission *cp = NULL;
+				struct cil_tree_node *cp_node = NULL;
+
+				cil_classpermission_init(&cp);
+				rc = cil_fill_classperms_list(arg_node, &cp->classperms);
+				if (rc != SEPOL_OK) {
+					cil_log(CIL_ERR, "Failed to create anonymous classpermission\n");
+					cil_destroy_classpermission(cp);
+					cil_destroy_args(arg);
 					goto exit;
-				} else {
-					new_arg->arg_str = pc->data;
 				}
-				break;
-			}
-			default:
-				cil_log(CIL_ERR, "Unexpected flavor: %d\n", 
-						(((struct cil_param*)item->data)->flavor));
-				cil_destroy_args(new_arg);
+				cil_tree_node_init(&cp_node);
+				cp_node->flavor = CIL_CLASSPERMISSION;
+				cp_node->data = cp;
+				cil_list_append(cp->datum.nodes, CIL_LIST_ITEM, cp_node);
+				arg->arg = (struct cil_symtab_datum*)cp;
+			} else if (arg_node->data == NULL) {
+				cil_tree_log(call_node, CIL_ERR, "Invalid macro parameter");
+				cil_destroy_args(arg);
 				rc = SEPOL_ERR;
 				goto exit;
+			} else {
+				arg->arg_str = arg_node->data;
 			}
-			new_arg->param_str = ((struct cil_param*)item->data)->str;
-			new_arg->flavor = flavor;
-
-			cil_list_append(new_call->args, CIL_ARGS, new_arg);
-
-			pc = pc->next;
+			break;
 		}
-
-		if (pc != NULL) {
-			cil_tree_log(current, CIL_ERR, "Unexpected arguments");
+		default:
+			cil_log(CIL_ERR, "Unexpected flavor: %d\n",
+					(((struct cil_param*)item->data)->flavor));
+			cil_destroy_args(arg);
 			rc = SEPOL_ERR;
 			goto exit;
 		}
-	} else if (new_call->args_tree != NULL) {
-		cil_tree_log(current, CIL_ERR, "Unexpected arguments");
+		arg->param_str = ((struct cil_param*)item->data)->str;
+		arg->flavor = flavor;
+
+		cil_list_append(call->args, CIL_ARGS, arg);
+
+		arg_node = arg_node->next;
+	}
+
+	if (arg_node != NULL) {
+		cil_tree_log(call_node, CIL_ERR, "Unexpected arguments");
 		rc = SEPOL_ERR;
 		goto exit;
 	}
 
-	if (new_call->copied == 0) {
-		new_call->copied = 1;
+	return SEPOL_OK;
 
-		rc = cil_check_recursive_call(current, macro_node);
-		if (rc != SEPOL_OK) {
-			goto exit;
-		}
+exit:
+	return rc;
+}
 
-		rc = cil_copy_ast(db, macro_node, current);
-		if (rc != SEPOL_OK) {
-			cil_log(CIL_ERR, "Failed to copy macro, rc: %d\n", rc);
-			goto exit;
-		}
+int cil_resolve_call(struct cil_tree_node *current, void *extra_args)
+{
+	struct cil_call *call = current->data;
+	struct cil_args_resolve *args = extra_args;
+	struct cil_tree_node *macro_node = NULL;
+	struct cil_symtab_datum *macro_datum = NULL;
+	int rc = SEPOL_ERR;
+
+	if (call->copied) {
+		return SEPOL_OK;
+	}
+
+	rc = cil_resolve_name(current, call->macro_str, CIL_SYM_BLOCKS, extra_args, &macro_datum);
+	if (rc != SEPOL_OK) {
+		goto exit;
+	}
+
+	macro_node = NODE(macro_datum);
+
+	if (macro_node->flavor != CIL_MACRO) {
+		cil_tree_log(current, CIL_ERR, "Failed to resolve %s to a macro", call->macro_str);
+		rc = SEPOL_ERR;
+		goto exit;
+	}
+	call->macro = (struct cil_macro*)macro_datum;
+
+	rc = cil_build_call_args(current, call, call->macro, extra_args);
+	if (rc != SEPOL_OK) {
+		goto exit;
 	}
 
+	rc = cil_check_recursive_call(current, macro_node);
+	if (rc != SEPOL_OK) {
+		goto exit;
+	}
+
+	rc = cil_copy_ast(args->db, macro_node, current);
+	if (rc != SEPOL_OK) {
+		cil_tree_log(current, CIL_ERR, "Failed to copy macro %s to call", macro_datum->name);
+		goto exit;
+	}
+
+	call->copied = 1;
+
 	return SEPOL_OK;
 
 exit:
 	return rc;
 }
 
-int cil_resolve_call2(struct cil_tree_node *current, void *extra_args)
+int cil_resolve_call_args(struct cil_tree_node *current, void *extra_args)
 {
-	struct cil_call *new_call = current->data;
+	struct cil_call *call = current->data;
 	int rc = SEPOL_ERR;
 	enum cil_sym_index sym_index = CIL_SYM_UNKNOWN;
 	struct cil_list_item *item;
 
-	if (new_call->args == NULL) {
+	if (call->args == NULL) {
 		rc = SEPOL_OK;
 		goto exit;
 	}
 
-	cil_list_for_each(item, new_call->args) {
+	cil_list_for_each(item, call->args) {
 		struct cil_args *arg = item->data;
 		if (arg->arg == NULL && arg->arg_str == NULL) {
 			cil_log(CIL_ERR, "Arguments not created correctly\n");
@@ -3603,19 +3615,14 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
 			rc = cil_resolve_blockabstract(node, args);
 		}
 		break;
-	case CIL_PASS_MACRO:
-		if (node->flavor == CIL_CALL && args->macro != NULL) {
-			rc = cil_resolve_call1(node, args);
-		}
-		break;
 	case CIL_PASS_CALL1:
-		if (node->flavor == CIL_CALL) {
-			rc = cil_resolve_call1(node, args);
+		if (node->flavor == CIL_CALL && args->macro == NULL) {
+			rc = cil_resolve_call(node, args);
 		}
 		break;
 	case CIL_PASS_CALL2:
-		if (node->flavor == CIL_CALL) {
-			rc = cil_resolve_call2(node, args);
+		if (node->flavor == CIL_CALL && args->macro == NULL) {
+			rc = cil_resolve_call_args(node, args);
 		}
 		break;
 	case CIL_PASS_ALIAS1:
@@ -3919,7 +3926,7 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 	}
 
 	if (node->flavor == CIL_MACRO) {
-		if (pass != CIL_PASS_TIF && pass != CIL_PASS_MACRO) {
+		if (pass != CIL_PASS_TIF) {
 			*finished = CIL_TREE_SKIP_HEAD;
 			rc = SEPOL_OK;
 			goto exit;
-- 
2.26.3

[PATCH 4/5 v2] libsepol/cil: Do not resolve arguments to declarations in the call

[James Carter]

Lorenzo Ceragioli <lorenzo.ceragioli@phd.unipi.it> noted that the
following policy:
  (type a)
  (block A
    (macro m ((type x))
      (type a)
      (allow x x (file (read))))
  )
  (block B
    (call A.m(a))
  )
results in the allow rule (allow B.a B.a (file(read))). This makes
no sense because the "a" being passed as an argument has to be the
global "a" and not the "a" defined in the macro.

This behavior occurs because the call arguments are resolved AFTER
the macro body has been copied and the declaration of "a" in the
macro has been added to block B's namespace, so this is the "a"
that the call argument resolves to, rather than the one in the
global namespace.

When resolving call arguments, check if the datum found belongs to
a declaration in the call. If it does, then remove the datum from
the symbol table, re-resolve the argument, and add the datum back
into the symbol table.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_resolve_ast.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index b96118ce..d416ce75 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3287,11 +3287,37 @@ int cil_resolve_call_args(struct cil_tree_node *current, void *extra_args)
 		}
 
 		if (sym_index != CIL_SYM_UNKNOWN) {
-			rc = cil_resolve_name(current, arg->arg_str, sym_index, extra_args, &(arg->arg));
+			struct cil_symtab_datum *datum;
+			struct cil_tree_node *n;
+			rc = cil_resolve_name(current, arg->arg_str, sym_index, extra_args, &datum);
 			if (rc != SEPOL_OK) {
 				cil_tree_log(current, CIL_ERR, "Failed to resolve %s in call argument list", arg->arg_str);
 				goto exit;
 			}
+			arg->arg = datum;
+			n = NODE(datum);
+			while (n && n->flavor != CIL_ROOT) {
+				if (n == current) {
+					symtab_t *s = datum->symtab;
+					/* Call arg should not resolve to declaration in the call
+					 * Need to remove datum temporarily to resolve to a datum outside
+					 * the call.
+					 */
+					cil_symtab_remove_datum(datum);
+					rc = cil_resolve_name(current, arg->arg_str, sym_index, extra_args, &(arg->arg));
+					if (rc != SEPOL_OK) {
+						cil_tree_log(current, CIL_ERR, "Failed to resolve %s in call argument list", arg->arg_str);
+						goto exit;
+					}
+					rc = cil_symtab_insert(s, datum->name, datum, NULL);
+					if (rc != SEPOL_OK) {
+						cil_tree_log(current, CIL_ERR, "Failed to re-insert datum while resolving %s in call argument list", arg->arg_str);
+						goto exit;
+					}
+					break;
+				}
+				n = n->parent;
+			}
 		}
 	}
 
-- 
2.26.3

[PATCH 5/5 v2] secilc/docs: Relocate and reword macro call name resolution order

[James Carter]

The listing of the order was in the macro section, but it belongs
in the call section.

Move the listing of the order to the call section and provide a
better explanation.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 secilc/docs/cil_call_macro_statements.md | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/secilc/docs/cil_call_macro_statements.md b/secilc/docs/cil_call_macro_statements.md
index 352a9fb0..3cc14bf8 100644
--- a/secilc/docs/cil_call_macro_statements.md
+++ b/secilc/docs/cil_call_macro_statements.md
@@ -8,6 +8,18 @@ Instantiate a [macro](#macro) within the current namespace. There may be zero or
 
 Each parameter passed contains an argument to be resolved by the [macro](#macro), these can be named or anonymous but must conform to the parameter types defined in the [`macro`](cil_call_macro_statements.md#macro) statement.
 
+Macro rules are resolved by searching in the following order:
+
+-   The macro namespace (If found this means that the name was declared in the macro and is now declared in the namespace of one of the parents of the call.)
+
+-   The call arguments
+
+-   The parent namespaces of the macro being called (if any) with the exception of the global namespace.
+
+-   The parent namespaces of the call (if any) with the exception of the global namespace.
+
+-   The global namespace
+
 **Statement definition:**
 
 ```secil
@@ -46,18 +58,6 @@ macro
 
 Declare a macro in the current namespace with its associated parameters. The macro identifier is used by the [`call`](cil_call_macro_statements.md#call) statement to instantiate the macro and resolve any parameters. The call statement may be within the body of a macro.
 
-When resolving macros the following places are checked in this order:
-
--   Items defined inside the macro
-
--   Items passed into the macro as arguments
-
--   Items defined in the same namespace of the macro
-
--   Items defined in the callers namespace
-
--   Items defined in the global namespace
-
 [`tunable`](cil_conditional_statements.md#tunable), [`in`](cil_container_statements.md#in), [`block`](cil_container_statements.md#block), [`blockinherit`](cil_container_statements.md#blockinherit), [`blockabstract`](cil_container_statements.md#blockabstract), and other [`macro`](cil_call_macro_statements.md#macro) statements are not allowed in [`macro`](cil_call_macro_statements.md#macro) blocks.
 
 **Statement definition:**
-- 
2.26.3

Re: [PATCH 0/5 v2] secilc/cil: Fix name resolution for macro calls

[James Carter]

On Tue, May 11, 2021 at 4:03 PM James Carter <jwcart2@gmail.com> wrote:
>
> This patch series fixes name resolution within expanded macro calls and
> with call arguments.
>
> The first patch is same as in v1.
> Patches 2-4 replace patch 2 in v1. The original way of fixing name
> resolution of call arguments did not work if a disabled optional
> forced a reset.
> Patch 5 is the same as patch 3 in v1.
>
> James Carter (5):
>   libsepol/cil: Make name resolution in macros work as documented
>   libsepol/cil: Do not add NULL node when inserting key into symtab
>   libsepo/cil: Refactor macro call resolution
>   libsepol/cil: Do not resolve arguments to declarations in the call
>   secilc/docs: Relocate and reword macro call name resolution order
>
>  libsepol/cil/src/cil_internal.h          |   1 -
>  libsepol/cil/src/cil_resolve_ast.c       | 635 ++++++++++++-----------
>  libsepol/cil/src/cil_symtab.c            |   8 +-
>  secilc/docs/cil_call_macro_statements.md |  24 +-
>  4 files changed, 354 insertions(+), 314 deletions(-)
>
> --
> 2.26.3
>

There hasn't been any comments on this series, and it has been more
than three weeks since I sent it, so I plan on merging it tomorrow.
Jim

Re: [PATCH 0/5 v2] secilc/cil: Fix name resolution for macro calls

[James Carter]

On Thu, Jun 3, 2021 at 1:06 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 4:03 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > This patch series fixes name resolution within expanded macro calls and
> > with call arguments.
> >
> > The first patch is same as in v1.
> > Patches 2-4 replace patch 2 in v1. The original way of fixing name
> > resolution of call arguments did not work if a disabled optional
> > forced a reset.
> > Patch 5 is the same as patch 3 in v1.
> >
> > James Carter (5):
> >   libsepol/cil: Make name resolution in macros work as documented
> >   libsepol/cil: Do not add NULL node when inserting key into symtab
> >   libsepo/cil: Refactor macro call resolution
> >   libsepol/cil: Do not resolve arguments to declarations in the call
> >   secilc/docs: Relocate and reword macro call name resolution order
> >
> >  libsepol/cil/src/cil_internal.h          |   1 -
> >  libsepol/cil/src/cil_resolve_ast.c       | 635 ++++++++++++-----------
> >  libsepol/cil/src/cil_symtab.c            |   8 +-
> >  secilc/docs/cil_call_macro_statements.md |  24 +-
> >  4 files changed, 354 insertions(+), 314 deletions(-)
> >
> > --
> > 2.26.3
> >
>
> There hasn't been any comments on this series, and it has been more
> than three weeks since I sent it, so I plan on merging it tomorrow.
> Jim

This series has been merged.
Jim