Re: [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan

[Nicolas Iooss <nicolas.iooss@m4x.org>]

On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libsepol/cil/src/cil_binary.c      | 12 ++++++++++++
>  libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++++
>  libsepol/cil/src/cil_symtab.c      |  1 +
>  libsepol/src/expand.c              |  3 +++
>  libsepol/src/kernel_to_cil.c       |  2 ++
>  libsepol/src/kernel_to_conf.c      |  2 ++
>  6 files changed, 30 insertions(+)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 0cc6eeb1..a645c95d 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
...
> @@ -4797,6 +4808,7 @@ static struct cil_list *cil_classperms_from_sepol(policydb_t *pdb, uint16_t clas
>         return cp_list;
>
>  exit:
> +       free(cp);
>         cil_log(CIL_ERR,"Failed to create CIL class-permissions from sepol values\n");
>         return NULL;
>  }

Before free(cp), should cp->perms be destroyed with a call to
cil_list_destroy(&cp->perms, CIL_FALSE)?

> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index fb9d9174..91187ed7 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -1522,6 +1522,7 @@ int cil_resolve_sidorder(struct cil_tree_node *current, void *extra_args)
>                 rc = cil_resolve_name(current, (char *)curr->data, CIL_SYM_SIDS, extra_args, &datum);
>                 if (rc != SEPOL_OK) {
>                         cil_log(CIL_ERR, "Failed to resolve sid %s in sidorder\n", (char *)curr->data);
> +                       free(new);
>                         goto exit;
>                 }
>                 cil_list_append(new, CIL_SID, datum);

Here, free(new) will not free the items that were already appended.
Would cil_list_destroy(&new, CIL_FALSE) work? (I have not tested it)

> @@ -1591,6 +1592,8 @@ int cil_resolve_catorder(struct cil_tree_node *current, void *extra_args)
>         return SEPOL_OK;
>
>  exit:
> +       if (new)
> +               free(new);
>         return rc;
>  }

Same comment

> @@ -1624,6 +1627,7 @@ int cil_resolve_sensitivityorder(struct cil_tree_node *current, void *extra_args
>         return SEPOL_OK;
>
>  exit:
> +       free(new);
>         return rc;
>  }

Why is there a "if(new)" in the previous chunk and not here? As
cil_list_init() fails hard when the memory allocation failed, new can
never be NULL so the previous if(new) is not needed.

All the other changes in this patch looked good to me.

Nicolas