Close the file and parent on every failed open

contrib/esp32p4/chd/chd_esp_vfs.c

@@ -65,12 +65,11 @@ chd_error chd_esp_vfs_open(const char *path, chd_file **chd)
 	if (!f)
 		return CHDERR_FILE_NOT_FOUND;
 
+	/* on failure libchdr has already closed f through the callback; closing
+	 * it again here would be a double fclose() */
 	err = chd_open_core_file_callbacks(&chd_esp_vfs_callbacks, f, CHD_OPEN_READ, NULL, chd);
 	if (err != CHDERR_NONE)
-	{
-		fclose(f);
 		return err;
-	}
 
 	/* Compressed hunks are small - a few KB - and laid out strictly
 	 * sequentially, so one larger read serves many of them and the fixed cost

contrib/esp32p4/idf-benchmark/main/benchmark_main.c

@@ -778,10 +778,8 @@ static run_result run_one(const char *name, const core_file_callbacks *cb, void
 	err = chd_open_core_file_callbacks(cb, argp, CHD_OPEN_READ, NULL, &chd);
 	if (err != CHDERR_NONE) {
 		printf("%-48s OPEN FAILED: %s\n", name, chd_error_string(err));
-		/* chd_open_core_file_callbacks()'s cleanup: path calls chd_close()
-		 * (hence core_fclose(argp)) on every failure past its first,
-		 * near-unfailable malloc(sizeof(chd_file)) - i.e. argp is already
-		 * closed here in every failure mode this benchmark actually hits.
+		/* chd_open_core_file_callbacks() closes argp through the callback
+		 * on every failure, so it is already closed here.
 		 * An extra cb->fclose(argp) here is a double-close: harmless on the
 		 * flash path (mem_fclose() is a no-op) but a real double-free of
 		 * the FATFS file object on the SD path, which corrupts a FreeRTOS

contrib/tangcore-bl616/chd/chd_fatfs.c

@@ -75,12 +75,10 @@ chd_error chd_fatfs_open(const char *path, FIL *fil, chd_file **chd)
 	if (f_open(fil, path, FA_READ) != FR_OK)
 		return CHDERR_FILE_NOT_FOUND;
 
+	/* on failure libchdr has already closed fil through the callback */
 	err = chd_open_core_file_callbacks(&chd_fatfs_callbacks, fil, CHD_OPEN_READ, NULL, chd);
 	if (err != CHDERR_NONE)
-	{
-		f_close(fil);
 		return err;
-	}
 
 	/* Compressed hunks are small - a few KB - and laid out strictly
 	 * sequentially, so one larger read serves many of them and the fixed cost

include/libchdr/chd.h

@@ -379,7 +379,19 @@ typedef chd_error (*chd_codec_interface_decompress)(void *codec, const uint8_t *
 /* same as chd_create(), but accepts an already-opened core_file object */
 /* chd_error chd_create_file(core_file *file, uint64_t logicalbytes, uint32_t hunkbytes, uint32_t compression, chd_file *parent); */
 
-/* open an existing CHD file */
+/* open an existing CHD file
+ *
+ * The file handle and the parent are handed over to libchdr by the call,
+ * whether it succeeds or not. On success, chd_close() closes both; on
+ * failure, they have already been closed - through the fclose callback, or
+ * chd_close() for the parent - and must not be closed again. To retry a child
+ * that reported CHDERR_REQUIRES_PARENT, open its file again. The one exception
+ * is chd_open_core_file_callbacks() with NULL callbacks, which has nothing to
+ * close the file with and leaves it to the caller.
+ *
+ * chd_open_file() never closes the FILE it is given, on success or failure:
+ * its fclose callback does nothing. It does take the parent, like the others.
+ * chd_open() closes the file it opened. */
 CHD_EXPORT chd_error chd_open_core_file_callbacks(const core_file_callbacks *callbacks, const void *user_data, int mode, chd_file *parent, chd_file **chd);
 CHD_EXPORT chd_error chd_open_core_file(core_file *file, int mode, chd_file *parent, chd_file **chd); /* Legacy; use chd_open_core_file_callbacks instead! */
 CHD_EXPORT chd_error chd_open_file(FILE *file, int mode, chd_file *parent, chd_file **chd);

src/libchdr_chd.c

@@ -1999,18 +1999,32 @@ CHD_EXPORT chd_error chd_open_core_file_callbacks(const core_file_callbacks *cal
 	chd_file *newchd = NULL;
 	chd_error err;
 
-	/* verify parameters */
+	/* The file and the parent belong to us from here on, whether the open
+	 * succeeds or not: every failure closes both, the way chd_close() of the
+	 * new handle would. Callers depend on that - they retry a child that
+	 * reported CHDERR_REQUIRES_PARENT with a fresh file, and never close the
+	 * parent they passed in. The exits before newchd exists have to do it by
+	 * hand. */
+
+	/* verify parameters; with no callbacks there is nothing to close it with */
 	if (callbacks == NULL)
-		EARLY_EXIT(err = CHDERR_INVALID_PARAMETER);
+		return CHDERR_INVALID_PARAMETER;
 
-	/* punt if invalid parent */
+	/* punt if invalid parent - which is not a chd_file, so not ours to close */
 	if (parent != NULL && parent->cookie != COOKIE_VALUE)
-		EARLY_EXIT(err = CHDERR_INVALID_PARAMETER);
+	{
+		callbacks->fclose((void *)user_data);
+		return CHDERR_INVALID_PARAMETER;
+	}
 
 	/* allocate memory for the final result */
 	newchd = (chd_file *)malloc(sizeof(**chd));
 	if (newchd == NULL)
-		EARLY_EXIT(err = CHDERR_OUT_OF_MEMORY);
+	{
+		callbacks->fclose((void *)user_data);
+		chd_close(parent);
+		return CHDERR_OUT_OF_MEMORY;
+	}
 	memset(newchd, 0, sizeof(*newchd));
 	newchd->cookie = COOKIE_VALUE;
 	newchd->parent = parent;
@@ -2231,8 +2245,8 @@ CHD_EXPORT chd_error chd_open_core_file_callbacks(const core_file_callbacks *cal
 	return CHDERR_NONE;
 
 cleanup:
-	if (newchd != NULL)
-		chd_close(newchd);
+	/* closes the file and the parent along with it */
+	chd_close(newchd);
 	return err;
 }
 

tests/CMakeLists.txt

@@ -61,6 +61,13 @@ chdr_test_defines(chdr-sparse-map)
 add_test(NAME sparse-map
          COMMAND chdr-sparse-map "${CMAKE_CURRENT_SOURCE_DIR}/corpus/seeds/rle_sparse.chd")
 
+# Who closes the file and the parent when an open fails. Builds its images in
+# memory, so it runs without the generated corpus.
+add_executable(chdr-open-ownership open_ownership.c)
+target_link_libraries(chdr-open-ownership PRIVATE chdr-static)
+chdr_test_defines(chdr-open-ownership)
+add_test(NAME open-ownership COMMAND chdr-open-ownership)
+
 # CHDs with a parent: seeds/ has none, and cannot (a child needs its parent, and
 # the workflows there open every file standalone), so COMPRESSION_PARENT would
 # otherwise go untested - including through the in-place CD spread, where a

tests/open_ownership.c

@@ -0,0 +1,228 @@
+/* license:BSD-3-Clause
+ * copyright-holders:Romain Tisserand
+ *
+ * Who closes what when an open fails.
+ *
+ * The file and the parent handed to chd_open_core_file_callbacks() are
+ * libchdr's from the call on, success or failure: a failed open has already
+ * closed both. Callers are built on that - they retry a child that reported
+ * CHDERR_REQUIRES_PARENT with a fresh file, and never close the parent they
+ * passed in. A caller that closed the file again after a failure would close
+ * it twice; one that kept using it would use a closed handle.
+ *
+ * The images are built in memory - a one-hunk uncompressed CHDv5, with or
+ * without a parent SHA1 - so this runs without the generated corpus.
+ *
+ * chd_open() opens its FILE itself, so the test cannot count its closes.
+ * On POSIX it checks instead that the descriptor came back: open() always
+ * returns the lowest free one, so a FILE the failed open kept would push the
+ * next open to a higher number.
+ */
+#if defined(__unix__) || defined(__APPLE__)
+#define _POSIX_C_SOURCE 200112L   /* fileno() under a strict -std= */
+#define HAVE_FILENO 1
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <libchdr/chd.h>
+
+#define HUNK 512
+#define IMAGE (2 * HUNK)
+#define SCRATCH_NAME "open_ownership_child.chd"
+
+typedef struct {
+	uint8_t data[IMAGE];
+	size_t size;
+	size_t pos;
+	int closes;
+} memfile;
+
+static uint64_t mem_fsize(void *argp)
+{
+	return ((memfile *)argp)->size;
+}
+
+static size_t mem_fread(void *ptr, size_t size, size_t count, void *argp)
+{
+	memfile *f = (memfile *)argp;
+	size_t want = size * count;
+	if (f->closes != 0)
+		return 0;   /* reading a closed file: the test below will notice */
+	if (f->pos >= f->size)
+		return 0;
+	if (want > f->size - f->pos)
+		want = f->size - f->pos;
+	memcpy(ptr, f->data + f->pos, want);
+	f->pos += want;
+	return size ? want / size : 0;
+}
+
+static int mem_fclose(void *argp)
+{
+	((memfile *)argp)->closes++;
+	return 0;
+}
+
+static int mem_fseek(void *argp, int64_t offset, int whence)
+{
+	memfile *f = (memfile *)argp;
+	int64_t origin = (whence == SEEK_SET) ? 0 : (whence == SEEK_CUR) ? (int64_t)f->pos : (int64_t)f->size;
+	if (origin + offset < 0)
+		return -1;
+	f->pos = (size_t)(origin + offset);
+	return 0;
+}
+
+static const core_file_callbacks mem_callbacks = {
+	mem_fsize, mem_fread, mem_fclose, mem_fseek
+};
+
+static void put_be32(uint8_t *p, uint32_t v)
+{
+	p[0] = (uint8_t)(v >> 24); p[1] = (uint8_t)(v >> 16);
+	p[2] = (uint8_t)(v >> 8);  p[3] = (uint8_t)v;
+}
+
+/* One uncompressed hunk: 124-byte v5 header, a 4-byte map entry right after
+ * it pointing at hunk slot 1, the data in that slot. sha1 names the image;
+ * a nonzero parentsha1 makes it a child of the image with that sha1. */
+static void make_image(memfile *f, uint8_t sha1, uint8_t parentsha1)
+{
+	uint8_t *h = f->data;
+	memset(f, 0, sizeof(*f));
+	memcpy(h, "MComprHD", 8);
+	put_be32(h + 8, 124);       /* header length */
+	put_be32(h + 12, 5);        /* version; compressors at 16-31 stay 0 */
+	put_be32(h + 36, HUNK);     /* logicalbytes, low half */
+	put_be32(h + 44, 124);      /* mapoffset, low half */
+	put_be32(h + 56, HUNK);     /* hunkbytes */
+	put_be32(h + 60, HUNK);     /* unitbytes */
+	memset(h + 84, sha1, 20);
+	memset(h + 104, parentsha1, 20);
+	put_be32(h + 124, 1);       /* hunk 0 lives at 1 * HUNK */
+	memset(h + HUNK, 0x5a, HUNK);
+	f->size = IMAGE;
+}
+
+static int failures;
+
+static void expect(int cond, const char *what)
+{
+	printf("  %s  %s\n", cond ? "ok  " : "FAIL", what);
+	if (!cond)
+		failures++;
+}
+
+int main(void)
+{
+	static memfile base, other, child, garbage;
+	uint32_t not_a_chd[64];
+	chd_file *parent = NULL, *chd = NULL;
+	FILE *scratch;
+	uint8_t buf[HUNK];
+	chd_error err;
+
+	printf("success: the file stays open until chd_close()\n");
+	make_image(&base, 0x11, 0);
+	err = chd_open_core_file_callbacks(&mem_callbacks, &base, CHD_OPEN_READ, NULL, &chd);
+	expect(err == CHDERR_NONE, "opens");
+	if (err == CHDERR_NONE) {
+		expect(base.closes == 0, "not closed while open");
+		expect(chd_read(chd, 0, buf) == CHDERR_NONE && buf[0] == 0x5a && buf[HUNK - 1] == 0x5a,
+			"hunk 0 reads back");
+		chd_close(chd);
+		expect(base.closes == 1, "closed once by chd_close()");
+	}
+
+	printf("bad header: closed by the failed open\n");
+	make_image(&garbage, 0x11, 0);
+	garbage.data[0] = 'X';
+	err = chd_open_core_file_callbacks(&mem_callbacks, &garbage, CHD_OPEN_READ, NULL, &chd);
+	expect(err != CHDERR_NONE, "fails");
+	expect(garbage.closes == 1, "closed exactly once");
+
+	printf("truncated file: closed by the failed open\n");
+	make_image(&garbage, 0x11, 0);
+	garbage.size = 60;
+	err = chd_open_core_file_callbacks(&mem_callbacks, &garbage, CHD_OPEN_READ, NULL, &chd);
+	expect(err != CHDERR_NONE, "fails");
+	expect(garbage.closes == 1, "closed exactly once");
+
+	printf("parent that is not a chd_file: the file is closed, the parent untouched\n");
+	memset(not_a_chd, 0, sizeof(not_a_chd));
+	make_image(&child, 0x22, 0x11);
+	err = chd_open_core_file_callbacks(&mem_callbacks, &child, CHD_OPEN_READ,
+		(chd_file *)(void *)not_a_chd, &chd);
+	expect(err == CHDERR_INVALID_PARAMETER, "fails with CHDERR_INVALID_PARAMETER");
+	expect(child.closes == 1, "file closed exactly once");
+
+	printf("child without its parent: closed, retried with a fresh file\n");
+	make_image(&child, 0x22, 0x11);
+	err = chd_open_core_file_callbacks(&mem_callbacks, &child, CHD_OPEN_READ, NULL, &chd);
+	expect(err == CHDERR_REQUIRES_PARENT, "fails with CHDERR_REQUIRES_PARENT");
+	expect(child.closes == 1, "closed exactly once");
+
+	make_image(&base, 0x11, 0);
+	make_image(&child, 0x22, 0x11);
+	err = chd_open_core_file_callbacks(&mem_callbacks, &base, CHD_OPEN_READ, NULL, &parent);
+	expect(err == CHDERR_NONE, "parent opens");
+	if (err == CHDERR_NONE) {
+		err = chd_open_core_file_callbacks(&mem_callbacks, &child, CHD_OPEN_READ, parent, &chd);
+		expect(err == CHDERR_NONE, "child opens with its parent");
+		if (err == CHDERR_NONE) {
+			expect(child.closes == 0 && base.closes == 0, "neither closed while open");
+			chd_close(chd);
+			expect(child.closes == 1, "child closed once by chd_close()");
+			expect(base.closes == 1, "parent closed once along with it");
+		}
+	}
+
+	printf("child with the wrong parent: both closed by the failed open\n");
+	make_image(&other, 0x33, 0);
+	make_image(&child, 0x22, 0x11);
+	err = chd_open_core_file_callbacks(&mem_callbacks, &other, CHD_OPEN_READ, NULL, &parent);
+	expect(err == CHDERR_NONE, "unrelated parent opens");
+	if (err == CHDERR_NONE) {
+		err = chd_open_core_file_callbacks(&mem_callbacks, &child, CHD_OPEN_READ, parent, &chd);
+		expect(err == CHDERR_INVALID_PARENT, "fails with CHDERR_INVALID_PARENT");
+		expect(child.closes == 1, "child closed exactly once");
+		expect(other.closes == 1, "parent closed exactly once");
+	}
+
+	printf("chd_open() with a parent that is not a chd_file: its own FILE closed\n");
+	make_image(&child, 0x22, 0x11);
+	scratch = fopen(SCRATCH_NAME, "wb");
+	if (scratch == NULL) {
+		printf("  skip  cannot write %s in the working directory\n", SCRATCH_NAME);
+	} else {
+		int written = fwrite(child.data, 1, child.size, scratch) == child.size;
+#if HAVE_FILENO
+		int fd_before = fileno(scratch);
+#endif
+		written &= fclose(scratch) == 0;
+		expect(written, "scratch image written");
+		err = chd_open(SCRATCH_NAME, CHD_OPEN_READ, (chd_file *)(void *)not_a_chd, &chd);
+		expect(err == CHDERR_INVALID_PARAMETER, "fails with CHDERR_INVALID_PARAMETER");
+#if HAVE_FILENO
+		scratch = fopen(SCRATCH_NAME, "rb");
+		expect(scratch != NULL && fileno(scratch) == fd_before, "its descriptor was released");
+		if (scratch != NULL)
+			fclose(scratch);
+#endif
+		remove(SCRATCH_NAME);
+	}
+
+	printf("no callbacks: rejected, nothing to close with\n");
+	err = chd_open_core_file_callbacks(NULL, &base, CHD_OPEN_READ, NULL, &chd);
+	expect(err == CHDERR_INVALID_PARAMETER, "fails with CHDERR_INVALID_PARAMETER");
+
+	if (failures) {
+		printf("%d check(s) failed\n", failures);
+		return 1;
+	}
+	printf("all checks passed\n");
+	return 0;
+}