[FFmpeg-devel] [PATCH] avutil/log: Check and warn for recursive calls

[Michael Niedermayer]

On Thu, Mar 20, 2014 at 05:36:11PM +0100, wm4 wrote:
> On Thu, 20 Mar 2014 14:51:11 +0100
> Michael Niedermayer <michaelni at gmx.at> wrote:
> 
> > this only works when a error checking mutex is available.
> > an alternative would be to use thread local storage to implement our own checking mutex
> > 
> > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > ---
> >  libavutil/log.c |   10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavutil/log.c b/libavutil/log.c
> > index a0bb5e4..b32dfe0 100644
> > --- a/libavutil/log.c
> > +++ b/libavutil/log.c
> > @@ -42,7 +42,11 @@
> >  
> >  #if HAVE_PTHREADS
> >  #include <pthread.h>
> > +#    ifdef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
> > +static pthread_mutex_t mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
> > +#    else
> >  static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> > +#    endif
> >  #endif
> >  
> >  #define LINE_SZ 1024
> > @@ -251,7 +255,11 @@ void av_log_default_callback(void* ptr, int level, const char* fmt, va_list vl)
> >      if (level > av_log_level)
> >          return;
> >  #if HAVE_PTHREADS
> > -    pthread_mutex_lock(&mutex);
> > +    if (pthread_mutex_lock(&mutex)) {
> > +        const char *msg = "av_log is thread safe, but cannot be called from signal handlers\n";
> > +        write (2, msg, strlen(msg));
> > +        return;
> > +    }
> 
> So this is just to catch signal handler usage?

Its the case iam aware of, i do not know if its the only case in
which this will ever be called in the wide world by all the
applications
maybe i should write it more generically but i thought thats the
most likely scenario and other people will understand it even in
other scenarios like interrupt handlers


> Calling pthread
> functions from signal handlers is AFAIK not safe, even if you attempt
> to workaround the fundamental deadlock problems.

this patch doesnt change this, the mutex lock is called from there
before the patch already


> 
> I'd say ffmpeg is not responsible for catching basic undefined behavior
> (like calling things that are not signal-safe from signal handlers).

I prefer a error message that tells me what is wrong over a deadock
that i cannot reproduce in gdb (this was actually the case for the
one this lead to this)


also my concern is not about an application directly calling
av_log() from a signal handler but more that it calls some function
that appears safe and that then calls av_log()

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140320/7bb581be/attachment.asc>