* [PATCH 3/6] libsepol: resource leaks in policydb.c
@ 2005-08-22 16:02 serue
2005-08-23 13:15 ` Stephen Smalley
0 siblings, 1 reply; 4+ messages in thread
From: serue @ 2005-08-22 16:02 UTC (permalink / raw)
To: SELinux
This patch fixes 2 resource leaks found by Coverity in policydb.c.
1. in avrule_read(), the 'cur' element has been malloc'ed
but not yet added to the avrule->perms list at one point
whene we jump to bad:
2. 'key' is not freed in one case in scope_read().
The second point makes me wonder - is scope_destroy() fully
implemented? It takes 'key' as an argument, but does nothing with
it. Same with its third arg. Of course, they both have attribute
unused...
Hmm, I see now my formatting is not matching what's in the original
file... I'll fix them by hand as I'm mailing them, and try to get
it right next time.
thanks,
-serge
Index: src/policydb.c
===================================================================
--- src.orig/policydb.c
+++ src/policydb.c
@@ -2027,8 +2027,10 @@ static avrule_t *avrule_read(policydb_t
class_perm_node_init(cur);
buf = next_entry(fp, sizeof(uint32_t) * 2);
- if (!buf)
+ if (!buf) {
+ free(cur);
goto bad;
+ }
cur->class = le32_to_cpu(buf[0]);
cur->data = le32_to_cpu(buf[1]);
@@ -2363,7 +2365,8 @@ static int scope_read(policydb_t * p, ha
if (strcmp(key, "object_r") == 0 && h == p->p_roles_scope.table) {
/* object_r was already added to this table in roles_init() */
- scope_destroy(key, scope, NULL);
+ free(key);
+ scope_destroy(NULL, scope, NULL);
}
else {
if (hashtab_insert(h, key, scope)) {
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 3/6] libsepol: resource leaks in policydb.c 2005-08-22 16:02 [PATCH 3/6] libsepol: resource leaks in policydb.c serue @ 2005-08-23 13:15 ` Stephen Smalley 2005-08-23 17:20 ` Joshua Brindle 0 siblings, 1 reply; 4+ messages in thread From: Stephen Smalley @ 2005-08-23 13:15 UTC (permalink / raw) To: serue; +Cc: Joshua Brindle, SELinux On Mon, 2005-08-22 at 11:02 -0500, serue@us.ibm.com wrote: > This patch fixes 2 resource leaks found by Coverity in policydb.c. > > 1. in avrule_read(), the 'cur' element has been malloc'ed > but not yet added to the avrule->perms list at one point > whene we jump to bad: > > 2. 'key' is not freed in one case in scope_read(). > > The second point makes me wonder - is scope_destroy() fully > implemented? It takes 'key' as an argument, but does nothing with > it. Same with its third arg. Of course, they both have attribute > unused... I'm merging this patch at present, but I think you are correct about scope_destroy being ambiguous in its usage - either it shouldn't take the key as an argument at all, or it should free it. Given that scope_destroy is also being used in a hashtab_map call by policydb_destroy to free the scope, I'd think we would want it to free the key. Joshua? -- Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3/6] libsepol: resource leaks in policydb.c 2005-08-23 13:15 ` Stephen Smalley @ 2005-08-23 17:20 ` Joshua Brindle 2005-08-25 17:02 ` Stephen Smalley 0 siblings, 1 reply; 4+ messages in thread From: Joshua Brindle @ 2005-08-23 17:20 UTC (permalink / raw) To: Stephen Smalley; +Cc: serue, SELinux Stephen Smalley wrote: >On Mon, 2005-08-22 at 11:02 -0500, serue@us.ibm.com wrote: > > >>This patch fixes 2 resource leaks found by Coverity in policydb.c. >> >> 1. in avrule_read(), the 'cur' element has been malloc'ed >> but not yet added to the avrule->perms list at one point >> whene we jump to bad: >> >> 2. 'key' is not freed in one case in scope_read(). >> >>The second point makes me wonder - is scope_destroy() fully >>implemented? It takes 'key' as an argument, but does nothing with >>it. Same with its third arg. Of course, they both have attribute >>unused... >> >> > >I'm merging this patch at present, but I think you are correct about >scope_destroy being ambiguous in its usage - either it shouldn't take >the key as an argument at all, or it should free it. Given that >scope_destroy is also being used in a hashtab_map call by >policydb_destroy to free the scope, I'd think we would want it to free >the key. Joshua? > > > You are right, I'm currently testing this patch but I think it's correct (this is before Serge's patch). Index: policydb.c =================================================================== RCS file: /cvsroot/selinux/nsa/selinux-usr/libsepol/src/policydb.c,v retrieving revision 1.19 diff -u -r1.19 policydb.c --- policydb.c 18 Aug 2005 20:42:25 -0000 1.19 +++ policydb.c 23 Aug 2005 15:04:09 -0000 @@ -970,8 +970,9 @@ } } -int scope_destroy(hashtab_key_t key __attribute__ ((unused)), hashtab_datum_t datum, void *p __attribute__ ((unused))) +int scope_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p __attribute__ ((unused))) { + free(key); scope_datum_t *cur = (scope_datum_t *) datum; if (cur != NULL) { free (cur->decl_ids); @@ -2374,8 +2375,7 @@ return 0; cleanup: - free(key); - scope_destroy(NULL, scope, NULL); + scope_destroy(key, scope, NULL); return -1; } @@ -2518,26 +2518,26 @@ if (p->policy_type == POLICY_MOD) { /* Get the module name and version */ if ((buf = next_entry(fp, sizeof(uint32_t))) == NULL) { - return -1; + goto bad; } len = le32_to_cpu(buf[0]); if ((buf = next_entry(fp, len)) == NULL) { - return -1; + goto bad; } if ((p->name = malloc(len + 1)) == NULL) { - return -1; + goto bad; } memcpy(p->name, buf, len); p->name[len] = '\0'; if ((buf = next_entry(fp, sizeof(uint32_t))) == NULL) { - return -1; + goto bad; } len = le32_to_cpu(buf[0]); if ((buf = next_entry(fp, len)) == NULL) { - return -1; + goto bad; } if ((p->version = malloc(len + 1)) == NULL) { - return -1; + goto bad; } memcpy(p->version, buf, len); p->version[len] = '\0'; @@ -2574,7 +2574,7 @@ free(p->global); p->global = NULL; if (avrule_block_read(p, &p->global, info->sym_num, fp) == -1) { - return -1; + goto bad; } for (i = 0; i < info->sym_num; i++) { if ((buf = next_entry(fp, sizeof(uint32_t))) == NULL) { -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3/6] libsepol: resource leaks in policydb.c 2005-08-23 17:20 ` Joshua Brindle @ 2005-08-25 17:02 ` Stephen Smalley 0 siblings, 0 replies; 4+ messages in thread From: Stephen Smalley @ 2005-08-25 17:02 UTC (permalink / raw) To: Joshua Brindle; +Cc: serue, SELinux On Tue, 2005-08-23 at 13:20 -0400, Joshua Brindle wrote: > Stephen Smalley wrote: > > >On Mon, 2005-08-22 at 11:02 -0500, serue@us.ibm.com wrote: > >>The second point makes me wonder - is scope_destroy() fully > >>implemented? It takes 'key' as an argument, but does nothing with > >>it. Same with its third arg. Of course, they both have attribute > >>unused... > >> > >> > > > >I'm merging this patch at present, but I think you are correct about > >scope_destroy being ambiguous in its usage - either it shouldn't take > >the key as an argument at all, or it should free it. Given that > >scope_destroy is also being used in a hashtab_map call by > >policydb_destroy to free the scope, I'd think we would want it to free > >the key. Joshua? > > > > > > > You are right, I'm currently testing this patch but I think it's > correct (this is before Serge's patch). Yielded a double free in /sbin/init, load_policy, and checkpolicy due to pointer aliasing in symtab_insert. Patch below should fix. Index: libsepol/src/util.c =================================================================== RCS file: /nfshome/pal/CVS/selinux-usr/libsepol/src/util.c,v retrieving revision 1.3 diff -u -p -r1.3 util.c --- libsepol/src/util.c 2 Aug 2005 18:32:15 -0000 1.3 +++ libsepol/src/util.c 25 Aug 2005 16:40:14 -0000 @@ -186,13 +186,18 @@ int symtab_insert(policydb_t *pol, uint3 * create it */ scope_datum = (scope_datum_t *) hashtab_search(pol->scope[sym].table, key); if (scope_datum == NULL) { + hashtab_key_t key2 = strdup((char*)key); + if (!key2) + return -ENOMEM; if ((scope_datum = malloc(sizeof(*scope_datum))) == NULL) { + free(key2); return -ENOMEM; } scope_datum->scope = scope; scope_datum->decl_ids = NULL; scope_datum->decl_ids_len = 0; - if ((rc = hashtab_insert(pol->scope[sym].table, key, scope_datum)) != 0) { + if ((rc = hashtab_insert(pol->scope[sym].table, key2, scope_datum)) != 0) { + free(key2); free(scope_datum); return rc; } -- Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2005-08-25 17:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2005-08-22 16:02 [PATCH 3/6] libsepol: resource leaks in policydb.c serue 2005-08-23 13:15 ` Stephen Smalley 2005-08-23 17:20 ` Joshua Brindle 2005-08-25 17:02 ` Stephen Smalley
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.