On Tue, Aug 10, 2021 at 8:22 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> CIL supports specifiying the original high-level language file and
> line numbers when reporting errors. This is done through line marks
> and is mostly used to report the original Refpolicy file and line
> number for neverallow rules that have been converted to CIL.
>
> As long as the line mark remain simple, everything works fine, but
> the wrong line numbers will be reported with more complex nextings
> of line marks.
>
> Example:
> ;;* lms 100 file01.hll
> (type t1a)
> (allow t1a self (CLASS (PERM)))
> ;;* lmx 200 file02.hll
> (type t2a)
> (allow t2a self (CLASS (PERM)))
> ;;* lme
> (type t1b)
> (allow t1b self (CLASS (PERM)))
> (allow bad1b self (CLASS (PERM))) ; file01.hll:101 (Should be 106)
> ;;* lme
>
> The primary problem is that the tree nodes can only store one hll
> line number. Instead a number is needed that can be used by any
> number of stacked line mark sections. This number would increment
> line a normal line number except when in lmx sections (that have
> the same line number throughout the section because they represent
> an expansion of a line -- like the expansion of a macro call. This
> number can go backwards when exiting a lms section within a lmx
> section, because line number will increase in the lms section, but
> outside the lmx section, the line number did not advance.
>
> This number is called the hll_offset and this is the value that is
> now stored in tree nodes instead of the hll line number. To calculate
> the hll line number for a rule, a search is made for an ancestor of
> the node that is a line mark and the line number for a lms section
> is the hll line number stored in the line mark, plus the hll offset
> of the rule, minus the hll offset of the line mark node, minus one.
> (hll_lineno + hll_offset_rule - hll_offset_lm - 1)
>
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> libsepol/cil/src/cil_binary.c | 9 ++--
> libsepol/cil/src/cil_build_ast.c | 4 +-
> libsepol/cil/src/cil_copy_ast.c | 2 +-
> libsepol/cil/src/cil_parser.c | 74 +++++++++++++++++++-------------
> libsepol/cil/src/cil_tree.c | 53 +++++++++++++++--------
> libsepol/cil/src/cil_tree.h | 4 +-
> 6 files changed, 90 insertions(+), 56 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 2b65c622..43c37fc2 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -4480,7 +4480,8 @@ static avrule_t *__cil_init_sepol_avrule(uint32_t kind, struct cil_tree_node *no
> avrule_t *avrule;
> struct cil_tree_node *source_node;
> char *source_path;
> - int is_cil;
> + char *lm_kind;
> + uint32_t hll_line;
>
> avrule = cil_malloc(sizeof(avrule_t));
> avrule->specified = kind;
> @@ -4492,11 +4493,11 @@ static avrule_t *__cil_init_sepol_avrule(uint32_t kind, struct cil_tree_node *no
>
> avrule->source_filename = NULL;
> avrule->source_line = node->line;
> - source_node = cil_tree_get_next_path(node, &source_path, &is_cil);
> + source_node = cil_tree_get_next_path(node, &lm_kind, &hll_line, &source_path);
> if (source_node) {
> avrule->source_filename = source_path;
> - if (!is_cil) {
> - avrule->source_line = node->hll_line;
> + if (lm_kind != CIL_KEY_SRC_CIL) {
> + avrule->source_line = hll_line + node->hll_offset - source_node->hll_offset - 1;
> }
> }
>
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index a0f58b1e..a5afc267 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -619,7 +619,7 @@ int cil_gen_perm_nodes(struct cil_db *db, struct cil_tree_node *current_perm, st
> cil_tree_node_init(&new_ast);
> new_ast->parent = ast_node;
> new_ast->line = current_perm->line;
> - new_ast->hll_line = current_perm->hll_line;
> + new_ast->hll_offset = current_perm->hll_offset;
>
> rc = cil_gen_perm(db, current_perm, new_ast, flavor, num_perms);
> if (rc != SEPOL_OK) {
> @@ -6203,7 +6203,7 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
>
> ast_node->parent = ast_current;
> ast_node->line = parse_current->line;
> - ast_node->hll_line = parse_current->hll_line;
> + ast_node->hll_offset = parse_current->hll_offset;
>
> if (parse_current->data == CIL_KEY_BLOCK) {
> rc = cil_gen_block(db, parse_current, ast_node, 0);
> diff --git a/libsepol/cil/src/cil_copy_ast.c b/libsepol/cil/src/cil_copy_ast.c
> index 02b9828f..34282a92 100644
> --- a/libsepol/cil/src/cil_copy_ast.c
> +++ b/libsepol/cil/src/cil_copy_ast.c
> @@ -2010,7 +2010,7 @@ int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) u
>
> new->parent = parent;
> new->line = orig->line;
> - new->hll_line = orig->hll_line;
> + new->hll_offset = orig->hll_offset;
> new->flavor = orig->flavor;
> new->data = data;
>
> diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
> index 842c327c..3ccef5d7 100644
> --- a/libsepol/cil/src/cil_parser.c
> +++ b/libsepol/cil/src/cil_parser.c
> @@ -45,21 +45,21 @@
> #define CIL_PARSER_MAX_EXPR_DEPTH (0x1 << 12)
>
> struct hll_info {
> - uint32_t hll_lineno;
> + uint32_t hll_offset;
> uint32_t hll_expand;
> };
>
> -static void push_hll_info(struct cil_stack *stack, uint32_t hll_lineno, uint32_t hll_expand)
> +static void push_hll_info(struct cil_stack *stack, uint32_t hll_offset, uint32_t hll_expand)
> {
> struct hll_info *new = cil_malloc(sizeof(*new));
>
> - new->hll_lineno = hll_lineno;
> + new->hll_offset = hll_offset;
> new->hll_expand = hll_expand;
>
> cil_stack_push(stack, CIL_NONE, new);
> }
>
> -static void pop_hll_info(struct cil_stack *stack, uint32_t *hll_lineno, uint32_t *hll_expand)
> +static void pop_hll_info(struct cil_stack *stack, uint32_t *hll_offset, uint32_t *hll_expand)
> {
> struct cil_stack_item *curr = cil_stack_pop(stack);
> struct hll_info *info;
> @@ -69,17 +69,17 @@ static void pop_hll_info(struct cil_stack *stack, uint32_t *hll_lineno, uint32_t
> }
> info = curr->data;
> *hll_expand = info->hll_expand;
> - *hll_lineno = info->hll_lineno;
> + *hll_offset = info->hll_offset;
> free(curr->data);
> }
>
> -static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, uint32_t line, uint32_t hll_line, void *value)
> +static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, uint32_t line, uint32_t hll_offset, void *value)
> {
> cil_tree_node_init(node);
> (*node)->parent = current;
> (*node)->flavor = CIL_NODE;
> (*node)->line = line;
> - (*node)->hll_line = hll_line;
> + (*node)->hll_offset = hll_offset;
> (*node)->data = value;
> }
>
> @@ -93,12 +93,12 @@ static void insert_node(struct cil_tree_node *node, struct cil_tree_node *curren
> current->cl_tail = node;
> }
>
> -static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno, uint32_t *hll_expand, struct cil_stack *stack, char *path)
> +static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_offset, uint32_t *hll_expand, struct cil_stack *stack, char *path)
> {
> char *hll_type;
> struct cil_tree_node *node;
> struct token tok;
> - int rc;
> + uint32_t prev_hll_expand, prev_hll_offset;
In this function, gcc (in GitHub Actions CI) reports:
../cil/src/cil_parser.c: In function ‘cil_parser’:
../cil/src/cil_parser.c:181:16: error: ‘hll_offset’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
181 | (*hll_offset)++;
| ~~~~~~~~~~~~~^~
../cil/src/cil_parser.c:222:11: note: ‘hll_offset’ was declared here
222 | uint32_t hll_offset = 1;
| ^~~~~~~~~~
In fact the warning is misleading because it comes from
prev_hll_offset not being initialized and gcc having trouble figuring
out it is initialized in calls to pop_hll_info.
To silence this warning, could you initialize prev_hll_offset to some value?
>
> cil_lexer_next(&tok);
> if (tok.type != SYMBOL) {
> @@ -115,19 +115,33 @@ static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno
> cil_log(CIL_ERR, "Line mark end without start\n");
> goto exit;
> }
> - pop_hll_info(stack, hll_lineno, hll_expand);
> + prev_hll_expand = *hll_expand;
> + pop_hll_info(stack, &prev_hll_offset, hll_expand);
> + if (*hll_expand) {
> + /* This is needed when exiting an lms section within an lmx section.
> + * In the lms section, hll_offset will increment and then revert
> + * back to its previous value when going back into the lmx section.
> + */
> + *hll_offset = prev_hll_offset;
> + }
> + if (prev_hll_expand && !*hll_expand) {
> + /* This is needed to count the lme at the end of an lmx section
> + * within an lms section (or within no hll section).
> + */
> + (*hll_offset)++;
> + }
> *current = (*current)->parent;
> } else {
> - push_hll_info(stack, *hll_lineno, *hll_expand);
> + push_hll_info(stack, *hll_offset, *hll_expand);
>
> - create_node(&node, *current, tok.line, *hll_lineno, NULL);
> + create_node(&node, *current, tok.line, *hll_offset, NULL);
> insert_node(node, *current);
> *current = node;
>
> - create_node(&node, *current, tok.line, *hll_lineno, CIL_KEY_SRC_INFO);
> + create_node(&node, *current, tok.line, *hll_offset, CIL_KEY_SRC_INFO);
> insert_node(node, *current);
>
> - create_node(&node, *current, tok.line, *hll_lineno, hll_type);
> + create_node(&node, *current, tok.line, *hll_offset, hll_type);
> insert_node(node, *current);
>
> cil_lexer_next(&tok);
> @@ -136,16 +150,9 @@ static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno
> goto exit;
> }
>
> - create_node(&node, *current, tok.line, *hll_lineno, cil_strpool_add(tok.value));
> + create_node(&node, *current, tok.line, *hll_offset, cil_strpool_add(tok.value));
> insert_node(node, *current);
>
> - rc = cil_string_to_uint32(tok.value, hll_lineno, 10);
> - if (rc != SEPOL_OK) {
> - goto exit;
> - }
> -
> - *hll_expand = (hll_type == CIL_KEY_SRC_HLL_LMX) ? 1 : 0;
> -
> cil_lexer_next(&tok);
> if (tok.type != SYMBOL && tok.type != QSTRING) {
> cil_log(CIL_ERR, "Invalid line mark syntax\n");
> @@ -157,8 +164,10 @@ static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno
> tok.value = tok.value+1;
> }
>
> - create_node(&node, *current, tok.line, *hll_lineno, cil_strpool_add(tok.value));
> + create_node(&node, *current, tok.line, *hll_offset, cil_strpool_add(tok.value));
> insert_node(node, *current);
> +
> + *hll_expand = (hll_type == CIL_KEY_SRC_HLL_LMX) ? 1 : 0;
> }
>
> cil_lexer_next(&tok);
> @@ -167,6 +176,11 @@ static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno
> goto exit;
> }
>
> + if (!*hll_expand) {
> + /* Need to increment because of the NEWLINE */
> + (*hll_offset)++;
> + }
> +
> return SEPOL_OK;
>
> exit:
> @@ -205,7 +219,7 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
> struct cil_tree_node *current = NULL;
> char *path = cil_strpool_add(_path);
> struct cil_stack *stack;
> - uint32_t hll_lineno = 0;
> + uint32_t hll_offset = 1;
> uint32_t hll_expand = 0;
> struct token tok;
> int rc = SEPOL_OK;
> @@ -223,7 +237,7 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
> cil_lexer_next(&tok);
> switch (tok.type) {
> case HLL_LINEMARK:
> - rc = add_hll_linemark(¤t, &hll_lineno, &hll_expand, stack, path);
> + rc = add_hll_linemark(¤t, &hll_offset, &hll_expand, stack, path);
> if (rc != SEPOL_OK) {
> goto exit;
> }
> @@ -234,7 +248,7 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
> cil_log(CIL_ERR, "Number of open parenthesis exceeds limit of %d at line %d of %s\n", CIL_PARSER_MAX_EXPR_DEPTH, tok.line, path);
> goto exit;
> }
> - create_node(&node, current, tok.line, hll_lineno, NULL);
> + create_node(&node, current, tok.line, hll_offset, NULL);
> insert_node(node, current);
> current = node;
> break;
> @@ -256,12 +270,12 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
> goto exit;
> }
>
> - create_node(&node, current, tok.line, hll_lineno, cil_strpool_add(tok.value));
> + create_node(&node, current, tok.line, hll_offset, cil_strpool_add(tok.value));
> insert_node(node, current);
> break;
> case NEWLINE :
> if (!hll_expand) {
> - hll_lineno++;
> + hll_offset++;
> }
> break;
> case COMMENT:
> @@ -269,7 +283,7 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
> cil_lexer_next(&tok);
> }
> if (!hll_expand) {
> - hll_lineno++;
> + hll_offset++;
> }
> if (tok.type != END_OF_FILE) {
> break;
> @@ -306,7 +320,7 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
>
> exit:
> while (!cil_stack_is_empty(stack)) {
> - pop_hll_info(stack, &hll_lineno, &hll_expand);
> + pop_hll_info(stack, &hll_offset, &hll_expand);
> }
> cil_lexer_destroy();
> cil_stack_destroy(&stack);
> diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c
> index 52b28999..4fdf339d 100644
> --- a/libsepol/cil/src/cil_tree.c
> +++ b/libsepol/cil/src/cil_tree.c
> @@ -50,10 +50,12 @@ __attribute__((noreturn)) __attribute__((format (printf, 1, 2))) void cil_tree_e
> exit(1);
> }
>
> -struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char **path, int* is_cil)
> +struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char **info_kind, uint32_t *hll_line, char **path)
> {
> + int rc;
> +
> if (!node) {
> - return NULL;
> + goto exit;
> }
>
> node = node->parent;
> @@ -62,16 +64,21 @@ struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char **
> if (node->flavor == CIL_NODE && node->data == NULL) {
> if (node->cl_head->data == CIL_KEY_SRC_INFO && node->cl_head->next != NULL && node->cl_head->next->next != NULL) {
> /* Parse Tree */
> - *path = node->cl_head->next->next->data;
> - *is_cil = (node->cl_head->next->data == CIL_KEY_SRC_CIL);
> + *info_kind = node->cl_head->next->data;
> + rc = cil_string_to_uint32(node->cl_head->next->next->data, hll_line, 10);
> + if (rc != SEPOL_OK) {
> + goto exit;
> + }
> + *path = node->cl_head->next->next->next->data;
> return node;
> }
> node = node->parent;
> } else if (node->flavor == CIL_SRC_INFO) {
> /* AST */
> struct cil_src_info *info = node->data;
> + *info_kind = info->kind;
> + *hll_line = info->hll_line;
> *path = info->path;
> - *is_cil = (info->kind == CIL_KEY_SRC_CIL);
> return node;
> } else {
> if (node->flavor == CIL_CALL) {
> @@ -86,17 +93,22 @@ struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char **
> }
> }
>
> +exit:
> + *info_kind = NULL;
> + *hll_line = 0;
> + *path = NULL;
> return NULL;
> }
>
> char *cil_tree_get_cil_path(struct cil_tree_node *node)
> {
> - char *path = NULL;
> - int is_cil;
> + char *info_kind;
> + uint32_t hll_line;
> + char *path;
>
> while (node) {
> - node = cil_tree_get_next_path(node, &path, &is_cil);
> - if (node && is_cil) {
> + node = cil_tree_get_next_path(node, &info_kind, &hll_line, &path);
> + if (node && info_kind == CIL_KEY_SRC_CIL) {
> return path;
> }
> }
> @@ -114,8 +126,7 @@ __attribute__((format (printf, 3, 4))) void cil_tree_log(struct cil_tree_node *n
>
> if (node) {
> char *path = NULL;
> - int is_cil;
> - unsigned hll_line = node->hll_line;
> + uint32_t hll_offset = node->hll_offset;
>
> path = cil_tree_get_cil_path(node);
>
> @@ -124,12 +135,20 @@ __attribute__((format (printf, 3, 4))) void cil_tree_log(struct cil_tree_node *n
> }
>
> while (node) {
> - node = cil_tree_get_next_path(node, &path, &is_cil);
> - if (node && !is_cil) {
> + do {
> + char *info_kind;
> + uint32_t hll_line;
> +
> + node = cil_tree_get_next_path(node, &info_kind, &hll_line, &path);
> + if (!node || info_kind == CIL_KEY_SRC_CIL) {
> + break;
> + }
> + if (info_kind == CIL_KEY_SRC_HLL_LMS) {
> + hll_line += hll_offset - node->hll_offset - 1;
> + }
> +
> cil_log(lvl," from %s:%d", path, hll_line);
While modifying this code, it would be nice if this "%d" was modified
to "%u", as hll_line should never be negative.
> - path = NULL;
> - hll_line = node->hll_line;
> - }
> + } while (1);
> }
> }
>
> @@ -222,7 +241,7 @@ void cil_tree_node_init(struct cil_tree_node **node)
> new_node->next = NULL;
> new_node->flavor = CIL_ROOT;
> new_node->line = 0;
> - new_node->hll_line = 0;
> + new_node->hll_offset = 0;
>
> *node = new_node;
> }
> diff --git a/libsepol/cil/src/cil_tree.h b/libsepol/cil/src/cil_tree.h
> index f4d22071..5a98da55 100644
> --- a/libsepol/cil/src/cil_tree.h
> +++ b/libsepol/cil/src/cil_tree.h
> @@ -46,11 +46,11 @@ struct cil_tree_node {
> struct cil_tree_node *next; //Each element in the list points to the next element
> enum cil_flavor flavor;
> uint32_t line;
> - uint32_t hll_line;
> + uint32_t hll_offset;
> void *data;
> };
>
> -struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char **path, int* is_cil);
> +struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char **info_kind, uint32_t *hll_line, char **path);
> char *cil_tree_get_cil_path(struct cil_tree_node *node);
> __attribute__((format (printf, 3, 4))) void cil_tree_log(struct cil_tree_node *node, enum cil_log_level lvl, const char* msg, ...);
>
> --
> 2.31.1
>
