| Message ID | 20200930132014.1113907-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 22a2386a561ccbaabbbfd5cf7f89b2cbbade71b0 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/movenc: Fix stack overflow when remuxing timecode tracks | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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@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
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@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
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. // Calculate the pts duration for that track instead.
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(). 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@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(-)