[FFmpeg-devel] [PATCH] make packet_size in AVFormatContext unsigned

[Michael Niedermayer]

On Mon, Jun 15, 2009 at 06:05:01PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Mon, Jun 15, 2009 at 5:49 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
> > On Mon, Jun 15, 2009 at 05:24:37PM -0400, Ronald S. Bultje wrote:
> >> On Mon, Jun 15, 2009 at 4:39 PM, Ronald S. Bultje<rsbultje at gmail.com> wrote:
> >> > On Mon, Jun 15, 2009 at 4:10 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
> >> >> the patch is changing a field to unsiged without you explaining why
> >>
> >> Oh right, I forgot the "why": to move ASF's packet_size value into
> >> AVFormatContext, it needs to be unsigned. You requested that, since
> >> ASFContext->packet_size is unsigned, but AVFormatcontext->.. is
> >> signed, therefore moving the value from one to the other is a
> >> potential security risk. We both then agreed it was silly that
> >> AVFormatcontext->.. was signed anyway, since any value <0 is
> >> meaningless, and thus here's my attempt to make it unsigned.
> >
> > yes it of coure should be unsigned but i need to check all uses and i
> > want to do it just once not repeatly point at bugs it introduces
> > and you say it does introduce a bug if i understood you correctly ...
> 
> It might, but let's be more exact: it basically makes an existing bug
> more severe.
> 
> So in mpegenc.c, ctx->packet_size is used once, to set
> MpegEncContext->packet_size (default=2048). There are no further value
> checks, so currently you can set a negative packet_size and it would
> try to run. These I call existing bugs. This
> MpegEncContext->packet_size value is used later as denominator in a
> division, to calculate buffer position and to calculate zero padding.
> In each of these cases, there are currently chances that other
> integers flip sign or over/underflow, i.e. existing potential bugs.
> 
> My patch currently assigns an unsigned value into
> MpegEncContext->packet_size (signed), which is clearly buggy. I did
> that because I think that in addition to making packet_size unsigned
> in MpegEncContext (I think that'd be a good thing), we should also set
> min/max values that this variable can take, and error out otherwise.
> Right now, it might introduce more bugs because the value becomes
> unsigned and can thus be even bigger, but these are just exaggerations
> of existing bugs.
> 
> Looking at all this, I wonder: what if I just copy (instead of move)
> AsfDemuxContext->packet_size into AVFormatContext->packet_size, and
> leave everything else as-is? you don't seem happy to review this
> patch..

You repeatly say it introduces or worsens a bug, dont you realize that
this effectively rejects your patch, theres no point in me reviewing it
if you, the author knows its not bugfree ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20090616/2155747b/attachment.pgp>