diff mbox series

[7/8] libsepol/cil: Report correct high-level language line numbers

Message ID 20210810180537.669439-8-jwcart2@gmail.com (mailing list archive)
State Superseded
Headers show
Series libsepol/cil: Line mark cleanup and fix | expand

Commit Message

James Carter Aug. 10, 2021, 6:05 p.m. UTC
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@gmail.com>
---
 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(-)

Comments

Nicolas Iooss Aug. 16, 2021, 9:23 a.m. UTC | #1
On Tue, Aug 10, 2021 at 8:22 PM James Carter <jwcart2@gmail.com> 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@gmail.com>
> ---
>  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(&current, &hll_lineno, &hll_expand, stack, path);
> +                       rc = add_hll_linemark(&current, &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
>
James Carter Aug. 16, 2021, 2:40 p.m. UTC | #2
On Mon, Aug 16, 2021 at 5:23 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Tue, Aug 10, 2021 at 8:22 PM James Carter <jwcart2@gmail.com> 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@gmail.com>
> > ---
> >  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?
>

Yes, I will do that.
Thanks,
Jim

> >
> >         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(&current, &hll_lineno, &hll_expand, stack, path);
> > +                       rc = add_hll_linemark(&current, &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
> >
>
diff mbox series

Patch

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;
 
 	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(&current, &hll_lineno, &hll_expand, stack, path);
+			rc = add_hll_linemark(&current, &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);
-				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, ...);