diff mbox series

[FFmpeg-devel,4/4] avcodec/osq: avoid using too large numbers for shifts and integers in update_residue_parameter()

Message ID 20230915131147.5945-4-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/4] avcodec/evc_ps: Check cpb_cnt_minus1 and propagate error | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer Sept. 15, 2023, 1:11 p.m. UTC
The code should be changed to not use floats in the VLC parameters
This patch merely fixes undefined behavior

Fixes: 2.96539e+09 is outside the range of representable values of type 'int'
Fixes: Assertion n>=0 && n<=32 failed at libavcodec/get_bits.h:423
Fixes: 62241/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_OSQ_fuzzer-4525761925873664

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/osq.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Paul B Mahol Sept. 15, 2023, 1:54 p.m. UTC | #1
On Fri, Sep 15, 2023 at 3:12 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> The code should be changed to not use floats in the VLC parameters
> This patch merely fixes undefined behavior
>
> Fixes: 2.96539e+09 is outside the range of representable values of type
> 'int'
> Fixes: Assertion n>=0 && n<=32 failed at libavcodec/get_bits.h:423
> Fixes:
> 62241/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_OSQ_fuzzer-4525761925873664
>


NAK

Breaks decoding.


>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/osq.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/osq.c b/libavcodec/osq.c
> index e2a84657fb4..e7f11691d2e 100644
> --- a/libavcodec/osq.c
> +++ b/libavcodec/osq.c
> @@ -152,11 +152,15 @@ static int update_residue_parameter(OSQChannel *cb)
>
>      sum = cb->sum;
>      x = sum / cb->count;
> -    rice_k = av_ceil_log2(x);
> +    rice_k = ceil(log2(x));
>      if (rice_k >= 30) {
> -        rice_k = floor(sum / 1.4426952 + 0.5);
> -        if (rice_k < 1)
> +        double f = floor(sum / 1.4426952 + 0.5);
> +        if (f <= 1) {
>              rice_k = 1;
> +        } else if (f >= 31) {
> +            rice_k = 31;
> +        } else
> +            rice_k = f;
>      }
>
>      return rice_k;
> --
> 2.17.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer Sept. 15, 2023, 2:38 p.m. UTC | #2
On Fri, Sep 15, 2023 at 03:54:19PM +0200, Paul B Mahol wrote:
> On Fri, Sep 15, 2023 at 3:12 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > The code should be changed to not use floats in the VLC parameters
> > This patch merely fixes undefined behavior
> >
> > Fixes: 2.96539e+09 is outside the range of representable values of type
> > 'int'
> > Fixes: Assertion n>=0 && n<=32 failed at libavcodec/get_bits.h:423
> > Fixes:
> > 62241/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_OSQ_fuzzer-4525761925873664
> >
> 
> 
> NAK
> 
> Breaks decoding.

Please provide sample that works before and fails after this.

That said, it has to be pointed out that the current code in osq is
buggy in multiply ways. The use of floats for computing vlc parameters is
not portable (unless theres alot of luck).

thx


[....]
diff mbox series

Patch

diff --git a/libavcodec/osq.c b/libavcodec/osq.c
index e2a84657fb4..e7f11691d2e 100644
--- a/libavcodec/osq.c
+++ b/libavcodec/osq.c
@@ -152,11 +152,15 @@  static int update_residue_parameter(OSQChannel *cb)
 
     sum = cb->sum;
     x = sum / cb->count;
-    rice_k = av_ceil_log2(x);
+    rice_k = ceil(log2(x));
     if (rice_k >= 30) {
-        rice_k = floor(sum / 1.4426952 + 0.5);
-        if (rice_k < 1)
+        double f = floor(sum / 1.4426952 + 0.5);
+        if (f <= 1) {
             rice_k = 1;
+        } else if (f >= 31) {
+            rice_k = 31;
+        } else
+            rice_k = f;
     }
 
     return rice_k;