[FFmpeg-devel] [PATCH] EA ADPCM R1, R2 and R3
Michael Niedermayer
michaelni
Wed Oct 24 04:04:54 CEST 2007
Hi
On Wed, Oct 24, 2007 at 12:30:21AM +0200, Aurelien Jacobs wrote:
> On Tue, 23 Oct 2007 00:50:06 +0200
> Michael Niedermayer <michaelni at gmx.at> wrote:
>
> > Hi
> >
> > On Mon, Oct 22, 2007 at 12:05:53AM +0200, Aurelien Jacobs wrote:
> > > On Sat, 20 Oct 2007 20:11:58 +0200
> > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > >
> > > > Hi
> > > >
> > > > On Sat, Oct 20, 2007 at 03:42:35PM +0200, Aurelien Jacobs wrote:
> > > > > Hi,
> > > > >
> > > > > Attached patch adds support for EA ADPCM R1, R2 and R3.
> > > > > It's based on the patch from Peter Ross.
> > > > > I intend to apply it soon.
> > > > >
> > > > > Aurel
> > > >
> > > > [...]
> > > > > @@ -637,9 +638,12 @@
> > > > > {
> > > > > ADPCMContext *c = avctx->priv_data;
> > > > >
> > > > > - if(avctx->channels > 2U){
> > > > > + if ((avctx->channels > 2
> > > > > + && avctx->codec->id != CODEC_ID_ADPCM_EA_R1
> > > > > + && avctx->codec->id != CODEC_ID_ADPCM_EA_R2
> > > > > + && avctx->codec->id != CODEC_ID_ADPCM_EA_R3)
> > > > > + || avctx->channels > 6)
> > > > > return -1;
> > > > > - }
> > > >
> > > > this is wrong, channels is signed so it must be 2U not 2
> > >
> > > OK.
> > >
> > > > also this is not the most readable way to check this especially if more
> > > > codecs get added ...
> > >
> > > I agree. After a quick look, it seems this check is here to prevent
> > > overflow of the status[] context var. As this patch increase the size
> > > of status[] to 6, I think it's safe to simply check for channels > 6U.
> > > I hope I'm right here ?
> >
> > no, quick look is not enough, you (or me) would have to check all adpcm
> > codecs for channel related security issues, maybe there are none but i
> > dont feel good with removing this check after a quick look
>
> Right, but I had a small hope that you might want to check all adpcm
> codecs yourself... ;-)
> Anyway, I now had a deeper look, and it didn't took me very long to
> find several places where channels > 2 would cause security issues.
> So it seems we will need to list all codecs which support more than
> 2 channels in one way or another. I modified original patch to use
> a switch as suggested by Peter, but in a slightly cleaner way.
> Is this OK ?
>
> > also i dont think the codecs will work with more than 2 channels after
> > this check is removed ...
>
> Right. No other adpcm codec will work with more than 2 channels.
>
> > > > > c->channel = 0;
> > > > > c->status[0].predictor = c->status[1].predictor = 0;
> > > > > @@ -1182,6 +1186,78 @@
> > > > > }
> > > > > }
> > > > > break;
> > > > > + case CODEC_ID_ADPCM_EA_R1:
> > > > > + case CODEC_ID_ADPCM_EA_R2:
> > > > > + case CODEC_ID_ADPCM_EA_R3: {
> > > > > + /* channel numbering
> > > > > + 2chan: 0=fl, 1=fr
> > > > > + 4chan: 0=fl, 1=rl, 2=fr, 3=rr
> > > > > + 6chan: 0=fl, 1=c, 2=fr, 3=rl, 4=rr, 5=sub */
> > > > > + const int big_endian = avctx->codec->id == CODEC_ID_ADPCM_EA_R3;
> > > > > + unsigned int channel;
> > > > > + uint16_t *samplesC;
> > > > > + uint8_t *srcC;
> > > > > +
> > > > > + samples_in_chunk = (big_endian ? bytestream_get_be32(&src)
> > > > > + : bytestream_get_le32(&src)) / 28;
> > > > > +
> > > > > + for (channel=0; channel<avctx->channels; channel++) {
> > > > > + srcC = src + (big_endian ? bytestream_get_be32(&src)
> > > > > + : bytestream_get_le32(&src))
> > > > > + + (avctx->channels-channel-1) * 4;
> > > > > + samplesC = samples + channel;
> > > > > +
> > > > > + if (avctx->codec->id == CODEC_ID_ADPCM_EA_R1) {
> > > > > + current_left_sample = (int16_t)bytestream_get_le16(&srcC);
> > > > > + previous_left_sample = (int16_t)bytestream_get_le16(&srcC);
> > > > > + } else {
> > > > > + current_left_sample = c->status[channel].predictor;
> > > > > + previous_left_sample = c->status[channel].prev_sample;
> > > > > + }
> > > > > +
> > > > > + for (count1=0; count1<samples_in_chunk; count1++) {
> > > > > + if (*srcC == 0xEE) { /* only seen in R2 and R3 */
> > > > > + srcC++;
> > > > > + current_left_sample = (int16_t)bytestream_get_be16(&srcC);
> > > > > + previous_left_sample = (int16_t)bytestream_get_be16(&srcC);
> > > > > +
> > > > > + for (count2=0; count2<28; count2++) {
> > > > > + *samplesC = (int16_t)bytestream_get_be16(&srcC);
> > > > > + samplesC += avctx->channels;
> > > > > + }
> > > >
> > > > completely missing checks for array bounds, this should be exploitable
> > >
> > > Right. Check added at the start of this decoding code.
> >
> > have you considered that the *channels in this check can overflow and thus
> > the check would fail ...
>
> Oh, indeed, it could overflow !
> New check should really prevent all kind of possible overflow.
patch looks ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/20071024/9f0049b2/attachment.pgp>
More information about the ffmpeg-devel
mailing list