[vlc-devel] [PATCH] decoder: Access p_owner->fmt from a locked context

[Rémi Denis-Courmont]

Le torstaina 7. syyskuuta 2017, 18.21.01 EEST Hugo Beauzée-Luyssen a écrit :
> Otherwise such a race can happen:
> ==17260==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x6020000b5470 at pc 0x7f1fc7a82181 bp 0x7f1f9056e320 sp 0x7f1f9056dad0
> READ of size 2 at 0x6020000b5470 thread T20
>     #0 0x7f1fc7a82180 in strdup
> (/usr/lib/x86_64-linux-gnu/libasan.so.3+0x58180)
>     #1 0x7f1fc72b138e in es_format_Copy ../../src/misc/es_format.c:471
>     #2 0x7f1fc71da8b9 in input_DecoderHasFormatChanged
> ../../src/input/decoder.c:2314
>     #3 0x7f1fc71ee3a3 in EsOutSend ../../src/input/es_out.c:2059
>     #4 0x7f1fc71f9690 in es_out_Send ../../include/vlc_es_out.h:135
>     #5 0x7f1fc7201a51 in CmdExecuteSend
> ../../src/input/es_out_timeshift.c:1348
>     #6 0x7f1fc71fadae in Send ../../src/input/es_out_timeshift.c:472
>     #7 0x7f1f8f446211 in es_out_Send ../../include/vlc_es_out.h:135
>     #8 0x7f1f8f44a7e4 in MP4_Block_Send
> ../../modules/demux/mp4/mp4.c:648
>     #9 0x7f1f8f44ece4 in DemuxTrack ../../modules/demux/mp4/mp4.c:1277
>     #10 0x7f1f8f44fcdc in DemuxMoov ../../modules/demux/mp4/mp4.c:1399
>     #11 0x7f1f8f44ffaf in Demux ../../modules/demux/mp4/mp4.c:1428
>     #12 0x7f1fc7209064 in demux_Demux ../../include/vlc_demux.h:347
>     #13 0x7f1fc720c326 in MainLoopDemux ../../src/input/input.c:572
>     #14 0x7f1fc720d3eb in MainLoop ../../src/input/input.c:721
>     #15 0x7f1fc720bef8 in Run ../../src/input/input.c:508
>     #16 0x7f1fc623b493 in start_thread
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x7493)
>     #17 0x7f1fc5f7dafe in __clone
> (/lib/x86_64-linux-gnu/libc.so.6+0xe8afe)
> 
> 0x6020000b5470 is located 0 bytes inside of 4-byte region
> [0x6020000b5470,0x6020000b5474)
> freed by thread T37 here:
>     #0 0x7f1fc7aeba10 in free
> (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc1a10)
>     #1 0x7f1fc71d1f03 in DecoderProcessSout
> ../../src/input/decoder.c:860
>     #2 0x7f1fc71d50bf in DecoderProcess ../../src/input/decoder.c:1394
>     #3 0x7f1fc71d632c in DecoderThread ../../src/input/decoder.c:1599
>     #4 0x7f1fc623b493 in start_thread
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x7493)
> ---
>  src/input/decoder.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/input/decoder.c b/src/input/decoder.c
> index 109d5a2205..ca08d099fd 100644
> --- a/src/input/decoder.c
> +++ b/src/input/decoder.c
> @@ -851,7 +851,6 @@ static void DecoderProcessSout( decoder_t *p_dec,
> block_t *p_block ) {
>              vlc_mutex_lock( &p_owner->lock );
>              DecoderUpdateFormatLocked( p_dec );
> -            vlc_mutex_unlock( &p_owner->lock );
> 
>              p_owner->fmt.i_group = p_dec->fmt_in.i_group;
>              p_owner->fmt.i_id = p_dec->fmt_in.i_id;
> @@ -869,6 +868,7 @@ static void DecoderProcessSout( decoder_t *p_dec,
> block_t *p_block ) {
>                  msg_Err( p_dec, "cannot create packetizer output (%4.4s)",
>                           (char *)&p_owner->fmt.i_codec );
> +                vlc_mutex_unlock( &p_owner->lock );
>                  p_owner->error = true;
> 
>                  if(p_block)
> @@ -877,6 +877,7 @@ static void DecoderProcessSout( decoder_t *p_dec,
> block_t *p_block ) block_ChainRelease(p_sout_block);
>                  break;
>              }
> +            vlc_mutex_unlock( &p_owner->lock );
>          }
> 
>          while( p_sout_block )

No way. The decoder lock is first and foremost to protect the decoder input 
queue. Retaining it while creating the sout instance is insane.

-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/