PR #25611 opened by Kilian (lusoris) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25611 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25611.patch
Two error-path fixes in `tools/ismindex`, both found by the automated review of PR 25471 and independent of it. **Missing option values.** `-n`, `-path-prefix`, `-ismc-prefix` and `-output` read `argv[i + 1]` without checking that it exists. A trailing `-output` crashes; a trailing `-ismc-prefix` writes `(null)foo.ismc` into the server manifest and exits 0. All four now print the usage and exit 1. **`.ismf` write errors.** `write_fragments()` ignores stream errors and the result of `fclose()`, so an index that could not be written still exits 0. It now prints the error and returns it, keeping an earlier fragment error. | Case, on 17a4a474f4 x86-64 | Before | After | | --- | --- | --- | | `ismindex -n foo in.ismv -output` | exit 139 | exit 1, usage | | `ismindex -n foo in.ismv -ismc-prefix` | exit 0, `(null)foo.ismc` in `foo.ism` | exit 1, usage | | `ismindex -n foo in.ismv -path-prefix` | exit 0 | exit 1, usage | | `ismindex -ismf -n foo in.ismv`, `foo.ismf` -> `/dev/full` | exit 0 | exit 1, `Error writing foo.ismf: No space left on device` | Tested with GCC 14.4.0 and GCC 16.2.1. On a 28,126-fragment input the `.ism`, `.ismc` and `.ismf` output is byte-identical to master. No new warnings. FATE has no `ismindex` test, so none was run. This conflicts textually with PR 25471 in `main()` and at the end of `write_fragments()`. Whichever lands second needs a rebase; I will do it if this one is second. >From c0348232f0bf35859dd5ec7827fa450e772a10f4 Mon Sep 17 00:00:00 2001 From: lusoris <[email protected]> Date: Fri, 30 Oct 2026 14:01:35 +0100 Subject: [PATCH 1/2] tools/ismindex: check that option values are present -n, -path-prefix, -ismc-prefix and -output read argv[i + 1] without checking that it exists. A trailing -output passes NULL to strlen() and crashes, and a trailing -ismc-prefix writes "(null)foo.ismc" into the server manifest and exits with success. Print the usage and fail when the value is missing, freeing the tracks through a common exit path. Signed-off-by: lusoris <[email protected]> --- tools/ismindex.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/tools/ismindex.c b/tools/ismindex.c index 4fd9a781d03..0407ebba4c4 100644 --- a/tools/ismindex.c +++ b/tools/ismindex.c @@ -785,20 +785,36 @@ int main(int argc, char **argv) const char *path_prefix = "", *ismc_prefix = ""; const char *output_prefix = ""; char output_prefix_buf[2048]; - int split = 0, ismf = 0, i; + int split = 0, ismf = 0, i, ret = 0; struct Tracks tracks = { 0, .video_track = -1, .audio_track = -1 }; for (i = 1; i < argc; i++) { if (!strcmp(argv[i], "-n")) { + if (i + 1 >= argc) { + ret = usage(argv[0], 1); + goto fail; + } basename = argv[i + 1]; i++; } else if (!strcmp(argv[i], "-path-prefix")) { + if (i + 1 >= argc) { + ret = usage(argv[0], 1); + goto fail; + } path_prefix = argv[i + 1]; i++; } else if (!strcmp(argv[i], "-ismc-prefix")) { + if (i + 1 >= argc) { + ret = usage(argv[0], 1); + goto fail; + } ismc_prefix = argv[i + 1]; i++; } else if (!strcmp(argv[i], "-output")) { + if (i + 1 >= argc) { + ret = usage(argv[0], 1); + goto fail; + } output_prefix = argv[i + 1]; i++; if (output_prefix[strlen(output_prefix) - 1] != '/') { @@ -828,7 +844,8 @@ int main(int argc, char **argv) path_prefix, ismc_prefix); output_client_manifest(&tracks, basename, output_prefix, split); +fail: clean_tracks(&tracks); - return 0; + return ret; } -- 2.54.0 >From e456431e16b27b09de1d9ba5c3535450c9f8336e Mon Sep 17 00:00:00 2001 From: lusoris <[email protected]> Date: Fri, 30 Oct 2026 14:01:42 +0100 Subject: [PATCH 2/2] tools/ismindex: report write errors on the .ismf index write_fragments() ignores stream errors and the result of fclose(), so an index that could not be written, for example on a full disk, still exits with success. stdio buffers the writes, so the failure usually only shows up at fclose(). Report it and return it, keeping an earlier fragment error. Stop read_mfra() from adding "Unable to read the MFRA atom" to errors from write_fragments(), which print their own message: the atom was read. Signed-off-by: lusoris <[email protected]> --- tools/ismindex.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tools/ismindex.c b/tools/ismindex.c index 0407ebba4c4..1d4b8f270c5 100644 --- a/tools/ismindex.c +++ b/tools/ismindex.c @@ -219,8 +219,16 @@ static int write_fragments(struct Tracks *tracks, int start_index, } } fail: - if (out) - fclose(out); + if (out) { + int io_ret = ferror(out) ? AVERROR(EIO) : 0; + if (fclose(out) && !io_ret) + io_ret = AVERROR(errno ? errno : EIO); + if (io_ret) { + fprintf(stderr, "Error writing %s: %s\n", idxname, av_err2str(io_ret)); + if (!ret) + ret = io_ret; + } + } return ret; } @@ -440,7 +448,8 @@ static int read_mfra(struct Tracks *tracks, int start_index, if (split || ismf) err = write_fragments(tracks, start_index, f, basename, split, ismf, output_prefix); - err_str = "error in write_fragments"; + avio_close(f); + return err; fail: if (f) -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
