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

[Andreas Rheinhardt]

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?
> 

I wouldn't object to this, but this could only be done after fixing the
second point below. (Also note that there is actually an overflow
problem with nb_streams when AVFormatContext.nb_streams is huge (it's
technically an unsigned, but restricted to the range of int) and if you
use -1, you can't really solve the overflow problem by using an unsigned.)

>> 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.
> 
Thanks, applied.

- Andreas