[FFmpeg-devel] [PATCH] pthread_frame: do not attempt to unlock a mutex on the wrong thread

[wm4]

On Sat, 25 Mar 2017 13:01:58 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Sat, Mar 25, 2017 at 12:03:23PM +0100, wm4 wrote:
> > On Fri, 24 Mar 2017 18:53:41 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Fri, Mar 24, 2017 at 01:47:10PM +0100, wm4 wrote:  
> > > > async_mutex has is used in a very strange but intentional way: it is
> > > > locked by default, and unlocked only in regions that can be run
> > > > concurrently.
> > > > 
> > > > If the user was calling API functions to the same context from different
> > > > threads (in a safe way), this could unintentionally unlock the mutex on
> > > > a different thread than the previous lock operation. It's not allowed by
> > > > the pthread API.
> > > > 
> > > > Fix this by emulating a binary semaphore using a mutex and condition
> > > > variable. (Posix semaphores are not available on all platforms.)
> > > > ---
> > > >  libavcodec/pthread_frame.c | 41 +++++++++++++++++++++++++++++++----------
> > > >  1 file changed, 31 insertions(+), 10 deletions(-)    
> > > 
> > > this breaks flac
> > > 
> > > make -j12 ffmpeg && ./ffmpeg -i ~/tickets/4421/testwav_cut.wav out.flac ; ./ffmpeg -i out.flac -f null -
> > > https://trac.ffmpeg.org/attachment/ticket/4421/testwav_cut.wav
> > > 
> > > Input #0, flac, from 'out.flac':
> > >   Metadata:
> > >     ENCODER         : Lavf57.67.100
> > >   Duration: 00:00:10.00, start: 0.000000, bitrate: 912 kb/s
> > >     Stream #0:0: Audio: flac, 44100 Hz, stereo, s16
> > > Stream mapping:
> > >   Stream #0:0 -> #0:0 (flac (native) -> pcm_s16le (native))
> > > Press [q] to stop, [?] for help
> > > Output #0, null, to 'pipe:':
> > >   Metadata:
> > >     encoder         : Lavf57.67.100
> > >     Stream #0:0: Audio: pcm_s16le, 44100 Hz, stereo, s16, 1411 kb/s
> > >     Metadata:
> > >       encoder         : Lavc57.86.103 pcm_s16le
> > > Assertion ret <= avpkt->size failed at libavcodec/utils.c:2507
> > > Aborted
> > > 
> > > [...]  
> > 
> > Actually that was a HTML link. Tried it again with the proper file,
> > still cannot reproduce, both commands run successfully. I tried 3 times.
> >   
> 
> > Please post only things that can be reproduced.  
> 
> It reproduces 100% here:
> 
> git log -2 --oneline
> 4bf2209f29 pthread_frame: do not attempt to unlock a mutex on the wrong thread
> 09ce5519f3 fate/checkasm: fix use of uninitialized memory on hevc_add_res tests
> 
> make distclean ; ./configure  --enable-pthreads --assert-level=2
> make -j12 ffmpeg && ./ffmpeg -y -i ~/tickets/4421/testwav_cut.wav out.flac ; ./ffmpeg -i out.flac -f null -
> 
> 77700c5eaf9daf4d5d5092af79774882  /home/michael/tickets/4421/testwav_cut.wav
> 1fb29c0b2086a7cabf389de647316249  out.flac
> 
> ...
> Press [q] to stop, [?] for help
> Output #0, null, to 'pipe:':
>   Metadata:
>     encoder         : Lavf57.67.100
>     Stream #0:0: Audio: pcm_s16le, 44100 Hz, stereo, s16, 1411 kb/s
>     Metadata:
>       encoder         : Lavc57.86.103 pcm_s16le
> Assertion ret <= avpkt->size failed at libavcodec/utils.c:2507
> Aborted
> 
> 2nd ffmpeg under valgrind
> 
> Output #0, null, to 'pipe:':
>   Metadata:
>     encoder         : Lavf57.67.100
>     Stream #0:0: Audio: pcm_s16le, 44100 Hz, stereo, s16, 1411 kb/s
>     Metadata:
>       encoder         : Lavc57.86.103 pcm_s16le
> ==20594== Conditional jump or move depends on uninitialised value(s)
> ==20594==    at 0xADBE5D: avcodec_decode_audio4 (utils.c:2390)
> ==20594==    by 0xADC797: do_decode (utils.c:2827)
> ==20594==    by 0xADD7EF: avcodec_receive_frame (utils.c:2949)
> ==20594==    by 0x49F99A: process_input_packet (ffmpeg.c:2256)
> ==20594==    by 0x4A11F8: process_input (ffmpeg.c:4171)
> ==20594==    by 0x4A362E: transcode (ffmpeg.c:4481)
> ==20594==    by 0x483B17: main (ffmpeg.c:4740)
> ==20594==
> ==20594== Conditional jump or move depends on uninitialised value(s)
> ==20594==    at 0xADC0F5: avcodec_decode_audio4 (utils.c:2497)
> ==20594==    by 0xADC797: do_decode (utils.c:2827)
> ==20594==    by 0xADD7EF: avcodec_receive_frame (utils.c:2949)
> ==20594==    by 0x49F99A: process_input_packet (ffmpeg.c:2256)
> ==20594==    by 0x4A11F8: process_input (ffmpeg.c:4171)
> ==20594==    by 0x4A362E: transcode (ffmpeg.c:4481)
> ==20594==    by 0x483B17: main (ffmpeg.c:4740)
> ==20594==
> ==20594== Conditional jump or move depends on uninitialised value(s)
> ==20594==    at 0xADC10D: avcodec_decode_audio4 (utils.c:2507)
> ==20594==    by 0xADC797: do_decode (utils.c:2827)
> ==20594==    by 0xADD7EF: avcodec_receive_frame (utils.c:2949)
> ==20594==    by 0x49F99A: process_input_packet (ffmpeg.c:2256)
> ==20594==    by 0x4A11F8: process_input (ffmpeg.c:4171)
> ==20594==    by 0x4A362E: transcode (ffmpeg.c:4481)
> ==20594==    by 0x483B17: main (ffmpeg.c:4740)
> ==20594==
> Assertion ret <= avpkt->size failed at libavcodec/utils.c:2507
> 

It's because ff_thread_decode_frame() does not initialize ret in all
code paths, now it works, fixed locally.