[FFmpeg-devel] [PATCH] matroska : Fix writing packets to a non-seekable AVIOContext

[Aaron Colwell]

On Fri, Mar 9, 2012 at 11:03 AM, Michael Niedermayer <michaelni at gmx.at>wrote:

> On Fri, Mar 09, 2012 at 10:18:11AM -0800, Aaron Colwell wrote:
> > On Fri, Mar 9, 2012 at 8:46 AM, Michael Niedermayer <michaelni at gmx.at
> >wrote:
> >
> > > On Wed, Mar 07, 2012 at 03:47:43PM -0800, Aaron Colwell wrote:
> > > > diff --git a/ffserver.c b/ffserver.c
> > > > index bddcb7d..2b819d0 100644
> > > > --- a/ffserver.c
> > > > +++ b/ffserver.c
> > > > @@ -2380,6 +2380,7 @@ static int http_prepare_data(HTTPContext *c)
> > > >                          ret = ffio_open_dyn_packet_buf(&ctx->pb,
> > > > max_packet_size);
> > > >                      } else {
> > > >                          ret = avio_open_dyn_buf(&ctx->pb);
> > > > +                        ctx->pb->pos = c->data_count;
> > > >                      }
> > > >                      if (ret < 0) {
> > > >                          /* XXX: potential leak */
> > >
> > > doesnt this break when some muxer calls avio_seek() that cannot be
> > > satisfied within the buffer ?
> > >
> > >
> > I don't think this adds any broken behavior. The ctx->pb->seekable
> equals 0
> > so I believe that muxers are only allowed to seek to positions > pos. The
> > avio_seek() code doesn't appear to have any logic protecting itself
> against
> > seeks to a position < pos when seekable is 0. If a muxer seeks to a
> > position < pos when seekable == 0, I believe that is the sign of a broken
> > muxer. It is possible I am misunderstanding the code & API contract
> though.
> > Any further insights would be welcome.
>
> a muxer could try to seek and handle the failure. Or it could
> seek > pos.
> These would messup the dyn_buf state causing it to realloc to pos+size
> on the next write. And iam not sure what it would do after that but
> i dont think it would write the intended data.
>
>
Removed this line from the patch. It wasn't necessary and I agree that it
would likely have bad consequences.


> > > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > > > index 0b36725..400ac97 100644
> > > > --- a/libavformat/matroskaenc.c
> > > > +++ b/libavformat/matroskaenc.c
> > > > @@ -372,7 +372,7 @@ static int mkv_add_cuepoint(mkv_cues *cues, int
> > > stream,
> > > > int64_t ts, int64_t clus
> > > >      if (entries == NULL)
> > > >          return AVERROR(ENOMEM);
> > > >
> > > > -    if (ts < 0)
> > > > +    if (ts < 0 || cluster_pos < cues->segment_offset)
> > > >          return 0;
> > > >
> > > >      entries[cues->num_entries  ].pts = ts;
> > >
> > > why is this needed ?
> >
> >
> > This prevents invalid seek index entries from getting created. The
> cluster
> > file position must always be > the segment_offset because entries in the
> > cues element are always relative to the segment offset. Negative values
> are
> > not allowed because all clusters must be contained within the segment. I
> > suppose this could be submitted as a separate patch if you would like. I
> > just happened to notice this bug while I was looking at all the places
> > cluster_pos was being used.
>
> yes, please submit it as a seperate patch and explain when this case
> happens


Removed from this patch and I'll submit it separately once this one lands.

The new patch is included below.

Aaron


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

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 0b36725..8d024de 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -978,6 +978,7 @@ static int mkv_write_header(AVFormatContext *s)
     av_init_packet(&mkv->cur_audio_pkt);
     mkv->cur_audio_pkt.size = 0;
     mkv->audio_buffer_size  = 0;
+    mkv->cluster_pos = -1;

     avio_flush(pb);
     return 0;
@@ -1150,7 +1151,7 @@ static int mkv_write_packet_internal(AVFormatContext
*s, AVPacket *pkt)
         pb = mkv->dyn_bc;
     }

-    if (!mkv->cluster_pos) {
+    if (mkv->cluster_pos == -1) {
         mkv->cluster_pos = avio_tell(s->pb);
         mkv->cluster = start_ebml_master(pb, MATROSKA_ID_CLUSTER, 0);
         put_ebml_uint(pb, MATROSKA_ID_CLUSTERTIMECODE, FFMAX(0, ts));
@@ -1204,14 +1205,14 @@ static int mkv_write_packet(AVFormatContext *s,
AVPacket *pkt)

     // start a new cluster every 5 MB or 5 sec, or 32k / 1 sec for
streaming or
     // after 4k and on a keyframe
-    if (mkv->cluster_pos &&
+    if (mkv->cluster_pos != -1 &&
         ((!s->pb->seekable && (cluster_size > 32*1024 || ts >
mkv->cluster_pts + 1000))
          ||                      cluster_size > 5*1024*1024 || ts >
mkv->cluster_pts + 5000
          || (codec->codec_type == AVMEDIA_TYPE_VIDEO && keyframe &&
cluster_size > 4*1024))) {
         av_log(s, AV_LOG_DEBUG, "Starting new cluster at offset %" PRIu64
                " bytes, pts %" PRIu64 "\n", avio_tell(pb), ts);
         end_ebml_master(pb, mkv->cluster);
-        mkv->cluster_pos = 0;
+        mkv->cluster_pos = -1;
         if (mkv->dyn_bc)
             mkv_flush_dynbuf(s);
     }
@@ -1255,7 +1256,7 @@ static int mkv_write_trailer(AVFormatContext *s)
     if (mkv->dyn_bc) {
         end_ebml_master(mkv->dyn_bc, mkv->cluster);
         mkv_flush_dynbuf(s);
-    } else if (mkv->cluster_pos) {
+    } else if (mkv->cluster_pos != -1) {
         end_ebml_master(pb, mkv->cluster);
     }

-- 
1.7.7.3