[FFmpeg-devel,1/2] avcodec/diracdec: Check perspective_exp and zrs_exp.

Submitted by Ronald S. Bultje on Aug. 16, 2017, 5:47 p.m.

Details

Message ID CAEEMt2kKSs8J8PQ8KTRTH-Ap6TJ8=x+xFeN-3e_d5fvg9Cai9A@mail.gmail.com
State New
Headers show

Commit Message

Ronald S. Bultje Aug. 16, 2017, 5:47 p.m.
Hi,

On Aug 15, 2017 3:32 AM, "Michael Niedermayer" <michael@niedermayer.cc>
wrote:

Fixes: undefined shift
Fixes: runtime error: shift exponent 264 is too large for 32-bit type 'int'
Fixes: 2860/clusterfuzz-testcase-minimized-4672811689836544

Found-by: continuous fuzzing process https://github.com/google/oss-
fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/diracdec.c | 5 +++++
 1 file changed, 5 insertions(+)

This message is utterly unhelpful. Why is it there? Please don't waste
binary size with crap logs.

Ronald

Comments

Michael Niedermayer Aug. 16, 2017, 7:50 p.m.
On Wed, Aug 16, 2017 at 01:47:55PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Aug 15, 2017 3:32 AM, "Michael Niedermayer" <michael@niedermayer.cc>
> wrote:
> 
> Fixes: undefined shift
> Fixes: runtime error: shift exponent 264 is too large for 32-bit type 'int'
> Fixes: 2860/clusterfuzz-testcase-minimized-4672811689836544
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-
> fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/diracdec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavcodec/diracdec.c b/libavcodec/diracdec.c
> index f2837aca69..be8b282314 100644
> --- a/libavcodec/diracdec.c
> +++ b/libavcodec/diracdec.c
> @@ -1161,6 +1161,11 @@ static int
> dirac_unpack_prediction_parameters(DiracContext
> *s)
>                  s->globalmc[ref].perspective[0]  = dirac_get_se_golomb(gb);
>                  s->globalmc[ref].perspective[1]  = dirac_get_se_golomb(gb);
>              }
> +            if (s->globalmc[ref].perspective_exp +
> (uint64_t)s->globalmc[ref].zrs_exp > 30) {
> +                av_log(s->avctx, AV_LOG_ERROR, "exp %d %d too large\n",
> s->globalmc[ref].perspective_exp, s->globalmc[ref].zrs_exp);
> 
> 
> This message is utterly unhelpful. Why is it there? Please don't waste
> binary size with crap logs.

If an error occurs, the user should be presented with an error message.
Its also helpfull for debuging to know where an error came from.

But considering this came up in the past and people always insisted
that error messages are removed at any cost. Ill just remove it

thx

[...]
--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.

Patch hide | download patch | download mbox

diff --git a/libavcodec/diracdec.c b/libavcodec/diracdec.c
index f2837aca69..be8b282314 100644
--- a/libavcodec/diracdec.c
+++ b/libavcodec/diracdec.c
@@ -1161,6 +1161,11 @@  static int
dirac_unpack_prediction_parameters(DiracContext
*s)
                 s->globalmc[ref].perspective[0]  = dirac_get_se_golomb(gb);
                 s->globalmc[ref].perspective[1]  = dirac_get_se_golomb(gb);
             }
+            if (s->globalmc[ref].perspective_exp +
(uint64_t)s->globalmc[ref].zrs_exp > 30) {
+                av_log(s->avctx, AV_LOG_ERROR, "exp %d %d too large\n",
s->globalmc[ref].perspective_exp, s->globalmc[ref].zrs_exp);