diff mbox series

[FFmpeg-devel] avcodec/h263: Fix global-buffer-overflow with noout flag2 set

Message ID AM7PR03MB66604125D537B03F541A6D378F9E9@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit 9207dc3b0db368bb9cf5eb295cbc1129c2975e31
Headers show
Series [FFmpeg-devel] avcodec/h263: Fix global-buffer-overflow with noout flag2 set
Related show

Checks

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

Commit Message

Andreas Rheinhardt Nov. 21, 2021, 1:58 a.m. UTC
h263_get_motion_length() forgot to take an absolute value;
as a consequence, a negative index was used to access an array.
This leads to potential crashes, but mostly it just accesses what
is to the left of ff_mvtab (unless one uses ASAN), thereby defeating
the purpose of the AV_CODEC_FLAG2_NO_OUTPUT because the sizes of
the returned packets differ from the sizes the encoder would actually
have produced.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Do we need this AV_CODEC_FLAG2_NO_OUTPUT codepath in h263.h and
mpeg4videoenc.c at all? It seems to have never worked and the speed
difference to encoding with output is negligible. (And I have not even
investigated whether the checks for whether said flag is set impact
the performance of ordinary encoding.)

 libavcodec/h263.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Michael Niedermayer Nov. 21, 2021, 6:31 p.m. UTC | #1
On Sun, Nov 21, 2021 at 02:58:35AM +0100, Andreas Rheinhardt wrote:
> h263_get_motion_length() forgot to take an absolute value;
> as a consequence, a negative index was used to access an array.
> This leads to potential crashes, but mostly it just accesses what
> is to the left of ff_mvtab (unless one uses ASAN), thereby defeating
> the purpose of the AV_CODEC_FLAG2_NO_OUTPUT because the sizes of
> the returned packets differ from the sizes the encoder would actually
> have produced.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Do we need this AV_CODEC_FLAG2_NO_OUTPUT codepath in h263.h and
> mpeg4videoenc.c at all? It seems to have never worked and the speed
> difference to encoding with output is negligible. (And I have not even
> investigated whether the checks for whether said flag is set impact
> the performance of ordinary encoding.)

For 2 pass encoding you dont need the data from the first pass just the
amount.

thx

[...]
Andreas Rheinhardt Nov. 26, 2021, 6:43 a.m. UTC | #2
Andreas Rheinhardt:
> h263_get_motion_length() forgot to take an absolute value;
> as a consequence, a negative index was used to access an array.
> This leads to potential crashes, but mostly it just accesses what
> is to the left of ff_mvtab (unless one uses ASAN), thereby defeating
> the purpose of the AV_CODEC_FLAG2_NO_OUTPUT because the sizes of
> the returned packets differ from the sizes the encoder would actually
> have produced.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Do we need this AV_CODEC_FLAG2_NO_OUTPUT codepath in h263.h and
> mpeg4videoenc.c at all? It seems to have never worked and the speed
> difference to encoding with output is negligible. (And I have not even
> investigated whether the checks for whether said flag is set impact
> the performance of ordinary encoding.)
> 
>  libavcodec/h263.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/h263.h b/libavcodec/h263.h
> index 70fd1ffdc0..d6bef8318d 100644
> --- a/libavcodec/h263.h
> +++ b/libavcodec/h263.h
> @@ -100,15 +100,16 @@ void ff_h263_encode_motion(PutBitContext *pb, int val, int f_code);
>  
>  
>  static inline int h263_get_motion_length(int val, int f_code){
> -    int l, bit_size, code;
> +    int bit_size, code, sign;
>  
>      if (val == 0) {
>          return 1; /* ff_mvtab[0][1] */
>      } else {
>          bit_size = f_code - 1;
>          /* modulo encoding */
> -        l= INT_BIT - 6 - bit_size;
> -        val = (val<<l)>>l;
> +        val  = sign_extend(val, 6 + bit_size);
> +        sign = val >> 31;
> +        val  = (val ^ sign) - sign; /* val = FFABS(val) */
>          val--;
>          code = (val >> bit_size) + 1;
>  
> 

Will apply this later tonight unless there are objections.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/h263.h b/libavcodec/h263.h
index 70fd1ffdc0..d6bef8318d 100644
--- a/libavcodec/h263.h
+++ b/libavcodec/h263.h
@@ -100,15 +100,16 @@  void ff_h263_encode_motion(PutBitContext *pb, int val, int f_code);
 
 
 static inline int h263_get_motion_length(int val, int f_code){
-    int l, bit_size, code;
+    int bit_size, code, sign;
 
     if (val == 0) {
         return 1; /* ff_mvtab[0][1] */
     } else {
         bit_size = f_code - 1;
         /* modulo encoding */
-        l= INT_BIT - 6 - bit_size;
-        val = (val<<l)>>l;
+        val  = sign_extend(val, 6 + bit_size);
+        sign = val >> 31;
+        val  = (val ^ sign) - sign; /* val = FFABS(val) */
         val--;
         code = (val >> bit_size) + 1;