From 524468ed729201c56458ac10ab8f787601ded636 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Fri, 11 Sep 2026 00:55:34 +0200 Subject: [PATCH] Close the file and parent on every failed open chd_open_core_file_callbacks() closes the caller's file and parent when an open fails: the cleanup path runs chd_close() on the half-built handle. The two exits before that handle exists - an invalid parent, and a failed malloc of the chd_file - returned without closing anything, so the same call either consumed its arguments or leaked them depending on where it failed. Close them on those two exits as well, and document the contract in chd.h. Taking the other side - leaving the file to the caller on failure - would break existing callers: one wraps the file in an object its fclose callback deletes and relies on libchdr to do so on failure, and none of them close the parent after a failed open. chd_open_file() is unaffected, its fclose callback does nothing. The only callers this can hurt are ones that close the file again after a failure, and those already close it twice on every other failure. The ESP-IDF bridge in contrib/ was one: it fclose()d the FILE after a failed open, a double fclose() on any image that failed past the allocation. It and the FatFs bridge now leave the close to libchdr. The new test builds its images in memory, so it runs without the corpus. Without this change it fails on the invalid-parent exit, both through a counting fclose callback and, for chd_open(), through the file descriptor the failed open kept. Fixes #193 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01846EhHuAFk5qvxwEA5Gq6y --- contrib/esp32p4/chd/chd_esp_vfs.c | 5 +- .../idf-benchmark/main/benchmark_main.c | 6 +- contrib/tangcore-bl616/chd/chd_fatfs.c | 4 +- include/libchdr/chd.h | 14 +- src/libchdr_chd.c | 28 ++- tests/CMakeLists.txt | 7 + tests/open_ownership.c | 228 ++++++++++++++++++ 7 files changed, 274 insertions(+), 18 deletions(-) create mode 100644 tests/open_ownership.c diff --git a/contrib/esp32p4/chd/chd_esp_vfs.c b/contrib/esp32p4/chd/chd_esp_vfs.c index 814becc1..e1d5d345 100644 --- a/contrib/esp32p4/chd/chd_esp_vfs.c +++ b/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 diff --git a/contrib/esp32p4/idf-benchmark/main/benchmark_main.c b/contrib/esp32p4/idf-benchmark/main/benchmark_main.c index 22bc0408..87c7c952 100644 --- a/contrib/esp32p4/idf-benchmark/main/benchmark_main.c +++ b/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 diff --git a/contrib/tangcore-bl616/chd/chd_fatfs.c b/contrib/tangcore-bl616/chd/chd_fatfs.c index 27666711..b8dc517c 100644 --- a/contrib/tangcore-bl616/chd/chd_fatfs.c +++ b/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 diff --git a/include/libchdr/chd.h b/include/libchdr/chd.h index 0d483b9a..f07c374e 100644 --- a/include/libchdr/chd.h +++ b/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); diff --git a/src/libchdr_chd.c b/src/libchdr_chd.c index 6be528d3..6e35aa21 100644 --- a/src/libchdr_chd.c +++ b/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; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f025e5dd..ae2c0f8f 100644 --- a/tests/CMakeLists.txt +++ b/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 diff --git a/tests/open_ownership.c b/tests/open_ownership.c new file mode 100644 index 00000000..8ae12279 --- /dev/null +++ b/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 +#include +#include + +#include + +#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; +}