On 1/31/19 4:33 PM, Nicolas Iooss wrote:
On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@xxxxxxxxxx> wrote:
Signed-off-by: Petr Lautrbach <plautrba@xxxxxxxxxx>
---
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@xxxxxxxxxxxxx>
National Security Agency
