[FFmpeg-devel] [PATCH] add AVCodecContext field to specify desired number of channels

[Michael Niedermayer]

Hi

On Tue, Aug 21, 2007 at 08:48:07AM +0200, Benoit Fouet wrote:
> Michael Niedermayer wrote:
> > Hi
> >
> > On Mon, Aug 20, 2007 at 11:28:37PM +0200, Reimar D?ffinger wrote:
> >   
> >> Hello,
> >> On Mon, Aug 20, 2007 at 09:20:29PM +0100, M?ns Rullg?rd wrote:
> >>     
> >>> Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:
> >>>       
> >>>> On Mon, Aug 20, 2007 at 07:37:47PM +0200, Michael Niedermayer wrote:
> >>>>         
> >>>>> On Mon, Aug 20, 2007 at 04:53:26PM +0200, Reimar D?ffinger wrote:
> >>>>>           
> >> [...]
> >>     
> >>>>>> @@ -753,6 +753,7 @@ static const AVOption options[]={
> >>>>>>  {"timecode_frame_start", "GOP timecode frame start number, in non drop frame format", OFFSET(timecode_frame_start), FF_OPT_TYPE_INT, 0, 0, INT_MAX, V|E},
> >>>>>>  {"drop_frame_timecode", NULL, 0, FF_OPT_TYPE_CONST, CODEC_FLAG2_DROP_FRAME_TIMECODE, INT_MIN, INT_MAX, V|E, "flags2"},
> >>>>>>  {"non_linear_q", "use non linear quantizer", 0, FF_OPT_TYPE_CONST, CODEC_FLAG2_NON_LINEAR_QUANT, INT_MIN, INT_MAX, V|E, "flags2"},
> >>>>>> +{"request_channels", "set desired number of audio channels", OFFSET(request_channels), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX, A|D},
> >>>>>>             
> >>>>> i think the max/min can be improved
> >>>>> 5000 channels is not sane neither is -3 channels
> >>>>>           
> >>>> I agree, but I didn't want to "invent" some numbers and those are the
> >>>> values that are used for "channels".
> >>>>         
> >>> I think you can safely exclude negative numbers.  Choosing an upper
> >>> limit is admittedly more arbitrary.
> >>>       
> >> sure, 0 instead of INT_MIN then. Is INT_MAX okay? And is there any
> >>     
> >
> > well id set max to 100 or so ...
> >
> >
> >   
> 
> i don't see why 100 is better than INT_MAX.
> why do you want to choose an arbitrary value ?

id like to limit variables to a sane range in general because of code like

x= av_malloc(channels * sizeof(mystruct));
for(i=0; i<channels; i++){
    x[i].something= read();
    if(read()){
        av_log("error unsupported blah");
        return -1;
    }
}
INT_MAX is 0x7FFFFFFF, lets assume channels is 0x40000001
sizeof(mystruct) == 4
channels * sizeof(mystruct) == 4
av_malloc will allocate 4 bytes
the for loop now will write over the end of the array and an amount
selected through the input data

of course limiting the stuff in the AVOptions isnt really solving this
as they would only affect things read from the command line, so iam
fine with leaving it at INT_MAX ..

and yes all such code should check that channels is not too large but
not all such code does ...
limiting channels at a few strategic points might prevent some exploits
if checks have been forgotten

[...]
-- 
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/20070821/fb58bdea/attachment.pgp>