switch_to_next_file() re-enters itself through open(), advancing current_file twice #2344

Activity

1. NaitikVerma6776 commented on Sep 13, 2026

   

   NaitikVerma6776

   on Sep 13, 2026

   Last edited by NaitikVerma6776

   Contributor

   More actions

   Hi @cfsmp3 , I’ve investigated this issue and have opened a PR with a proposed fix.
   The issue appears to be caused by detect_stream_type() running with binary_concat enabled, allowing its probing read to reach EOF and trigger switch_to_next_file(), which advances current_file a second time.

   I’ve followed the approach already used in #2209 by temporarily disabling binary_concat during stream detection and restoring it afterward. I’ve also added a regression test to ensure that detection does not unexpectedly advance to the next input file.

   PR: #2347

   Please let me know if this approach matches the direction you had in mind, or if you’d prefer a different approach regarding the probing read or the C/Rust switch_to_next_file() implementations.
2. kaihere14 commented on Sep 13, 2026

   

   kaihere14

   on Sep 13, 2026

   Contributor

   More actions

   Went deep into this while working through general_loop/switch_to_next_file, wanted to understand exactly why the re-entrancy still reproduces despite #2209 already being merged. Here's the full trace.

   Two independent implementations, only one ever runs

   ccx_demuxer_open (src/lib_ccx/ccx_demuxer.c:59) is a thin shim in a normal build:

   static int ccx_demuxer_open(struct ccx_demuxer *ctx, const char *file)
   {
   #ifndef DISABLE_RUST
       return ccxr_demuxer_open(ctx, file);
   #else
       ... (full C body)
   #endif

   DISABLE_RUST is only ever set by linux/build under an explicit -min-rust flag, the default build ships with Rust enabled. So in any normal build, this function never runs its own C body at all; it immediately hands off to ccxr_demuxer_open (src/rust/src/libccxr_exports/demuxer.rs:498), which calls the real implementation: CcxDemuxer::open() in src/rust/src/demuxer/demux.rs:150.

   The #2209 guard only exists in the dead C path

   The binary_concat = 0 guard added by #2209 lives at ccx_demuxer.c:137-151, inside that same #else branch:

   int saved_binary_concat = ccx_options.binary_concat;
   ccx_options.binary_concat = 0;
   detect_stream_type(ctx);
   ccx_options.binary_concat = saved_binary_concat;

   This code is only compiled under DISABLE_RUST. The Rust implementation's own call to detect_stream_type (demux.rs, around line 217) has no equivalent, no save, no suppression, no restore. I grepped the whole src/rust/ tree for binary_concat = false; the only hit is in test setup code (stream_functions.rs:519), not anywhere in the real open/detect path.

   So #2209 fixed the symptom in a build configuration that isn't the default. Every normal build still runs the unguarded Rust path.

   Confirmed against the actual repro

   With the default build (Rust enabled): ccx_demuxer_open → ccxr_demuxer_open → CcxDemuxer::open() → unguarded detect_stream_type → buffered_read_opt (file.rs:261) → switch_to_next_file (file.rs:150, called from the binary_concat-gated branches at lines 376/453/501/595) while binary_concat is still true. This is exactly the mechanism from your watchpoint trace, a small file like the 153-byte RCWT sample from #2342 hits EOF inside the 1MB detect_stream_type probing read, and the re-entrant call fires.

   Full caller inventory, for completeness

   Every switch_to_next_file call site, C and Rust:

   Site	In the open()-triggered chain?
   ccextractor.c:148 (start_ccx outer loop)	Reaches into open() → detect_stream_type, but only neutralized if the C guard applies, which it doesn't in a default build
   file_functions.c:400/432/461/506	The re-entrant calls themselves (C side)
   file.rs:376/453/501/595	Rust mirrors of the above, this is the actual live path in a default build

   No other production call sites exist (file.rs:917/923/954/990 are unit-test only).

   On the two open questions

   Should detect_stream_type's probing read be able to switch files at all? I don't think it should. Probing is meant to answer "what format is this file", not "what's the next file in the list", those are two unrelated concerns that currently share one mutable counter purely by accident of buffered_read_opt treating EOF-during-probing the same as EOF-during-real-reading. Porting the guard (below) fixes the symptom for autodetection specifically, but the deeper issue is that detect_stream_type has no way to say "I hit the end of this file while probing" without going through the exact same code path as "I hit the end of this file while actually done with it". A cleaner long-term fix might be giving the probing read its own short-circuit that just reports EOF without ever calling switch_to_next_file, rather than suppressing binary_concat around it.

   Should the C switch_to_next_file() remain? I'd walk back a cleaner "just remove it" take here. DISABLE_RUST exists for a reason, presumably build environments where a Rust toolchain isn't available or wanted (older distros, minimal-dependency package builds, certain CI targets), and if the C implementation goes away entirely, that build path stops being an option at all. So the C version probably needs to stay for those builds to keep working. The actual problem isn't that two implementations exist, it's that they can silently drift apart with no mechanism catching it, which is exactly how this bug happened in the first place. A test that fails CI when a fix lands in one implementation but not the other (or some shared source of truth both build paths compile from) seems like a better fix than deleting either copy.

   Suggested fix

   Short term: port the same save/suppress/restore pattern into demux.rs around the detect_stream_type call, mirror the C guard exactly, so both implementations behave identically during autodetection regardless of which one a given build actually runs.

   Longer term, per the above: worth considering giving the probing read its own EOF path that never touches switch_to_next_file at all, and adding some safeguard (CI parity check or a shared source of truth) so a fix to one implementation can't silently miss the other again.

   @cfsmp3 if this is still unclaimed I'd like to take it on, happy to start with either the short term fix or discussing the longer term direction first, whichever you'd prefer.
3. NaitikVerma6776 commented on Sep 14, 2026

   

   NaitikVerma6776

   on Sep 14, 2026

   Last edited by NaitikVerma6776

   Contributor

   More actions

   Hi @cfsmp3 , I investigated #2344 and traced the re-entry through the current Rust path.

   The issue appears to be that CcxDemuxer::open() calls detect_stream_type() with binary_concat still enabled. For small inputs, the 1 MiB detection read reaches EOF and buffered_read_opt() treats that as a concatenation boundary, calling Rust switch_to_next_file() and advancing the shared current_file during the outer open().

   I also noticed that #2209 already prevents this on the C ccx_demuxer_open() path by temporarily disabling binary_concat around detection.

   My current understanding is that the Rust CcxDemuxer::open() path should follow the same contract: save binary_concat, disable it for detect_stream_type(), and restore it before returning/copying the Rust state back.

   I’m planning to add a regression test with two small RCWT inputs to verify that detection doesn't advance current_file.

   Does this approach look right to you? If so, I’d be happy to work on the fix. I have also linked a Pr fix for this please review it .
4. pranayr710 commented on Sep 19, 2026

   

   pranayr710

   on Sep 19, 2026

   Contributor

   More actions

   A concrete user-visible consequence of this, in case it helps prioritise: the double-advance breaks MP4 extraction entirely on the default (GPAC) path.

   ccextractor.c:252 passes ctx->inputfile[ctx->current_file] straight to processmp4(). By the time CCX_SM_MP4 is reached, current_file has already been advanced a second time inside open(), so with a single input file the index lands one past the last entry and the pointer is NULL:

   $ ccextractor plain.mp4 --out=srt
   Opening file: plain.mp4
   File seems to be a MP4
   Analyzing data with GPAC (MP4 library)
   Error: NULL file path provided to processmp4
   Done, processing time = 0 seconds
   

   The current_file == -1 guard just above it doesn't catch this, because the index is not -1 — it is simply past the end.

   With two input files the second is opened before the first is processed, which shows the advance directly:

   $ ccextractor plain.mp4 plain2.mp4 --out=srt
   Opening file: plain.mp4
   Opening file: plain2.mp4      <-- opened during the first file's open()
   File seems to be a MP4
   

   It only affects autodetect. Naming the format skips detect_stream_type(), so the re-entrant path in your trace is never taken and everything works:

   $ ccextractor plain.mp4 --input mp4 --out=srt
   Opening '/w/plain.mp4': ok
   Track 1, type=vide subtype=avc1
   Track 2, type=sbtl subtype=tx3g
   MP4: found 2 tracks: 1 avc, 0 hevc, 1 cc, 0 vobsub
   

   So --input mp4 is a workaround for anyone who hits this.

   TS is unaffected because it keeps using the already-open demuxer rather than re-reading the filename after close_input_file().

   Reproduced on 2364994a (current master, includes #2343) and also on 907be052 and 4405f958, so it predates #2343 rather than being introduced by it. Built from docker/Dockerfile with BUILD_TYPE=minimal; linux/build takes the same GPAC path, since ENABLE_FFMPEG_MP4 is only set by src/CMakeLists.txt.

   Any MP4 reproduces it — ffmpeg -f lavfi -i testsrc=duration=3 -c:v libx264 plain.mp4 is enough. Not touching the fix, since you two are already on it; just flagging the impact.
5. cfsmp3

   closed this as completedin #2347on Oct 1, 2026