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

[Ronald S. Bultje]

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

Ronald