Hi Stephen,
Thank you for pointing out the error. I removed free(s) from symtab_destroy in the new patch. I did not go further to pass in a destroy function to symtab_destroy and invoke hashtab_map() on the table... I think the tool maintainers or someone more familiar with the structures will be more suitable to handle that. I hope the my version of symtab_destroy cleans up memory allocated by symtab_init. I think it does. The followings are the places that call symtab_destroy:
o expand.c line 255 and 269: when symtab_destroy is called, there should be no hashtab_node struct created that can be referenced through new_common. A cleanup by hashtab_destroy() in symtab_destroy should be sufficient.
o link.c line 294 and 303: (similar to expand.c) no hashtab_node struct can be referenced through new_class.
Here is the updated patch for further review:
>From fa295645d43c1a6fc7afbe52e1624d72474e472f Mon Sep 17 00:00:00 2001
From: Alice Chu <alice.chu@xxxxxxxxxxxxxxx>
Date: Wed, 9 Jan 2013 18:34:32 -0800
Subject: [PATCH] Fix memory leak issues found by Klocwork
Change-Id: I019c6a3ba7e4d71cae2628f66c8526a1b2172079
---
include/sepol/policydb/symtab.h | 1 +
src/expand.c | 13 +++++++++++++
src/genusers.c | 11 ++++++++++-
src/hierarchy.c | 1 +
src/link.c | 7 ++++++-
src/policydb.c | 6 ++++--
src/policydb_convert.c | 1 +
src/services.c | 2 ++
src/symtab.c | 8 ++++++++
src/write.c | 1 +
10 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/include/sepol/policydb/symtab.h b/include/sepol/policydb/symtab.h
index c8ad664..490731b 100644
--- a/include/sepol/policydb/symtab.h
+++ b/include/sepol/policydb/symtab.h
@@ -32,6 +32,7 @@ typedef struct {
} symtab_t;
extern int symtab_init(symtab_t *, unsigned int size);
+extern void symtab_destroy(symtab_t *);
#endif /* _SYMTAB_H_ */
diff --git a/src/expand.c b/src/expand.c
index 2003eb6..cd137b2 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -251,6 +251,8 @@ static int common_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
new_id = strdup(id);
if (!new_id) {
ERR(state->handle, "Out of memory!");
+ /* free memory created by symtab_init first, then free new_common */
+ symtab_destroy(&new_common->permissions);
free(new_common);
return -1;
}
@@ -263,6 +265,8 @@ static int common_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
(hashtab_datum_t *) new_common);
if (ret) {
ERR(state->handle, "hashtab overflow");
+ /* free memory created by symtab_init first, then free new_common */
+ symtab_destroy(&new_common->permissions);
free(new_common);
free(new_id);
return -1;
@@ -812,6 +816,7 @@ static int role_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
new_id = strdup(id);
if (!new_id) {
ERR(state->handle, "Out of memory!");
+ free(new_role);
return -1;
}
@@ -963,6 +968,7 @@ static int user_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
new_id = strdup(id);
if (!new_id) {
ERR(state->handle, "Out of memory!");
+ free(new_user);
return -1;
}
ret = hashtab_insert(state->out->p_users.table,
@@ -1982,6 +1988,7 @@ static int cond_node_copy(expand_state_t * state, cond_node_t * cn)
if (cond_node_map_bools(state, tmp)) {
ERR(state->handle, "Error mapping booleans");
+ free(tmp);
return -1;
}
@@ -2189,6 +2196,7 @@ static int genfs_copy(expand_state_t * state)
newgenfs->fstype = strdup(genfs->fstype);
if (!newgenfs->fstype) {
ERR(state->handle, "Out of memory!");
+ free(newgenfs);
return -1;
}
@@ -2197,12 +2205,17 @@ static int genfs_copy(expand_state_t * state)
newc = malloc(sizeof(ocontext_t));
if (!newc) {
ERR(state->handle, "Out of memory!");
+ free(newgenfs->fstype);
+ free(newgenfs);
return -1;
}
memset(newc, 0, sizeof(ocontext_t));
newc->u.name = strdup(c->u.name);
if (!newc->u.name) {
ERR(state->handle, "Out of memory!");
+ free(newc);
+ free(newgenfs->fstype);
+ free(newgenfs);
return -1;
}
newc->v.sclass = c->v.sclass;
diff --git a/src/genusers.c b/src/genusers.c
index 37528e2..a31ea08 100644
--- a/src/genusers.c
+++ b/src/genusers.c
@@ -91,13 +91,20 @@ static int load_users(struct policydb *policydb, const char *path)
ebitmap_init(&usrdatum->roles.roles);
} else {
char *id = strdup(q);
+ if (!id) {
+ ERR(NULL, "out of memory");
+ free(buffer);
+ fclose(fp);
+ return -1;
+ }
/* Adding a new user definition. */
usrdatum =
(user_datum_t *) malloc(sizeof(user_datum_t));
- if (!id || !usrdatum) {
+ if (!usrdatum) {
ERR(NULL, "out of memory");
free(buffer);
+ free(id);
fclose(fp);
return -1;
}
@@ -108,6 +115,8 @@ static int load_users(struct policydb *policydb, const char *path)
id, (hashtab_datum_t) usrdatum)) {
ERR(NULL, "out of memory");
free(buffer);
+ free(id);
+ free(usrdatum);
fclose(fp);
return -1;
}
diff --git a/src/hierarchy.c b/src/hierarchy.c
index e2df5a4..d787a64 100644
--- a/src/hierarchy.c
+++ b/src/hierarchy.c
@@ -360,6 +360,7 @@ static int check_cond_avtab_hierarchy(cond_list_t * cond_list,
args->numerr++;
}
cond_av_list_destroy(expl);
+ avtab_destroy(&expa);
/*
* Check false condition
diff --git a/src/link.c b/src/link.c
index 01d3231..4d36132 100644
--- a/src/link.c
+++ b/src/link.c
@@ -291,6 +291,7 @@ static int class_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
}
new_id = strdup(id);
if (new_id == NULL) {
+ symtab_destroy(&new_class->permissions);
ERR(state->handle, "Memory error\n");
ret = SEPOL_ERR;
goto err;
@@ -299,6 +300,7 @@ static int class_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
(hashtab_key_t) new_id,
(hashtab_datum_t) new_class);
if (ret) {
+ symtab_destroy(&new_class->permissions);
ERR(state->handle,
"could not insert new class into symtab");
goto err;
@@ -1301,7 +1303,8 @@ static int copy_avrule_list(avrule_t * list, avrule_t ** dst,
if (new_rule->perms == NULL) {
new_rule->perms = new_perm;
} else {
- tail_perm->next = new_perm;
+ if (tail_perm) /* make sure no dereferencing of NULL pointer*/
+ tail_perm->next = new_perm;
}
tail_perm = new_perm;
cur_perm = cur_perm->next;
@@ -1765,6 +1768,7 @@ static int copy_avrule_block(link_state_t * state, policy_module_t * module,
new_decl->module_name = strdup(module->policy->name);
if (new_decl->module_name == NULL) {
ERR(state->handle, "Out of memory\n");
+ avrule_decl_destroy(new_decl);
ret = -1;
goto cleanup;
}
@@ -1784,6 +1788,7 @@ static int copy_avrule_block(link_state_t * state, policy_module_t * module,
ret = copy_avrule_decl(state, module, decl, new_decl);
if (ret) {
+ avrule_decl_destroy(new_decl);
goto cleanup;
}
diff --git a/src/policydb.c b/src/policydb.c
index ff292f6..fd9b57b 100644
--- a/src/policydb.c
+++ b/src/policydb.c
@@ -3448,7 +3448,8 @@ static int avrule_block_read(policydb_t * p,
if (curblock->branch_list == NULL) {
curblock->branch_list = curdecl;
} else {
- last_decl->next = curdecl;
+ if (last_decl != NULL)
+ last_decl->next = curdecl;
}
last_decl = curdecl;
num_decls--;
@@ -3457,7 +3458,8 @@ static int avrule_block_read(policydb_t * p,
if (*block == NULL) {
*block = curblock;
} else {
- last_block->next = curblock;
+ if (last_block != NULL)
+ last_block->next = curblock;
}
last_block = curblock;
diff --git a/src/policydb_convert.c b/src/policydb_convert.c
index 32832bb..3fc40cb 100644
--- a/src/policydb_convert.c
+++ b/src/policydb_convert.c
@@ -20,6 +20,7 @@ int policydb_from_image(sepol_handle_t * handle,
pf.handle = handle;
if (policydb_read(policydb, &pf, 0)) {
+ policydb_destroy(policydb);
ERR(handle, "policy image is invalid");
errno = EINVAL;
return STATUS_ERR;
diff --git a/src/services.c b/src/services.c
index 9c2920c..bed1e9b 100644
--- a/src/services.c
+++ b/src/services.c
@@ -96,6 +96,7 @@ int sepol_set_policydb_from_file(FILE * fp)
return -1;
}
if (policydb_read(&mypolicydb, &pf, 0)) {
+ policydb_destroy(&mypolicydb);
ERR(NULL, "can't read binary policy: %s", strerror(errno));
return -1;
}
@@ -1016,6 +1017,7 @@ int hidden sepol_load_policy(void *data, size_t len)
return -ENOMEM;
if (policydb_read(&newpolicydb, fp, 1)) {
+ policydb_destroy(&newpolicydb);
return -EINVAL;
}
diff --git a/src/symtab.c b/src/symtab.c
index b3a7aa8..b319c8f 100644
--- a/src/symtab.c
+++ b/src/symtab.c
@@ -46,4 +46,12 @@ int symtab_init(symtab_t * s, unsigned int size)
return 0;
}
+void symtab_destroy(symtab_t * s)
+{
+ if (!s)
+ return;
+ if (s->table)
+ hashtab_destroy(s->table);
+ return;
+}
/* FLASK */
diff --git a/src/write.c b/src/write.c
index 22e6143..ab1c257 100644
--- a/src/write.c
+++ b/src/write.c
@@ -1810,6 +1810,7 @@ static int scope_write(hashtab_key_t key, hashtab_datum_t datum, void *ptr)
buf[0] = cpu_to_le32(key_len);
if (put_entry(buf, sizeof(*buf), 1, fp) != 1 ||
put_entry(key, 1, key_len, fp) != key_len) {
+ free(dyn_buf);
return POLICYDB_ERROR;
}
buf[0] = cpu_to_le32(scope->scope);
--
1.7.0.4
Thank you very much,
Alice
________________________________________
From: Stephen Smalley [sds@xxxxxxxxxxxxx]
Sent: Tuesday, January 08, 2013 7:14 AM
To: Alice Chu
Cc: selinux@xxxxxxxxxxxxx; seandroid-list@xxxxxxxxxxxxx
Subject: Re: Fixing external/libsepol issues found by Klocwork
On 01/07/2013 08:51 PM, Alice Chu wrote:
> Hello,
>
> Attached you will find the Klocwork report on seandroid master branch external/libsepol. The following is the fix.
> Please review and give me your feedback.
>
> Thank you very much,
> Alice Chu
> ======================================================================================
>>From 1b6465ee62e9d5f1b5c7ef5c69b97bc572472f78 Mon Sep 17 00:00:00 2001
> From: Alice Chu <alice.chu@xxxxxxxxxxxxxxx>
> Date: Fri, 4 Jan 2013 15:51:37 -0800
> Subject: [PATCH] Fix issues found by Klocwork
>
> Change-Id: I055d851c95de6326a6833b02a40261f282847c7b
> ---
> include/sepol/policydb/symtab.h | 1 +
> src/expand.c | 13 +++++++++++++
> src/genusers.c | 11 ++++++++++-
> src/hierarchy.c | 1 +
> src/link.c | 7 ++++++-
> src/policydb.c | 6 ++++--
> src/policydb_convert.c | 1 +
> src/services.c | 2 ++
> src/symtab.c | 9 +++++++++
> src/write.c | 1 +
> 10 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/src/symtab.c b/src/symtab.c
> index b3a7aa8..87046b2 100644
> --- a/src/symtab.c
> +++ b/src/symtab.c
> @@ -46,4 +46,13 @@ int symtab_init(symtab_t * s, unsigned int size)
> return 0;
> }
>
> +void symtab_destroy(symtab_t * s)
> +{
> + if (!s)
> + return;
> + if (s->table)
> + hashtab_destroy(s->table);
> + free(s);
> + return;
> +}
> /* FLASK */
symtab_init() does not allocate s, so symtab_destroy() should not free
it. It isn't separately allocated from the containing structure. Also,
if you truly want to fully free the symtab, you'll need to pass in a
destroy function to symtab_destroy and invoke hashtab_map() on the table
with that destroy function - see for example symtabs_destroy(),
class_destroy(), etc in policydb.c.
--
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.
