[FFmpeg-devel] [PATCH] oops I broke rdt.c

[Michael Niedermayer]

On Tue, Dec 16, 2008 at 01:38:10PM -0500, Ronald S. Bultje wrote:
> Hi Michael,
> 
> On Tue, Dec 16, 2008 at 1:03 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> p,,[
> > this sounds fragile, someone just needs to move some inocent looking
> > code around or find a way that does not reset the value after
> > _read_mdpr()
> >
> > please decide what the valid range of st->id can be
> > and enforce it both in code writing it as well as ensuring that
> > all code using it does not blow up with values within that range
> 
> I'll add checks for that.
> 
> > besides, thinking about it, your code looks just wrong
> > the array size of MAX_STREAMS just is in no way related to a 16bit
> > id read from the file.
> > are you sure you are not misusing the variable.
> > each variable has one and only one semantic meaning, either it is a
> > stream index or a 16bit id it can absolutely not be both nor
> > can that change after a function call. This is a recipe for a
> > unmaintainable mess
> 
> st->id is always the index number of that stream in the media (as
> opposed to in the array of AVFormatContext->streams, that is
> st->index). In .rm files, this is a 16-bit integer. In RTSP, the value
> was unused.
> 
> Therefore, for RDT, I decided to use it to specify the index of the
> stream within the "set of streams with identical content". I can make
> a new variable for that but it sounded kind of unnecessary. st->id is
> ensured (I can assert() that if you wish) to be st->index -
> first_st_of_set->index and therefore automatically is always smaller
> than MAX_STREAMS, and is more specifically within the range of 0 and
> RDTDemuxContext->n_streams. I think the assert() would be the easiest
> way to ensure this / make it clear without making the code more
> complex. If you're affraid that RTSP might some day start using
> st->id, then I'll make a new variable or just use st->index -
> first_st_of_set->index.

this sounds fragile.
st->id is 2 different things in one file (rmdec.c)
first it is a 16bit value that is in no way checked
second it is a security critical value that has to be < MAX_STREAMS

a assert() does not prevent an exploit, an assert() is only enabled
when NDEBUG is not set


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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081217/a670ea59/attachment.pgp>