Hi Stephen,
Thanks for pointing out the error. Here is the change (with a more specific commit comment).
>From 0d73ef7049feee794f14cf1af88d05dae8139914 Mon Sep 17 00:00:00 2001
From: Alice Chu <alice.chu@xxxxxxxxxxxxxxx>
Date: Tue, 8 Jan 2013 16:05:25 -0800
Subject: [PATCH] Free allocated memory when clean up / exit.
Change-Id: I116a3e1fa17f357b10e7414efde6c78ffb9678df
---
policy_define.c | 25 ++++++++++++++++++++-----
1 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/policy_define.c b/policy_define.c
index 2c12447..8af6141 100644
--- a/policy_define.c
+++ b/policy_define.c
@@ -1497,12 +1497,12 @@ int define_compute_type_helper(int which, avrule_t ** rule)
while ((id = queue_remove(id_queue))) {
if (set_types(&avrule->stypes, id, &add, 0))
- return -1;
+ goto bad;
}
add = 1;
while ((id = queue_remove(id_queue))) {
if (set_types(&avrule->ttypes, id, &add, 0))
- return -1;
+ goto bad;
}
ebitmap_init(&tclasses);
@@ -1531,7 +1531,7 @@ int define_compute_type_helper(int which, avrule_t ** rule)
perm = malloc(sizeof(class_perm_node_t));
if (!perm) {
yyerror("out of memory");
- return -1;
+ goto bad;
}
class_perm_node_init(perm);
perm->class = i + 1;
@@ -2050,10 +2050,12 @@ role_datum_t *merge_roles_dom(role_datum_t * r1, role_datum_t * r2)
new->s.value = 0; /* temporary role */
if (ebitmap_or(&new->dominates, &r1->dominates, &r2->dominates)) {
yyerror("out of memory");
+ free(new);
return NULL;
}
if (ebitmap_or(&new->types.types, &r1->types.types, &r2->types.types)) {
yyerror("out of memory");
+ free(new);
return NULL;
}
if (!r1->s.value) {
@@ -2458,13 +2460,17 @@ int define_role_allow(void)
role_allow_rule_init(ra);
while ((id = queue_remove(id_queue))) {
- if (set_roles(&ra->roles, id))
+ if (set_roles(&ra->roles, id)) {
+ free(ra);
return -1;
+ }
}
while ((id = queue_remove(id_queue))) {
- if (set_roles(&ra->new_roles, id))
+ if (set_roles(&ra->new_roles, id)) {
+ free(ra);
return -1;
+ }
}
append_role_allow(ra);
@@ -2777,6 +2783,7 @@ int define_constraint(constraint_expr_t * expr)
}
if (!node->expr) {
yyerror("out of memory");
+ free(node);
return -1;
}
node->permissions = 0;
@@ -3281,6 +3288,7 @@ cond_expr_t *define_cond_expr(uint32_t expr_type, void *arg1, void *arg2)
return expr;
default:
yyerror("illegal conditional expression");
+ free(expr);
return NULL;
}
}
@@ -3582,6 +3590,12 @@ static int parse_security_context(context_struct_t * c)
return 0;
}
+ /* check context c to make sure ok to dereference c later */
+ if (c == NULL) {
+ yyerror("null context pointer!");
+ return -1;
+ }
+
context_init(c);
/* extract the user */
@@ -4673,6 +4687,7 @@ int define_range_trans(int class_specified)
out:
range_trans_rule_destroy(rule);
+ free(rule);
return -1;
}
--
1.7.0.4
Thanks,
Alice
________________________________________
From: Stephen Smalley [sds@xxxxxxxxxxxxx]
Sent: Tuesday, January 08, 2013 7:07 AM
To: Alice Chu
Cc: selinux@xxxxxxxxxxxxx; seandroid-list@xxxxxxxxxxxxx
Subject: Re: Fixing external/checkpolicy issues found by Klocwork
On 01/07/2013 08:29 PM, Alice Chu wrote:
> Hello,
>
> Attached you will find the Klocwork report on seandroid master branch external/checkpolicy. The following is the fix for issues found in policy_define.c.
> Please review and give me your feedback.
>
> Thank you very much,
> Alice Chu
>
> ============================================================================
>>From 18555451c5831fd95044e665d3dc514eb69e3b75 Mon Sep 17 00:00:00 2001
> From: Alice Chu <alice.chu@xxxxxxxxxxxxxxx>
> Date: Mon, 7 Jan 2013 15:29:29 -0800
> Subject: [PATCH] Fix issues found by Klocwork
>
> Change-Id: Ic3a01364b6855529f6b58a8820c6011a22c21841
> ---
> policy_define.c | 24 +++++++++++++++++++-----
> 1 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/policy_define.c b/policy_define.c
> index 2c12447..504af69 100644
> --- a/policy_define.c
> +++ b/policy_define.c
> @@ -3583,6 +3591,11 @@ static int parse_security_context(context_struct_t * c)
> }
>
> context_init(c);
> + /* check context c to make sure ok to dereference c later */
> + if (c == NULL) {
> + yyerror("null context pointer!");
> + goto bad;
> + }
>
> /* extract the user */
> id = queue_remove(id_queue);
I think you want this check before context_init(), as it dereferences c.
And then just return -1 in the error path.
This btw is an illegal state as NULL should only be passed if pass == 1.
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
