"Ronald S. Bultje" <[email protected]> writes: > Hi guys, > > attached patches fix integer overflows in swscale as detected by IOC. > > As for #3, the one that fixes overflows in RGB48 by changing initFilter: > for certain types of filters, sums of coefficients at any intermediate > stage can be >1 (as long as individual coefficients are <=1), because > subsequent coefficients can be negative (bicubic, spline, lanczos). We > support that and it works fine, basically up to a maximum of 2 in any > intermediate (which is enough to cover anything). > > What initFilter did is to merge coefficients on edges, e.g. if there are 2 > coefficients for position -1 and 0 (assuming edge extension), then -1 and 0 > have the same pixel value, so we can also use coeffA+coeffB for pixel 0 and > add a zero as last coefficient to the end. This prevents overreads at the > line edges. The problem is that with spline/lanczos etc filters, the > intermediate sum of coefficients can be >1 at any point, so the > coeffA+coeffB can (and is) in some cases >1, which makes a single > coefficient >1. We don't support that for the vertical filter (the > horizontal one does it fine), and I don't think it's possible to support it > unless I decrease the number of bits precision within the filters. > > Therefore, this patch basically removes the merging at edges for the > vertical filter and instead duplicates lines in the 2D array by using the > same pointer (line[0]) for "line[-1]" as well as line[0]. This fixes the > integer overflows in the vertical filter. We don't copy data, just > pointers, and only at a per-line basis, so it has very little effect on > performance.
This explanation should go in the commit message. > The rest of the patches are straightforward. Please comment. This patch set changes the checksums for the rgb48 tests, suggesting the overflow was actually harmful. Have you verified the new output? > From 1ef4e41d40b68b6cdbc433df4f6932ca5ccf3587 Mon Sep 17 00:00:00 2001 > From: Ronald S. Bultje <[email protected]> > Date: Sun, 4 Dec 2011 15:05:51 -0800 > Subject: [PATCH] Fix add overflow in swscale. > > --- > libswscale/swscale.c | 17 +++++++++-------- > 1 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/libswscale/swscale.c b/libswscale/swscale.c > index 3d41b8d..a4c7e40 100644 > --- a/libswscale/swscale.c > +++ b/libswscale/swscale.c > @@ -754,8 +754,8 @@ yuv2rgb48_X_c_template(SwsContext *c, const int16_t > *lumFilter, > > for (i = 0; i < (dstW >> 1); i++) { > int j; > - int Y1 = 0; > - int Y2 = 0; > + int Y1 = -0x40000000; > + int Y2 = -0x40000000; > int U = -128 << 23; // 19 > int V = -128 << 23; > int R, G, B; > @@ -789,12 +789,13 @@ yuv2rgb48_X_c_template(SwsContext *c, const int16_t > *lumFilter, > B = U * c->yuv2rgb_u2b_coeff; > > // 8bit: 30 - 22 = 8bit, 16bit: 30bit - 14 = 16bit > - output_pixel(&dest[0], av_clip_uintp2(R_B + Y1, 30) >> 14); > - output_pixel(&dest[1], av_clip_uintp2( G + Y1, 30) >> 14); > - output_pixel(&dest[2], av_clip_uintp2(B_R + Y1, 30) >> 14); > - output_pixel(&dest[3], av_clip_uintp2(R_B + Y2, 30) >> 14); > - output_pixel(&dest[4], av_clip_uintp2( G + Y2, 30) >> 14); > - output_pixel(&dest[5], av_clip_uintp2(B_R + Y2, 30) >> 14); > +#define clip_intp2(a, b) av_clip(a, -(1 << (b - 1)), (1 << (b - 1)) - 1) > + output_pixel(&dest[0], 0x8000 + (clip_intp2(R_B + Y1, 30) >> 14)); > + output_pixel(&dest[1], 0x8000 + (clip_intp2( G + Y1, 30) >> 14)); > + output_pixel(&dest[2], 0x8000 + (clip_intp2(B_R + Y1, 30) >> 14)); > + output_pixel(&dest[3], 0x8000 + (clip_intp2(R_B + Y2, 30) >> 14)); > + output_pixel(&dest[4], 0x8000 + (clip_intp2( G + Y2, 30) >> 14)); > + output_pixel(&dest[5], 0x8000 + (clip_intp2(B_R + Y2, 30) >> 14)); > dest += 6; > } > } > -- > 1.7.2.1 We should put that clipping function in lavu like the others. > From 107ec830427f0a8f8e75787f4afa23f084e03029 Mon Sep 17 00:00:00 2001 > From: Ronald S. Bultje <[email protected]> > Date: Sun, 4 Dec 2011 14:08:03 -0800 > Subject: [PATCH] Fix overflow in swscale. > > --- > libswscale/swscale.c | 37 ++++++++++++++++++++++++++++++++++--- > libswscale/utils.c | 12 +++++++----- > 2 files changed, 41 insertions(+), 8 deletions(-) > > diff --git a/libswscale/swscale.c b/libswscale/swscale.c > index 975a0bd..3d41b8d 100644 > --- a/libswscale/swscale.c > +++ b/libswscale/swscale.c > @@ -2486,9 +2486,9 @@ static int swScale(SwsContext *c, const uint8_t* src[], > const int firstLumSrcY= vLumFilterPos[dstY]; //First line needed as > input > const int firstLumSrcY2= vLumFilterPos[FFMIN(dstY | > ((1<<c->chrDstVSubSample) - 1), dstH-1)]; > const int firstChrSrcY= vChrFilterPos[chrDstY]; //First line needed > as input > - int lastLumSrcY= firstLumSrcY + vLumFilterSize -1; // Last line > needed as input > - int lastLumSrcY2=firstLumSrcY2+ vLumFilterSize -1; // Last line > needed as input > - int lastChrSrcY= firstChrSrcY + vChrFilterSize -1; // Last line > needed as input > + int lastLumSrcY = FFMIN(c->srcH, firstLumSrcY + vLumFilterSize) > - 1; // Last line needed as input > + int lastLumSrcY2 = FFMIN(c->srcH, firstLumSrcY2 + vLumFilterSize) > - 1; // Last line needed as input > + int lastChrSrcY = FFMIN(c->chrSrcH, firstChrSrcY + vChrFilterSize) > - 1; // Last line needed as input > int enough_lines; > > //handle holes (FAST_BILINEAR & weird filters) > @@ -2580,10 +2580,41 @@ static int swScale(SwsContext *c, const uint8_t* > src[], > } > > { > + const int16_t *tmpYA[2][vLumBufSize], *tmpUV[2][vChrBufSize]; > const int16_t **lumSrcPtr= (const int16_t **) lumPixBuf + > lumBufIndex + firstLumSrcY - lastInLumBuf + vLumBufSize; > const int16_t **chrUSrcPtr= (const int16_t **) chrUPixBuf + > chrBufIndex + firstChrSrcY - lastInChrBuf + vChrBufSize; > const int16_t **chrVSrcPtr= (const int16_t **) chrVPixBuf + > chrBufIndex + firstChrSrcY - lastInChrBuf + vChrBufSize; > const int16_t **alpSrcPtr= (CONFIG_SWSCALE_ALPHA && alpPixBuf) ? > (const int16_t **) alpPixBuf + lumBufIndex + firstLumSrcY - lastInLumBuf + > vLumBufSize : NULL; > + > + if (firstLumSrcY < 0 || firstLumSrcY + vLumFilterSize > c->srcH) > { > + //printf("Overflow %d %d for [0, %d]\n", firstLumSrcY, > + // firstLumSrcY + vLumFilterSize - 1, c->srcH); These comments should obviously go away in the final commit. [...] > diff --git a/libswscale/utils.c b/libswscale/utils.c > index b644ed9..55500de 100644 > --- a/libswscale/utils.c > +++ b/libswscale/utils.c > @@ -182,7 +182,7 @@ static double getSplineCoeff(double a, double b, double > c, double d, double dist > > static int initFilter(int16_t **outFilter, int16_t **filterPos, int > *outFilterSize, int xInc, > int srcW, int dstW, int filterAlign, int one, int > flags, int cpu_flags, > - SwsVector *srcFilter, SwsVector *dstFilter, double > param[2]) > + SwsVector *srcFilter, SwsVector *dstFilter, double > param[2], int is_horizontal) > { > int i; > int filterSize; > @@ -459,6 +459,7 @@ static int initFilter(int16_t **outFilter, int16_t > **filterPos, int *outFilterSi > //FIXME try to align filterPos if possible > > //fix borders > + if (is_horizontal) { > for (i=0; i<dstW; i++) { > int j; > if ((*filterPos)[i] < 0) { > @@ -482,6 +483,7 @@ static int initFilter(int16_t **outFilter, int16_t > **filterPos, int *outFilterSi > (*filterPos)[i]= srcW - filterSize; > } > } > + } Needs reindent. In addition, this patch (or equivalent) is needed to avoid overflowing when the alpha values are left-shifted by 24: diff --git a/libswscale/swscale.c b/libswscale/swscale.c index 3804fd8..ab9a7bc 100644 --- a/libswscale/swscale.c +++ b/libswscale/swscale.c @@ -920,7 +920,7 @@ YUV2PACKED16WRAPPER(yuv2, rgb48, bgr48le, PIX_FMT_BGR48LE) static av_always_inline void yuv2rgb_write(uint8_t *_dest, int i, int Y1, int Y2, - int U, int V, int A1, int A2, + int U, int V, unsigned A1, unsigned A2, const void *_r, const void *_g, const void *_b, int y, enum PixelFormat target, int hasAlpha) { -- Måns Rullgård [email protected] _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
