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

[Aaron Colwell]

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.


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

Aaron