[FFmpeg-devel] [PATCH] avformat/movenc: Fix stack overflow when remuxing timecode tracks

[Martin Storsjö]

On Wed, 30 Sep 2020, Andreas Rheinhardt wrote:

> There are two possible kinds of timecode tracks (with tag "tmcd") in the
> mov muxer: Tracks created internally by the muxer and timecode tracks
> sent by the user. If any of the latter exists, the former are
> deactivated. The former all belong to another track, the source
> track; the latter don't have a source track set, but the index of the
> source track is initially zeroed by av_mallocz_array().

Would it be worthwhile to explicitly initialize these to e.g. -1, to make 
src_track clearer?

> This is a problem since 3d894db700cc1e360a7a75ab9ac8bf67ac6670a3: Said 
> commit added a function that calculates the duration of tracks and the 
> duration of timecode tracks is calculated by rescaling the duration 
> (calculated by the very same function) of the source track. This gives 
> an infinite recursion if the first track (the one that will be treated 
> as source track for all timecode tracks) is a timecode track itself, 
> leading to a stack overflow.
>
> This commit fixes this by not using the nonexistent source track
> when calculating the duration of timecode tracks not created internally
> by the mov muxer.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> 1. Remuxing timecode tracks is currently impossible for mp4 (the codec
> tag validation fails); I wonder whether this is actually intended given
> that there is a warning that timecode metadata is ignored if an explicit
> timecode track is present.
> 2. There is another place where the src_track field is used even when a
> timecode track doesn't have a src_track: When writing the moov tag
> (lines 4156-4166). This will probably also need to be fixed, but it is
> not dangerous.
> 3. There is btw no validation that a track with "tmcd" tag is a data
> stream.
>
> libavformat/movenc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 2006fcee4b..265465f97b 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -2901,7 +2901,7 @@ static int mov_write_minf_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContext
> 
> static int64_t calc_pts_duration(MOVMuxContext *mov, MOVTrack *track)
> {
> -    if (track->tag == MKTAG('t','m','c','d')) {
> +    if (track->tag == MKTAG('t','m','c','d') && mov->nb_meta_tmcd) {
>         // tmcd tracks gets track_duration set in mov_write_moov_tag from
>         // another track's duration, while the end_pts may be left at zero.

This fix in itself is probably good and safe, so LGTM.

// Martin