| Message ID | 20220817203147.1957322-2-martin@martin.st |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/2] checkasm: sw_scale: Fix the difference printing for approximate functions | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Hi, On Wed, Aug 17, 2022 at 4:32 PM Martin Storsjö <martin@martin.st> wrote: > This avoids overflows on some inputs in the x86 case, where the > assembly version would clip/overflow differently from the > C reference function. > > This doesn't seem to be a real issue with actual input data, but > only with the previous fully random input data. > I'm a bit scared of this change... If we can trigger overflows with specific pixel patterns, doesn't that make FFmpeg input-data exploitable? Think of how that would go with corporate users with user-provided input data. Ronald
On Wed, 17 Aug 2022, Ronald S. Bultje wrote: > On Wed, Aug 17, 2022 at 4:32 PM Martin Storsjö <martin@martin.st> wrote: > This avoids overflows on some inputs in the x86 case, where the > assembly version would clip/overflow differently from the > C reference function. > > This doesn't seem to be a real issue with actual input data, but > only with the previous fully random input data. > > > I'm a bit scared of this change... If we can trigger overflows with specific > pixel patterns, doesn't that make FFmpeg input-data exploitable? Think of > how that would go with corporate users with user-provided input data. No, this most probably isn't a real issue with actual filters - it's only that the current checkasm test was overly simplistic. The input to this DSP function isn't raw user-provided input pixels, but 16 bit integers produced as the output of the first (horizontal) scaling step. Yesterday when I wrote the patch, I hadn't checked exactly what the range of those values were, and I assumed it wasn't the whole int16_t range - but apparently they can range at least up to max int16_t. (They most probably can't range down to the minimum negative int16_t though - simulating that aspect would be nice too.) The filter coefficients should add up to 4096. The input sample range is 15 bits (plus sign), and filter coefficients add a total magnitude of 12 bits, giving a total range of 27 bits (plus sign). After shifting down by 19 bits at the end, this produces 8 bits output (which is clipped). The critical stage here is the 27 bits, where there's still plenty of headroom (4 bits) for filter overshoot - with a real filter. However in the current test, the filter coefficients are just plain random int16_t values in the whole range - and that can easily cause overflows in the intermediates. So I guess we shouldn't scale down the input "pixels" here as they actually can use the whole range up to max int16_t (but ideally, they wouldn't range further down below zero than what you'd get from the maximal negative filter overshoot either), but we should scale down the fully random filter coefficients, so that they can't overflow even if they all happen to align in the worst way. Alternatively we could construct a more realistic test filter, e.g. something like what's used in the hscale test. There, if the filter should add up to 1<<F, and we have N filter coefficients, we have all of them but one be set to -((1<<F)/(N-1)) and one set to ((1<<(F+1)) - 1). It doesn't look much like a real actual filter, but keeps most properties - it adds up to the right sum and doesn't trigger unreal overflows and it produces both positive and negative values. Anyway, I'll update the patch and make a clearer comment for it. // Martin
diff --git a/tests/checkasm/sw_scale.c b/tests/checkasm/sw_scale.c index d72506ed86..89403da317 100644 --- a/tests/checkasm/sw_scale.c +++ b/tests/checkasm/sw_scale.c @@ -187,8 +187,16 @@ static void check_yuv2yuvX(int accurate) } *vFilterData; uint8_t d_val = rnd(); memset(dither, d_val, LARGEST_INPUT_SIZE); + randomize_buffers((uint8_t*)src_pixels, LARGEST_FILTER * LARGEST_INPUT_SIZE * sizeof(int16_t)); randomize_buffers((uint8_t*)filter_coeff, LARGEST_FILTER * sizeof(int16_t)); + // Limit the range of the filter coefficients and intermediate + // pixel values, to avoid risk of clipping filter intermediates on x86. + for (i = 0; i < LARGEST_FILTER; i++) + filter_coeff[i] >>= 2; + for (i = 0; i < LARGEST_FILTER * LARGEST_INPUT_SIZE; i++) + src_pixels[i] >>= 2; + ctx = sws_alloc_context(); if (accurate) ctx->flags |= SWS_ACCURATE_RND;
This avoids overflows on some inputs in the x86 case, where the assembly version would clip/overflow differently from the C reference function. This doesn't seem to be a real issue with actual input data, but only with the previous fully random input data. This also makes the test produce a bit more realistic output pixel values, instead of having essentially all pixels clipped to either 0 or 255. Signed-off-by: Martin Storsjö <martin@martin.st> --- tests/checkasm/sw_scale.c | 8 ++++++++ 1 file changed, 8 insertions(+)