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; +}