parse_sbat_var_data()/cleanup_sbat_var(): fix free logic

Valgrind was showing me a memory leak in the tests, and it's because of
several minor problems:
- the allocation isn't actually ever a list entry, because the entry
  array was before the linked list in the allocations
- the comparison for "first" when trying to free it is incorrect, so
  that was never getting set.
- we never free the test variable data that was parsed
- we're never calling cleanup_sbat_var() from several test cases.

This fixes these issues.

Before:
==2525955==
==2525955== HEAP SUMMARY:
==2525955==     in use at exit: 181 bytes in 3 blocks
==2525955==   total heap usage: 17 allocs, 14 frees, 2,310 bytes allocated
==2525955==
==2525955== 15 bytes in 1 blocks are definitely lost in loss record 1 of 3
==2525955==    at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2525955==    by 0x401D21: UnknownInlinedFun (test-sbat.c:937)
==2525955==    by 0x401D21: main (test-sbat.c:1043)
==2525955==
==2525955== 56 bytes in 1 blocks are definitely lost in loss record 2 of 3
==2525955==    at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2525955==    by 0x402ACB: parse_sbat_var_data (sbat.c:234)
==2525955==    by 0x40189D: UnknownInlinedFun (test-sbat.c:445)
==2525955==    by 0x40189D: main (test-sbat.c:1029)
==2525955==
==2525955== 110 bytes in 1 blocks are definitely lost in loss record 3 of 3
==2525955==    at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2525955==    by 0x402ACB: parse_sbat_var_data (sbat.c:234)
==2525955==    by 0x401D67: UnknownInlinedFun (test-sbat.c:943)
==2525955==    by 0x401D67: main (test-sbat.c:1043)
==2525955==
==2525955== LEAK SUMMARY:
==2525955==    definitely lost: 181 bytes in 3 blocks
==2525955==    indirectly lost: 0 bytes in 0 blocks
==2525955==      possibly lost: 0 bytes in 0 blocks
==2525955==    still reachable: 0 bytes in 0 blocks
==2525955==         suppressed: 0 bytes in 0 blocks
==2525955==
==2525955== For lists of detected and suppressed errors, rerun with: -s
==2525955== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

After:
==2591367== Memcheck, a memory error detector
==2591367== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2591367== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==2591367== Command: ./test-sbat
==2591367==
==2591367==
==2591367== HEAP SUMMARY:
==2591367==     in use at exit: 56 bytes in 1 blocks
==2591367==   total heap usage: 17 allocs, 16 frees, 5,382 bytes allocated
==2591367==
==2591367== 56 bytes in 1 blocks are definitely lost in loss record 1 of 1
==2591367==    at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2591367==    by 0x402AEB: parse_sbat_var_data (sbat.c:234)
==2591367==    by 0x40189D: UnknownInlinedFun (test-sbat.c:445)
==2591367==    by 0x40189D: main (test-sbat.c:1033)
==2591367==
==2591367== LEAK SUMMARY:
==2591367==    definitely lost: 56 bytes in 1 blocks
==2591367==    indirectly lost: 0 bytes in 0 blocks
==2591367==      possibly lost: 0 bytes in 0 blocks
==2591367==    still reachable: 0 bytes in 0 blocks
==2591367==         suppressed: 0 bytes in 0 blocks
==2591367==
==2591367== For lists of detected and suppressed errors, rerun with: -s
==2591367== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Signed-off-by: Peter Jones <pjones@redhat.com>

2 files changed, 10 insertions, 6 deletions

diff --git a/sbat.c b/sbat.c
index 5821475b..e102914c 100644
--- a/sbat.c
+++ b/sbat.c
@@ -139,7 +139,7 @@ cleanup_sbat_var(list_t *entries)
 	list_for_each_safe(pos, tmp, entries) {
 		entry = list_entry(pos, struct sbat_var_entry, list);
 
-		if ((uintptr_t)entry < (uintptr_t)first && entry != NULL)
+		if (first == NULL || (uintptr_t)entry < (uintptr_t)first)
 			first = entry;
 
 		list_del(&entry->list);
@@ -239,10 +239,10 @@ parse_sbat_var_data(list_t *entry_list, UINT8 *data, UINTN datasize)
 
 	INIT_LIST_HEAD(entry_list);
 
-	entries = (struct sbat_var_entry **)strtab;
-	strtab += sizeof(struct sbat_var_entry *) * n;
 	entry = (struct sbat_var_entry *)strtab;
 	strtab += sizeof(struct sbat_var_entry) * n;
+	entries = (struct sbat_var_entry **)strtab;
+	strtab += sizeof(struct sbat_var_entry *) * n;
 	n = 0;
 
 	list_for_each(pos, &csv) {
diff --git a/test-sbat.c b/test-sbat.c
index 8f361da6..e97c3d87 100644
--- a/test-sbat.c
+++ b/test-sbat.c
@@ -903,6 +903,7 @@ test_parse_and_verify(void)
 	struct sbat_section_entry *test_entries[] = {
 		&test_section_entry1, &test_section_entry2,
 	};
+	int rc = -1;
 
 	status = parse_sbat_section(sbat_section, sizeof(sbat_section)-1,
 	                            &n_section_entries, &section_entries);
@@ -941,16 +942,19 @@ test_parse_and_verify(void)
 
 	INIT_LIST_HEAD(&sbat_var);
 	status = parse_sbat_var_data(&sbat_var, sbat_var_alloced, sbat_var_data_size);
+	free(sbat_var_alloced);
 	if (status != EFI_SUCCESS || list_empty(&sbat_var))
 		return -1;
 
 	status = verify_sbat(n_section_entries, section_entries);
+	assert_equal_goto(status, EFI_SECURITY_VIOLATION, err, "expected %#x got %#x\n");
 
-	assert_equal_return(status, EFI_SECURITY_VIOLATION, -1, "expected %#x got %#x\n");
-	cleanup_sbat_var(&sbat_var);
+	rc = 0;
+err:
 	cleanup_sbat_section_entries(n_section_entries, section_entries);
+	cleanup_sbat_var(&sbat_var);
 
-	return 0;
+	return rc;
 }
 
 int