Re: [PATCH 3/3] selinux: remove duplicate cond_list clean up calls

[Paul Moore]

On Fri, Jan 28, 2022 at 3:29 PM <vbendel@xxxxxxxxxx> wrote:
> From: Vratislav Bendel <vbendel@xxxxxxxxxx>
>
> On error path from cond_read_list() and duplicate_policydb_cond_list()
> the *_destroy() functions get called a second time in caller functions.
> Remove the first calls and let the callers clean it.
>
> Suggested-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> Signed-off-by: Vratislav Bendel <vbendel@xxxxxxxxxx>
> ---
>  security/selinux/ss/conditional.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 8bc16ad3af9e..c333daaeceab 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -432,19 +432,16 @@ int cond_read_list(struct policydb *p, void *fp)
>
>         rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
>         if (rc)
> -               goto err;
> +               return rc;
>
>         p->cond_list_len = len;
>
>         for (i = 0; i < len; i++) {
>                 rc = cond_read_node(p, &p->cond_list[i], fp);
>                 if (rc)
> -                       goto err;
> +                       return rc;
>         }
>         return 0;
> -err:
> -       cond_list_destroy(p);
> -       return rc;
>  }

I tend to prefer functions that cleanup their own allocations on
error.  It makes it easier and quicker to reason about a function's
error handling.  I recognize in this case it may mean multiple calls
to cond_list_destroy(), but that should be safe (considering the
previous patches in this series), and we are on the error path anyway
so I'm not as worried about a few extra instructions.

-- 
paul-moore.com