Re: [Non-DoD Source] Re: [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan

[jwcart2 <jwcart2@tycho.nsa.gov>]

On 1/31/19 4:33 PM, Nicolas Iooss wrote:
> On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>>   libsepol/cil/src/cil_binary.c      | 12 ++++++++++++
>>   libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++++
>>   libsepol/cil/src/cil_symtab.c      |  1 +
>>   libsepol/src/expand.c              |  3 +++
>>   libsepol/src/kernel_to_cil.c       |  2 ++
>>   libsepol/src/kernel_to_conf.c      |  2 ++
>>   6 files changed, 30 insertions(+)
>>
>> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
>> index 0cc6eeb1..a645c95d 100644
>> --- a/libsepol/cil/src/cil_binary.c
>> +++ b/libsepol/cil/src/cil_binary.c
> ...
>> @@ -4797,6 +4808,7 @@ static struct cil_list *cil_classperms_from_sepol(policydb_t *pdb, uint16_t clas
>>          return cp_list;
>>
>>   exit:
>> +       free(cp);
>>          cil_log(CIL_ERR,"Failed to create CIL class-permissions from sepol values\n");
>>          return NULL;
>>   }
> 
> Before free(cp), should cp->perms be destroyed with a call to
> cil_list_destroy(&cp->perms, CIL_FALSE)?

Instead of "free(cp);" use "cil_destroy_classperms(cp);" That will destroy the 
permissions list as well.

> 
>> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
>> index fb9d9174..91187ed7 100644
>> --- a/libsepol/cil/src/cil_resolve_ast.c
>> +++ b/libsepol/cil/src/cil_resolve_ast.c
>> @@ -1522,6 +1522,7 @@ int cil_resolve_sidorder(struct cil_tree_node *current, void *extra_args)
>>                  rc = cil_resolve_name(current, (char *)curr->data, CIL_SYM_SIDS, extra_args, &datum);
>>                  if (rc != SEPOL_OK) {
>>                          cil_log(CIL_ERR, "Failed to resolve sid %s in sidorder\n", (char *)curr->data);
>> +                       free(new);
>>                          goto exit;
>>                  }
>>                  cil_list_append(new, CIL_SID, datum);
> 
> Here, free(new) will not free the items that were already appended.
> Would cil_list_destroy(&new, CIL_FALSE) work? (I have not tested it)
> 

Yes, "cil_list_destroy(&new, CIL_FALSE)" is what is needed here. Using CIL_FALSE 
means that the datums will not be destroyed which is what we would want.

>> @@ -1591,6 +1592,8 @@ int cil_resolve_catorder(struct cil_tree_node *current, void *extra_args)
>>          return SEPOL_OK;
>>
>>   exit:
>> +       if (new)
>> +               free(new);
>>          return rc;
>>   }
> 
> Same comment

Should use "cil_list_destroy(&new, CIL_FALSE)" here as well.
The "if (new)" is not needed since cil_list_destroy() will return if new is NULL.
> 
>> @@ -1624,6 +1627,7 @@ int cil_resolve_sensitivityorder(struct cil_tree_node *current, void *extra_args
>>          return SEPOL_OK;
>>
>>   exit:
>> +       free(new);
>>          return rc;
>>   }
> 
> Why is there a "if(new)" in the previous chunk and not here? As
> cil_list_init() fails hard when the memory allocation failed, new can
> never be NULL so the previous if(new) is not needed.
> 
> All the other changes in this patch looked good to me.
> 
Should use "cil_list_destroy(&new, CIL_FALSE)" here as well.

> Nicolas
> 
> 


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency