[FFmpeg-devel] [PATCH] alac : check readsamplesize

[Jai Menon]

Hi,

-------- Original-Nachricht --------
> Datum: Mon, 14 Jul 2008 11:34:12 +0200
> Von: matthieu castet <castet.matthieu at free.fr>
> An: FFmpeg development discussions and patches <ffmpeg-devel at mplayerhq.hu>
> Betreff: Re: [FFmpeg-devel] [PATCH] alac : check readsamplesize

> Jai Menon wrote:
> > Hi,
> > 
> >> Hi,
> >>
> >> Jai Menon wrote:
> >>> Hi,
> >>>> check readsamplesize in alac, with the current code, it could be 
> >>>> negative or bigger than 17 (and get_bits will fail).
> >>>>
> >>> And what about 24-bit alac streams? not to mention multi-channel alac
> if
> >> possible
> >>> that is...
> >> ATM ffmpeg decoder doesn't support 24-bit stream, nor multi-channel :
> >> - get_bits will return garbage if readsamplesize > 17
> > 
> > So isn't it better if we just check if channels>2 or wasted_bytes is
> nonzero
> if channels = 2, wasted_bytes = 0, alac->setinfo_sample_size = 32
> I got readsamplesize = 33. get_bits will fail, extend_sign32 will fail
> 

Ok, I didn't realise that such samples are actually possible. As a side note, which 
tool/software do you use to generate alac streams?

> 
> > 
> >> - in the 24-bit stream I saw on mplayer sample, wasted_bytes is used,
> so 
> >> readsamplesize could be less than 17, but our decoder doesn't handle it
> >> correctly
> >> - for not 16-bit stream, we got 'FIXME: unimplemented sample size'.
> >> - MAX_CHANNELS is 2
> >>
> >> So until we implement correct support for multi-channel and other bits 
> >> per sample, I think this check is safe.
> >>
> > 
> > Hmm..couldn't we just add a check to see if the stream is multichannel 
> Already done.
> 

Yeah, I don't have access to the source right now, above comment was based on (bad)memory

> > or > 16 bits sample size 
> Done only at the end of decoding
> 
> > and return an error from decode_frame instead of checking each value
> > explicitly? 
> What do you mean by "instead of checking each value explicitly"
> 

What I meant is to ask is wouldn't it be better if we could just recognise invalid files which
quicktime/itunes couldn't possibly have generated, and die with an error from the decoder...

> 
> 
> PS : note that with the current code channels = 1, wasted_bytes = 3, 
> alac->setinfo_sample_size = 16, will give readsamplesize = -8.
> readsamplesize is only use for reading, so it should be exploitable, but 
> this will give some segdefault instead of clean failure...

Yes, and clean failure is good. 
My point is just that maybe we could actually look into the 
possibility of some combination of "parameters" which are totally invalid. These could be
rejected outright and then we don't need to make redundant checks on valid samples.

Again, my interest here is purely to see if any of these sanity checks make sense from
the encoder's perspective, which I'm in the process of writing as part od soc.

Regards,

Jai Menon
<realityman at gmx.net>

-- 
GMX startet ShortView.de. Hier findest Du Leute mit Deinen Interessen!
Jetzt dabei sein: http://www.shortview.de/wasistshortview.php?mc=sv_ext_mf at gmx