Fixing external/libsepol issues found by Klocwork

Fixing external/libsepol issues found by Klocwork

[Alice Chu]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]

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
======================================================================================

Re: Fixing external/libsepol issues found by Klocwork

[Stephen Smalley]

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@sta.samsung.com>
> 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@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: Fixing external/libsepol issues found by Klocwork

[Alice Chu]

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:

Re: Fixing external/libsepol issues found by Klocwork

[Eric Paris]

> From: Alice Chu <alice.chu@sta.samsung.com>
> Date: Wed, 9 Jan 2013 18:34:32 -0800
> Subject: [PATCH] Fix memory leak issues found by Klocwork

> diff --git a/src/link.c b/src/link.c
> index 01d3231..4d36132 100644
> --- a/src/link.c
> +++ b/src/link.c

> @@ -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;

I actually made this assert(tail_perm); If we did hit the case where
this was null, someone changed the code flow and broke it.  I don't
want to silently hide that fact.

> 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--;

Same

> @@ -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;

Similar, but not quite the same.  Since we rely on the caller setting
*block to NULL. I actually added 2 asserts.  One here on last_block
and one at the top on *block == NULL.

>
> 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;

No.  All of the callers seem to do init() and then destroy() on error.
 If we called destroy() as well it would be a double free.  Destroy
does not reset everything to nulls, it just frees the memory.


You should seem most of these patches in the next upstream push.  (Or
at least something like these fixes)

Thanks so much!

-Eric

--
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.