Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220128202858.96935-1-vbendel@redhat.com> <20220128202858.96935-4-vbendel@redhat.com>
In-Reply-To: <20220128202858.96935-4-vbendel@redhat.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Tue, 1 Feb 2022 15:09:53 -0500
Message-ID: <CAHC9VhRXFf22+2wjxT6KAbVC2R6FVZ6m1i7EFGTbm1icWdr_vw@mail.gmail.com>
Subject: Re: [PATCH 3/3] selinux: remove duplicate cond_list clean up calls
To:     vbendel@redhat.com
Cc:     stephen.smalley.work@gmail.com, eparis@parisplace.org,
        omosnace@redhat.com, selinux@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jan 28, 2022 at 3:29 PM <vbendel@redhat.com> wrote:
> From: Vratislav Bendel <vbendel@redhat.com>
>
> 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@redhat.com>
> Signed-off-by: Vratislav Bendel <vbendel@redhat.com>
> ---
>  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