directory_iterator: fix wrong free, add tests

during one of the last refactorings of the heap constructors, there was a bug introduced that caused a free of the wrong pointer. This commit fixes that bug, makes the wrongly freed pointer const to yield compiler warnings on missuse, and adds a test to make sure it doesn't happen again.
Gottox · Aug 16, 2023 · 767629f · 767629f
1 parent 1eee394
commit 767629f
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 8 deletions.
diff --git a/include/sqsh_directory_private.h b/include/sqsh_directory_private.h
@@ -54,7 +54,7 @@ struct SqshDirectoryIterator {
 	/**
 	 * @privatesection
 	 */
-	struct SqshFile *file;
+	const struct SqshFile *file;
 	uint32_t remaining_size;
 
 	struct SqshMetablockReader metablock;
@@ -76,7 +76,7 @@ struct SqshDirectoryIterator {
  * @return 0 on success, a negative value on error.
  */
 SQSH_NO_EXPORT SQSH_NO_UNUSED int sqsh__directory_iterator_init(
-		struct SqshDirectoryIterator *iterator, struct SqshFile *inode);
+		struct SqshDirectoryIterator *iterator, const struct SqshFile *inode);
 
 /**
  * @internal

diff --git a/lib/directory/directory_iterator.c b/lib/directory/directory_iterator.c
@@ -54,7 +54,7 @@ static int
 load_metablock(
 		struct SqshDirectoryIterator *iterator, const uint64_t outer_offset,
 		uint32_t inner_offset) {
-	struct SqshFile *file = iterator->file;
+	const struct SqshFile *file = iterator->file;
 	struct SqshArchive *archive = file->archive;
 	const struct SqshSuperblock *superblock = sqsh_archive_superblock(archive);
 
@@ -94,7 +94,7 @@ directory_iterator_index_lookup(
 		const size_t name_len) {
 	int rv = 0;
 	struct SqshDirectoryIndexIterator index_iterator = {0};
-	struct SqshFile *file = iterator->file;
+	const struct SqshFile *file = iterator->file;
 	const uint64_t inode_ref = sqsh_file_inode_ref(file);
 	uint64_t outer_offset = sqsh_file_directory_block_start(file);
 	uint32_t inner_offset = sqsh_file_directory_block_offset(file);
@@ -175,7 +175,7 @@ sqsh_directory_iterator_lookup(
 
 int
 sqsh__directory_iterator_init(
-		struct SqshDirectoryIterator *iterator, struct SqshFile *file) {
+		struct SqshDirectoryIterator *iterator, const struct SqshFile *file) {
 	int rv = 0;
 
 	if (sqsh_file_type(file) != SQSH_FILE_TYPE_DIRECTORY) {
@@ -205,8 +205,8 @@ sqsh_directory_iterator_new(struct SqshFile *file, int *err) {
 	}
 	rv = sqsh__directory_iterator_init(iterator, file);
 	if (rv < 0) {
-		free(file);
-		file = NULL;
+		free(iterator);
+		iterator = NULL;
 	}
 out:
 	if (err != NULL) {

diff --git a/test/directory/directory_iterator.c b/test/directory/directory_iterator.c
@@ -124,7 +124,7 @@ iter_two_files(void) {
 			[INODE_TABLE_OFFSET] = METABLOCK_HEADER(0, 1024),
 			INODE_HEADER(1, 0, 0, 0, 0, 1),
 			INODE_BASIC_DIR(0, 1024, 0, 0),
-			[INODE_TABLE_OFFSET+2+128] = 
+			[INODE_TABLE_OFFSET+2+128] =
 			INODE_HEADER(3, 0, 0, 0, 0, 2),
 			INODE_BASIC_SYMLINK(3),
 			't', 'g', 't',
@@ -171,8 +171,41 @@ iter_two_files(void) {
 	sqsh__archive_cleanup(&archive);
 }
 
+static void
+iter_invalid_file_type(void) {
+	int rv;
+	struct SqshArchive archive = {0};
+	uint8_t payload[] = {
+			/* clang-format off */
+			SQSH_HEADER,
+			/* inode */
+			[INODE_TABLE_OFFSET] = METABLOCK_HEADER(0, 1024),
+			INODE_HEADER(3, 0, 0, 0, 0, 1),
+			INODE_BASIC_SYMLINK(3),
+			't', 'g', 't',
+
+			[FRAGMENT_TABLE_OFFSET] = 0,
+			/* clang-format on */
+	};
+	mk_stub(&archive, payload, sizeof(payload));
+
+	struct SqshFile file = {0};
+	rv = sqsh__file_init(&file, &archive, 0);
+	assert(rv == 0);
+
+	struct SqshDirectoryIterator *iter =
+			sqsh_directory_iterator_new(&file, &rv);
+	assert(rv == -SQSH_ERROR_NOT_A_DIRECTORY);
+	assert(iter == NULL);
+
+	sqsh_directory_iterator_free(iter);
+	sqsh__file_cleanup(&file);
+	sqsh__archive_cleanup(&archive);
+}
+
 DECLARE_TESTS
 TEST(iter_two_files)
 TEST(iter_invalid_file_name_with_slash)
 TEST(iter_invalid_file_name_with_0)
+TEST(iter_invalid_file_type)
 END_TESTS