2022-01-31 11:35:07

by Vratislav Bendel

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

From: Vratislav Bendel <[email protected]>

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 <[email protected]>
Signed-off-by: Vratislav Bendel <[email protected]>
---
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;
}

int cond_write_bool(void *vkey, void *datum, void *ptr)
@@ -643,7 +640,7 @@ static int duplicate_policydb_cond_list(struct policydb *newp,
sizeof(*newp->cond_list),
GFP_KERNEL);
if (!newp->cond_list)
- goto error;
+ return -ENOMEM;

for (i = 0; i < origp->cond_list_len; i++) {
struct cond_node *newn = &newp->cond_list[i];
@@ -656,27 +653,22 @@ static int duplicate_policydb_cond_list(struct policydb *newp,
orign->expr.len * sizeof(*orign->expr.nodes),
GFP_KERNEL);
if (!newn->expr.nodes)
- goto error;
+ return -ENOMEM;

newn->expr.len = orign->expr.len;

rc = cond_dup_av_list(&newn->true_list, &orign->true_list,
&newp->te_cond_avtab);
if (rc)
- goto error;
+ return rc;

rc = cond_dup_av_list(&newn->false_list, &orign->false_list,
&newp->te_cond_avtab);
if (rc)
- goto error;
+ return rc;
}

return 0;
-
-error:
- avtab_destroy(&newp->te_cond_avtab);
- cond_list_destroy(newp);
- return -ENOMEM;
}

static int cond_bools_destroy(void *key, void *datum, void *args)
--
2.26.3

2022-02-03 00:12:17

by Paul Moore

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

On Fri, Jan 28, 2022 at 3:29 PM <[email protected]> wrote:
> From: Vratislav Bendel <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Vratislav Bendel <[email protected]>
> ---
> 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