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

[Michael Niedermayer]

On Mon, Mar 12, 2012 at 02:43:59PM -0700, Aaron Colwell wrote:
> 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.

patch applied

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120314/9c63e76f/attachment.asc>