diff mbox series

[FFmpeg-devel,07/13] avformat/matroskaenc: Don't segfault when seekability changes

Message ID 20200502171700.28991-2-andreas.rheinhardt@gmail.com
State Accepted
Commit 8aabcf6c1151b9e50ae5447da6709a72022b9a60
Headers show
Series [FFmpeg-devel,1/6] avformat/matroskaenc: Move adding SeekEntry into end_ebml_master_crc32()
Related show

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

Andreas Rheinhardt May 2, 2020, 5:16 p.m. UTC
If the Matroska muxer's AVIOContext was unseekable when writing the
header, but is seekable when writing the trailer, the code for writing
the trailer presumes that a dynamic buffer exists and tries to update
its content in order to overwrite data that has already been
preliminarily written when writing the header, yet said buffer doesn't
exist as it has been written finally and not preliminarily when writing
the header (because of the unseekability it was presumed that one won't
be able to update the data anyway).

This commit adds a check for this and also for a similar situation
involving updating extradata with new data from packet side-data.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
It seems that the whole code was based around the presumption that the
seekability does not change mid-stream. E.g. before add68dcca there were
the way level 1 elements were written depended upon the seekability and
the output would be corrupt if it changed between opening and closing. 

Given the lack of tickets about this it seems that this assumption holds
true. Therefore this patch uses the existence of the Tracks buffer as
proxy for seekability.

 libavformat/matroskaenc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Andreas Rheinhardt May 17, 2020, 11:50 p.m. UTC | #1
Andreas Rheinhardt:
> If the Matroska muxer's AVIOContext was unseekable when writing the
> header, but is seekable when writing the trailer, the code for writing
> the trailer presumes that a dynamic buffer exists and tries to update
> its content in order to overwrite data that has already been
> preliminarily written when writing the header, yet said buffer doesn't
> exist as it has been written finally and not preliminarily when writing
> the header (because of the unseekability it was presumed that one won't
> be able to update the data anyway).
> 
> This commit adds a check for this and also for a similar situation
> involving updating extradata with new data from packet side-data.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> It seems that the whole code was based around the presumption that the
> seekability does not change mid-stream. E.g. before add68dcca there were
> the way level 1 elements were written depended upon the seekability and
> the output would be corrupt if it changed between opening and closing. 
> 
> Given the lack of tickets about this it seems that this assumption holds
> true. Therefore this patch uses the existence of the Tracks buffer as
> proxy for seekability.
> 
>  libavformat/matroskaenc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 99b549ecc4..1a142403fa 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -2186,7 +2186,7 @@ static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt)
>  
>      switch (par->codec_id) {
>      case AV_CODEC_ID_AAC:
> -        if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) {
> +        if (side_data_size && mkv->track.bc) {
>              int filler, output_sample_rate = 0;
>              ret = get_aac_sample_rates(s, side_data, side_data_size, &track->sample_rate,
>                                         &output_sample_rate);
> @@ -2213,7 +2213,7 @@ static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt)
>          }
>          break;
>      case AV_CODEC_ID_FLAC:
> -        if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) {
> +        if (side_data_size && mkv->track.bc) {
>              uint8_t *old_extradata = par->extradata;
>              if (side_data_size != par->extradata_size) {
>                  av_log(s, AV_LOG_ERROR, "Invalid FLAC STREAMINFO metadata for output stream %d\n",
> @@ -2229,8 +2229,7 @@ static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt)
>      // FIXME: Remove the following once libaom starts propagating extradata during init()
>      //        See https://bugs.chromium.org/p/aomedia/issues/detail?id=2012
>      case AV_CODEC_ID_AV1:
> -        if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live &&
> -            !par->extradata_size) {
> +        if (side_data_size && mkv->track.bc && !par->extradata_size) {
>              AVIOContext *dyn_cp;
>              uint8_t *codecpriv;
>              int codecpriv_size;
> @@ -2541,6 +2540,7 @@ static int mkv_write_trailer(AVFormatContext *s)
>          if (ret < 0)
>              return ret;
>  
> +        if (mkv->info.bc) {
>          // update the duration
>          av_log(s, AV_LOG_DEBUG, "end duration = %" PRIu64 "\n", mkv->duration);
>          avio_seek(mkv->info.bc, mkv->duration_offset, SEEK_SET);
> @@ -2549,6 +2549,7 @@ static int mkv_write_trailer(AVFormatContext *s)
>                                      MATROSKA_ID_INFO, 0, 0, 0);
>          if (ret < 0)
>              return ret;
> +        }
>  
>          if (mkv->track.bc) {
>              // write Tracks master
> 
Will apply this patchset tomorrow unless there are objections.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 99b549ecc4..1a142403fa 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -2186,7 +2186,7 @@  static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt)
 
     switch (par->codec_id) {
     case AV_CODEC_ID_AAC:
-        if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) {
+        if (side_data_size && mkv->track.bc) {
             int filler, output_sample_rate = 0;
             ret = get_aac_sample_rates(s, side_data, side_data_size, &track->sample_rate,
                                        &output_sample_rate);
@@ -2213,7 +2213,7 @@  static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt)
         }
         break;
     case AV_CODEC_ID_FLAC:
-        if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) {
+        if (side_data_size && mkv->track.bc) {
             uint8_t *old_extradata = par->extradata;
             if (side_data_size != par->extradata_size) {
                 av_log(s, AV_LOG_ERROR, "Invalid FLAC STREAMINFO metadata for output stream %d\n",
@@ -2229,8 +2229,7 @@  static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt)
     // FIXME: Remove the following once libaom starts propagating extradata during init()
     //        See https://bugs.chromium.org/p/aomedia/issues/detail?id=2012
     case AV_CODEC_ID_AV1:
-        if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live &&
-            !par->extradata_size) {
+        if (side_data_size && mkv->track.bc && !par->extradata_size) {
             AVIOContext *dyn_cp;
             uint8_t *codecpriv;
             int codecpriv_size;
@@ -2541,6 +2540,7 @@  static int mkv_write_trailer(AVFormatContext *s)
         if (ret < 0)
             return ret;
 
+        if (mkv->info.bc) {
         // update the duration
         av_log(s, AV_LOG_DEBUG, "end duration = %" PRIu64 "\n", mkv->duration);
         avio_seek(mkv->info.bc, mkv->duration_offset, SEEK_SET);
@@ -2549,6 +2549,7 @@  static int mkv_write_trailer(AVFormatContext *s)
                                     MATROSKA_ID_INFO, 0, 0, 0);
         if (ret < 0)
             return ret;
+        }
 
         if (mkv->track.bc) {
             // write Tracks master