[FFmpeg-devel,1/2] avutil/internal: Add ff_elog()

Submitted by Michael Niedermayer on Feb. 22, 2017, 6:16 p.m.

Details

Message ID 20170222181647.12076-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Feb. 22, 2017, 6:16 p.m.
This enables the extra error messages in case of DEBUG or high assrtion levels.
High assertion levels imply slow checks in inner loops so any extra error should
be insignificant.
Is it preferred to have a separate switch for ff_elog() so it doesnt depend on
DEBUG/assertion level ?

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavutil/internal.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Paul B Mahol Feb. 23, 2017, 7:08 a.m.
On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> This enables the extra error messages in case of DEBUG or high assrtion
> levels.
> High assertion levels imply slow checks in inner loops so any extra error
> should
> be insignificant.
> Is it preferred to have a separate switch for ff_elog() so it doesnt depend
> on
> DEBUG/assertion level ?
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavutil/internal.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/libavutil/internal.h b/libavutil/internal.h
> index 7780a9a791..208f8f474f 100644
> --- a/libavutil/internal.h
> +++ b/libavutil/internal.h
> @@ -262,6 +262,12 @@ void avpriv_request_sample(void *avc,
>  #   define ff_dlog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_DEBUG,
> __VA_ARGS__); } while (0)
>  #endif
>
> +#if defined(DEBUG) || ASSERT_LEVEL > 1
> +#   define ff_elog(ctx, ...) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__)
> +#else
> +#   define ff_elog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_ERROR,
> __VA_ARGS__); } while (0)
> +#endif
> +

I guess this is fine.
wm4 Feb. 23, 2017, 7:19 a.m.
On Wed, 22 Feb 2017 19:16:46 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> This enables the extra error messages in case of DEBUG or high assrtion levels.
> High assertion levels imply slow checks in inner loops so any extra error should
> be insignificant.
> Is it preferred to have a separate switch for ff_elog() so it doesnt depend on
> DEBUG/assertion level ?
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavutil/internal.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/libavutil/internal.h b/libavutil/internal.h
> index 7780a9a791..208f8f474f 100644
> --- a/libavutil/internal.h
> +++ b/libavutil/internal.h
> @@ -262,6 +262,12 @@ void avpriv_request_sample(void *avc,
>  #   define ff_dlog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_DEBUG, __VA_ARGS__); } while (0)
>  #endif
>  
> +#if defined(DEBUG) || ASSERT_LEVEL > 1
> +#   define ff_elog(ctx, ...) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__)
> +#else
> +#   define ff_elog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__); } while (0)
> +#endif
> +
>  // For debuging we use signed operations so overflows can be detected (by ubsan)
>  // For production we use unsigned so there are no undefined operations
>  #ifdef CHECKED

Not really a fan of this. (And neither ff_dlog.) But I guess I don't
mean to stop you.

In my opinion, code that checks for overflow and fuzz issues should be
as small and unintrusive as possible, or preferably not exist.

It seems like all these fuzz fixes make code less readable. (Not just
because of the additional logging and control flow, also because of all
those unobvious tricky checks.)
Ronald S. Bultje Feb. 23, 2017, 12:57 p.m.
Hi,

On Thu, Feb 23, 2017 at 2:19 AM, wm4 <nfxjfg@googlemail.com> wrote:

> On Wed, 22 Feb 2017 19:16:46 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
>
> > This enables the extra error messages in case of DEBUG or high assrtion
> levels.
> > High assertion levels imply slow checks in inner loops so any extra
> error should
> > be insignificant.
> > Is it preferred to have a separate switch for ff_elog() so it doesnt
> depend on
> > DEBUG/assertion level ?
> >
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavutil/internal.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/libavutil/internal.h b/libavutil/internal.h
> > index 7780a9a791..208f8f474f 100644
> > --- a/libavutil/internal.h
> > +++ b/libavutil/internal.h
> > @@ -262,6 +262,12 @@ void avpriv_request_sample(void *avc,
> >  #   define ff_dlog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_DEBUG,
> __VA_ARGS__); } while (0)
> >  #endif
> >
> > +#if defined(DEBUG) || ASSERT_LEVEL > 1
> > +#   define ff_elog(ctx, ...) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__)
> > +#else
> > +#   define ff_elog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_ERROR,
> __VA_ARGS__); } while (0)
> > +#endif
> > +
> >  // For debuging we use signed operations so overflows can be detected
> (by ubsan)
> >  // For production we use unsigned so there are no undefined operations
> >  #ifdef CHECKED
>
> Not really a fan of this. (And neither ff_dlog.) But I guess I don't
> mean to stop you.
>
> In my opinion, code that checks for overflow and fuzz issues should be
> as small and unintrusive as possible, or preferably not exist.


This ^^^ +100.

And to be clear, "code" means binary as well as source code size.

(Whether that applies to h264_ps specifically is a separate issue; as I
said on IRC a few days ago, it may well be that out-of-bounds values for
uv_qp_offset are common outside of fuzzing, e.g. some encoders creating
non-spec-compliant files. If that is really true, and we have examples of
such files, then I totally understand that you want a log message to aid
debugging, and maybe even a way to override them using -flags, as well as
an indication of this option/flag in the error message.)

Ronald
wm4 Feb. 23, 2017, 1:08 p.m.
On Thu, 23 Feb 2017 07:57:41 -0500
"Ronald S. Bultje" <rsbultje@gmail.com> wrote:

> Hi,
> 
> On Thu, Feb 23, 2017 at 2:19 AM, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Wed, 22 Feb 2017 19:16:46 +0100
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >  
> > > This enables the extra error messages in case of DEBUG or high assrtion  
> > levels.  
> > > High assertion levels imply slow checks in inner loops so any extra  
> > error should  
> > > be insignificant.
> > > Is it preferred to have a separate switch for ff_elog() so it doesnt  
> > depend on  
> > > DEBUG/assertion level ?
> > >
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavutil/internal.h | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/libavutil/internal.h b/libavutil/internal.h
> > > index 7780a9a791..208f8f474f 100644
> > > --- a/libavutil/internal.h
> > > +++ b/libavutil/internal.h
> > > @@ -262,6 +262,12 @@ void avpriv_request_sample(void *avc,
> > >  #   define ff_dlog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_DEBUG,  
> > __VA_ARGS__); } while (0)  
> > >  #endif
> > >
> > > +#if defined(DEBUG) || ASSERT_LEVEL > 1
> > > +#   define ff_elog(ctx, ...) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__)
> > > +#else
> > > +#   define ff_elog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_ERROR,  
> > __VA_ARGS__); } while (0)  
> > > +#endif
> > > +
> > >  // For debuging we use signed operations so overflows can be detected  
> > (by ubsan)  
> > >  // For production we use unsigned so there are no undefined operations
> > >  #ifdef CHECKED  
> >
> > Not really a fan of this. (And neither ff_dlog.) But I guess I don't
> > mean to stop you.
> >
> > In my opinion, code that checks for overflow and fuzz issues should be
> > as small and unintrusive as possible, or preferably not exist.  
> 
> 
> This ^^^ +100.
> 
> And to be clear, "code" means binary as well as source code size.

Well, I think the appeal here is that the error message is normally
not compiled by default, because DEBUG is undefined and ASSERT_LEVEL is
0. I guess.

> (Whether that applies to h264_ps specifically is a separate issue; as I
> said on IRC a few days ago, it may well be that out-of-bounds values for
> uv_qp_offset are common outside of fuzzing, e.g. some encoders creating
> non-spec-compliant files. If that is really true, and we have examples of
> such files, then I totally understand that you want a log message to aid
> debugging, and maybe even a way to override them using -flags, as well as
> an indication of this option/flag in the error message.)

Didn't know this... acknowledged.
Michael Niedermayer Feb. 23, 2017, 2:45 p.m.
On Thu, Feb 23, 2017 at 07:57:41AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Thu, Feb 23, 2017 at 2:19 AM, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Wed, 22 Feb 2017 19:16:46 +0100
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >
> > > This enables the extra error messages in case of DEBUG or high assrtion
> > levels.
> > > High assertion levels imply slow checks in inner loops so any extra
> > error should
> > > be insignificant.
> > > Is it preferred to have a separate switch for ff_elog() so it doesnt
> > depend on
> > > DEBUG/assertion level ?
> > >
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavutil/internal.h | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/libavutil/internal.h b/libavutil/internal.h
> > > index 7780a9a791..208f8f474f 100644
> > > --- a/libavutil/internal.h
> > > +++ b/libavutil/internal.h
> > > @@ -262,6 +262,12 @@ void avpriv_request_sample(void *avc,
> > >  #   define ff_dlog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_DEBUG,
> > __VA_ARGS__); } while (0)
> > >  #endif
> > >
> > > +#if defined(DEBUG) || ASSERT_LEVEL > 1
> > > +#   define ff_elog(ctx, ...) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__)
> > > +#else
> > > +#   define ff_elog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_ERROR,
> > __VA_ARGS__); } while (0)
> > > +#endif
> > > +
> > >  // For debuging we use signed operations so overflows can be detected
> > (by ubsan)
> > >  // For production we use unsigned so there are no undefined operations
> > >  #ifdef CHECKED
> >
> > Not really a fan of this. (And neither ff_dlog.) But I guess I don't
> > mean to stop you.
> >
> > In my opinion, code that checks for overflow and fuzz issues should be
> > as small and unintrusive as possible, or preferably not exist.
> 
> 
> This ^^^ +100.
> 
> And to be clear, "code" means binary as well as source code size.
> 

> (Whether that applies to h264_ps specifically is a separate issue; as I
> said on IRC a few days ago, it may well be that out-of-bounds values for
> uv_qp_offset are common outside of fuzzing, e.g. some encoders creating
> non-spec-compliant files. If that is really true, and we have examples of
> such files, then I totally understand that you want a log message to aid
> debugging, and maybe even a way to override them using -flags, as well as
> an indication of this option/flag in the error message.)

Iam not aware of such file but i know even less about the range
encoders use for this value. Its not even clear how you would test
for this, encoders wouldnt neccesarily generate the corner case
values on demand.

With a clear message there, anyone running into such a file and posting
it would lead to this range being extended to support the real world
range. without message the issue will be found quickly after someone
looks at that specific failure.
Problem is it commonly takes months before some things get looked into,
Its not uncommon to spot older issues on trac that are in fact rather
simple to fix, giving me the feeling that there are few people looking
into some classes of issues.

all that isnt specific to this patch, case or error and a reason
why i pushed for adding errors to most cases.
But this fight is too tireing and too many people opposed it also it
costs much more time than just maintaining the messages outside ffmpeg
and so i since yesterday just add the messages locally and keep them
in a branch. I intend to put that on my github repo if it grows to
something thats worth publishing.

[...]

Patch hide | download patch | download mbox

diff --git a/libavutil/internal.h b/libavutil/internal.h
index 7780a9a791..208f8f474f 100644
--- a/libavutil/internal.h
+++ b/libavutil/internal.h
@@ -262,6 +262,12 @@  void avpriv_request_sample(void *avc,
 #   define ff_dlog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_DEBUG, __VA_ARGS__); } while (0)
 #endif
 
+#if defined(DEBUG) || ASSERT_LEVEL > 1
+#   define ff_elog(ctx, ...) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__)
+#else
+#   define ff_elog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__); } while (0)
+#endif
+
 // For debuging we use signed operations so overflows can be detected (by ubsan)
 // For production we use unsigned so there are no undefined operations
 #ifdef CHECKED