[apparmor] [PATCH 2/3] parser: Never leave entries list in a bad state

[Tyler Hicks]

When merging file entries in process_file_entries(), an error condition
can leave the entries list in a bad state which can cause invalid reads
and/or double frees when freeing the codomain and entries list memory.

The problem comes from the need to sort the entries linked list. An
array of pointers is created to represent the linked list, then the
array is sorted, then the linked list and the array coexist while the
entries are merged, then the linked list is reconstructed and the array
is freed. While the entries are being merged, an error condition can
occur and the function can return while the linked list is partially
modified.

The solution is to complete the sorting, reconstruct the linked list,
and free the array immediately. Once the linked list is in a good state,
the entries can be merged. Care is taken to adjust the linked list
pointers as entries are merged. An error condition can occur but the
linked list is always in a good state and proper cleanup can be
performed without any memory access issues.

Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
---
 parser/parser_merge.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/parser/parser_merge.c b/parser/parser_merge.c
index 04ff6df..bada358 100644
--- a/parser/parser_merge.c
+++ b/parser/parser_merge.c
@@ -101,44 +101,34 @@ static int process_file_entries(struct codomain *cod)
 
 	qsort(table, count, sizeof(struct cod_entry *), file_comp);
 	table[count] = NULL;
-
+	for (n = 0; n < count; n++)
+		table[n]->next = table[n + 1];
+	cod->entries = table[0];
+	free(table);
 
 	/* walk the sorted table merging similar entries */
-	for (cur = table[0], next = table[1], n = 1; next != NULL; n++, next = table[n]) {
+	for (cur = cod->entries, next = cur->next; next != NULL; next = cur->next) {
 		if (file_comp(&cur, &next) == 0) {
 			/* check for merged x consistency */
 			if (!is_merged_x_consistent(cur->mode, next->mode)) {
 				PERROR(_("profile %s: has merged rule %s with "
 					 "conflicting x modifiers\n"),
 					 cod->name, cur->name);
-				free(table);
 				return 0;
 			}
 //if (next->audit)
 //fprintf(stderr, "warning: merging rule 0x%x %s\n", next->audit, next->name);
 			cur->mode |= next->mode;
 			cur->audit |= next->audit;
+			cur->next = next->next;
+
 			next->next = NULL;
 			free_cod_entries(next);
-			table[n] = NULL;
 		} else {
 			cur = next;
 		}
 	}
 
-	/* rebuild the file_entry chain */
-	cur = table[0];
-	for (n = 1; n < count; n++) {
-		if (table[n] != NULL) {
-			cur->next = table[n];
-			cur = table[n];
-		}
-	}
-	cur->next = NULL;
-	cod->entries = table[0];
-
-	free(table);
-
 	return 1;
 }
 
-- 
1.8.3.2