| 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 | expand |
| 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 |
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: > 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 --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;
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(-)