| Message ID | 20230713105553.21052-16-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/33] fftools/ffmpeg_mux_init: return errors from of_open() instead of aborting | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Thu, Jul 13, 2023 at 12:55:36PM +0200, Anton Khirnov wrote: > Replace duplicated(!) and broken* custom string parsing with > av_dict_parse_string(). Return error codes instead of aborting. > > * e.g. it treats NULL returned from av_get_token() as "separator not > found", when in fact av_get_token() only returns NULL on memory > allocation failure > --- > fftools/ffmpeg_mux_init.c | 111 ++++++++++++++++++-------------------- > 1 file changed, 52 insertions(+), 59 deletions(-) smells like memory corruption -i ~/videos/01c56b0dc1.ts -copy_unknown -map 0 -c copy -fflags +bitexact -t 3 -y file-copy-unknown.ts http://samples.mplayerhq.hu/ts/01c56b0dc1.ts [mpegts @ 0x2ced6980] Could not find codec parameters for stream 6 (Unknown: none ([5][0][0][0] / 0x0005)): unknown codec Consider increasing the value for the 'analyzeduration' (0) and 'probesize' (5000000) options Input #0, mpegts, from 'videos/01c56b0dc1.ts': Duration: 00:00:10.73, start: 40848.136244, bitrate: 8431 kb/s Program 1201 Stream #0:0[0xfb]: Video: h264 (Main) ([27][0][0][0] / 0x001B), yuv420p(tv, bt709, top first), 1920x1080 [SAR 1:1 DAR 16:9], 25 fps, 50 tbr, 90k tbn Stream #0:1[0x12d](eng): Audio: aac_latm (HE-AAC) ([17][0][0][0] / 0x0011), 48000 Hz, stereo, fltp Stream #0:2[0x132](eng): Audio: aac_latm (HE-AAC) ([17][0][0][0] / 0x0011), 48000 Hz, stereo, fltp (visual impaired) (descriptions) Stream #0:3[0x192](eng): Audio: ac3 ([6][0][0][0] / 0x0006), 48000 Hz, 5.1(side), fltp, 384 kb/s Stream #0:4[0x3ea]: Unknown: none ([11][0][0][0] / 0x000B) Stream #0:5[0x401](eng): Subtitle: dvb_subtitle ([6][0][0][0] / 0x0006) (hearing impaired) Stream #0:6[0x1f40]: Unknown: none ([5][0][0][0] / 0x0005) ==2131== Conditional jump or move depends on uninitialised value(s) ==2131== at 0x3162AF: of_open (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== by 0x31863B: open_files.isra.2 (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== by 0x319BAA: ffmpeg_parse_options (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== by 0x2F7A00: main (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== Uninitialised value was created by a stack allocation ==2131== at 0x3139DD: of_open (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== ==2131== Invalid free() / delete / delete[] / realloc() ==2131== at 0x4C32D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==2131== by 0x315AEF: of_open (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== by 0x31863B: open_files.isra.2 (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== by 0x319BAA: ffmpeg_parse_options (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== by 0x2F7A00: main (in ffmpeg-git/ffmpeg/ffmpeg_g) ==2131== Address 0x22f462840 is not stack'd, malloc'd or (recently) free'd ==2131== Output #0, mpegts, to 'file-copy-unknown.ts': Stream #0:0: Video: h264 (Main) ([27][0][0][0] / 0x001B), yuv420p(tv, bt709, top first), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 25 fps, 50 tbr, 90k tbn Stream #0:1(eng): Audio: aac_latm (HE-AAC) ([17][0][0][0] / 0x0011), 48000 Hz, stereo, fltp (default) Stream #0:2(eng): Audio: aac_latm (HE-AAC) ([17][0][0][0] / 0x0011), 48000 Hz, stereo, fltp (visual impaired) (descriptions) Stream #0:3(eng): Audio: ac3 ([6][0][0][0] / 0x0006), 48000 Hz, 5.1(side), fltp, 384 kb/s Stream #0:4: Unknown: none ([11][0][0][0] / 0x000B) Stream #0:5(eng): Subtitle: dvb_subtitle ([6][0][0][0] / 0x0006) (hearing impaired) Stream #0:6: Unknown: none ([5][0][0][0] / 0x0005) Stream mapping: Stream #0:0 -> #0:0 (copy) Stream #0:1 -> #0:1 (copy) Stream #0:2 -> #0:2 (copy) Stream #0:3 -> #0:3 (copy) Stream #0:4 -> #0:4 (copy) Stream #0:5 -> #0:5 (copy) Stream #0:6 -> #0:6 (copy) Press [q] to stop, [?] for help [mpegts @ 0x2ced6980] PES packet size mismatche=00:00:02.98 bitrate= 0.0kbits/s speed=5.97x [mpegts @ 0x2ced6980] Packet corrupt (stream = 1, dts = 3677260813). [in#0/mpegts @ 0x2ced67c0] corrupt input packet in stream 1 [mpegts @ 0x2ced6980] PES packet size mismatch [mpegts @ 0x2ced6980] Packet corrupt (stream = 2, dts = 3677250303). [in#0/mpegts @ 0x2ced67c0] corrupt input packet in stream 2 Last message repeated 3 times [mpegts @ 0x2ced6980] PES packet size mismatch [mpegts @ 0x2ced6980] Packet corrupt (stream = 3, dts = 3677239462). [in#0/mpegts @ 0x2ced67c0] corrupt input packet in stream 3 Last message repeated 1 times [mpegts @ 0x2ea0edc0] Stream 4, codec none, is muxed as a private data stream and may not be recognized upon reading. [mpegts @ 0x2ea0edc0] Stream 6, codec none, is muxed as a private data stream and may not be recognized upon reading. [out#0/mpegts @ 0x2d09f040] video:484kB audio:206kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 6.355365% frame= 42 fps=0.0 q=-1.0 Lsize= 734kB time=00:00:02.98 bitrate=2011.2kbits/s speed=3.31x ==2131== ==2131== HEAP SUMMARY: ==2131== in use at exit: 49,276 bytes in 240 blocks ==2131== total heap usage: 23,803 allocs, 23,564 frees, 66,233,951 bytes allocated [...]
Quoting Michael Niedermayer (2023-07-14 01:30:24) > On Thu, Jul 13, 2023 at 12:55:36PM +0200, Anton Khirnov wrote: > > Replace duplicated(!) and broken* custom string parsing with > > av_dict_parse_string(). Return error codes instead of aborting. > > > > * e.g. it treats NULL returned from av_get_token() as "separator not > > found", when in fact av_get_token() only returns NULL on memory > > allocation failure > > --- > > fftools/ffmpeg_mux_init.c | 111 ++++++++++++++++++-------------------- > > 1 file changed, 52 insertions(+), 59 deletions(-) > > smells like memory corruption > > -i ~/videos/01c56b0dc1.ts -copy_unknown -map 0 -c copy -fflags +bitexact -t 3 -y file-copy-unknown.ts I have no idea why are you replying to this specific patch, when this issue apparently exists in current master. I'll send a fix for it in a later patchset, but it's completely unrelated to this set.
On Fri, Jul 14, 2023 at 11:07:19AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-07-14 01:30:24) > > On Thu, Jul 13, 2023 at 12:55:36PM +0200, Anton Khirnov wrote: > > > Replace duplicated(!) and broken* custom string parsing with > > > av_dict_parse_string(). Return error codes instead of aborting. > > > > > > * e.g. it treats NULL returned from av_get_token() as "separator not > > > found", when in fact av_get_token() only returns NULL on memory > > > allocation failure > > > --- > > > fftools/ffmpeg_mux_init.c | 111 ++++++++++++++++++-------------------- > > > 1 file changed, 52 insertions(+), 59 deletions(-) > > > > smells like memory corruption > > > > -i ~/videos/01c56b0dc1.ts -copy_unknown -map 0 -c copy -fflags +bitexact -t 3 -y file-copy-unknown.ts > > I have no idea why are you replying to this specific patch, when this > issue apparently exists in current master. git bisect of a segfault lead me to this one with memory corruption, its quite possible it doesnt always segfault so git bisect results can be bogus sorry [...]
diff --git a/fftools/ffmpeg_mux_init.c b/fftools/ffmpeg_mux_init.c index 3a327048bb..b0befb8924 100644 --- a/fftools/ffmpeg_mux_init.c +++ b/fftools/ffmpeg_mux_init.c @@ -1782,76 +1782,66 @@ static int setup_sync_queues(Muxer *mux, AVFormatContext *oc, int64_t buf_size_u return 0; } -static void of_add_programs(Muxer *mux, const OptionsContext *o) +static int of_add_programs(Muxer *mux, const OptionsContext *o) { AVFormatContext *oc = mux->fc; /* process manually set programs */ for (int i = 0; i < o->nb_program; i++) { - const char *p = o->program[i].u.str; - int progid = i+1; + AVDictionary *dict = NULL; + const AVDictionaryEntry *e; AVProgram *program; + int ret, progid = i + 1; - while(*p) { - const char *p2 = av_get_token(&p, ":"); - const char *to_dealloc = p2; - char *key; - if (!p2) - break; + ret = av_dict_parse_string(&dict, o->program[i].u.str, "=", ":", + AV_DICT_MULTIKEY); + if (ret < 0) { + av_log(mux, AV_LOG_ERROR, "Error parsing program specification %s\n", + o->program[i].u.str); + return ret; + } - if(*p) p++; - - key = av_get_token(&p2, "="); - if (!key || !*p2) { - av_freep(&to_dealloc); - av_freep(&key); - break; - } - p2++; - - if (!strcmp(key, "program_num")) - progid = strtol(p2, NULL, 0); - av_freep(&to_dealloc); - av_freep(&key); + e = av_dict_get(dict, "program_num", NULL, 0); + if (e) { + progid = strtol(e->value, NULL, 0); + av_dict_set(&dict, e->key, NULL, 0); } program = av_new_program(oc, progid); - if (!program) - report_and_exit(AVERROR(ENOMEM)); - - p = o->program[i].u.str; - while(*p) { - const char *p2 = av_get_token(&p, ":"); - const char *to_dealloc = p2; - char *key; - if (!p2) - break; - if(*p) p++; - - key = av_get_token(&p2, "="); - if (!key) { - av_log(mux, AV_LOG_FATAL, - "No '=' character in program string %s.\n", - p2); - exit_program(1); - } - if (!*p2) - exit_program(1); - p2++; - - if (!strcmp(key, "title")) { - av_dict_set(&program->metadata, "title", p2, 0); - } else if (!strcmp(key, "program_num")) { - } else if (!strcmp(key, "st")) { - int st_num = strtol(p2, NULL, 0); - av_program_add_stream_index(oc, progid, st_num); - } else { - av_log(mux, AV_LOG_FATAL, "Unknown program key %s.\n", key); - exit_program(1); - } - av_freep(&to_dealloc); - av_freep(&key); + if (!program) { + ret = AVERROR(ENOMEM); + goto fail; } + + e = av_dict_get(dict, "title", NULL, 0); + if (e) { + av_dict_set(&program->metadata, e->key, e->value, 0); + av_dict_set(&dict, e->key, NULL, 0); + } + + e = NULL; + while (e = av_dict_get(dict, "st", e, 0)) { + int st_num = strtol(e->value, NULL, 0); + av_program_add_stream_index(oc, progid, st_num); + } + + // make sure that nothing but "st" entries are left in the dict + e = NULL; + while (e = av_dict_iterate(dict, e)) { + if (!strcmp(e->key, "st")) + continue; + + av_log(mux, AV_LOG_FATAL, "Unknown program key %s.\n", e->key); + ret = AVERROR(EINVAL); + goto fail; + } + +fail: + av_dict_free(&dict); + if (ret < 0) + return ret; } + + return 0; } /** @@ -2547,7 +2537,10 @@ int of_open(const OptionsContext *o, const char *filename) if (err < 0) return err; - of_add_programs(mux, o); + err = of_add_programs(mux, o); + if (err < 0) + return err; + of_add_metadata(of, oc, o); err = set_dispositions(mux, o);