[vlc-devel] [PATCH] input: decoder: fix NULL deref on early cancellation

[Thomas Guillem]

On Tue, Mar 10, 2020, at 08:01, Steve Lhomme wrote:
> The patch won't hurt but I also wonder how it's possible to end up in 

I don't like this reasoning, the variables state of input/decoder.c need to be clearly defined.

> this situation.

Easy, Kill the decoder with an asynchronous decoder.
DeleteDecoder() will first destroy the out_pool, then the module will be unloaded.
So, nothing prevent the module to request a picture from an asynchronous thread between the out_pool destroy and the module unloading.

Since ModuleThread_NewVideoBuffer() is called without a lock, this patch is wrong.
The proposed solution is to first unload the module, then destroy the out_pool from DeleteDecoder().

> 
> The decoder thread is not canceled as far as I can see in decoder.c. So 
> that means the ModuleThread in this case is an internal thread of the 
> decoder (likely lavc). If it's canceled it should prevent other threads 
> trying to continue using the decoder after that. In this case the 
> "decoder" object is a resource that should be protected from multiple 
> thread accesses at the same time.
> 
> So maybe this fix (if it actually happened) is hiding a different problem.
> 
> On 2020-03-10 7:49, Thomas Guillem wrote:
> > 
> > 
> > On Mon, Mar 9, 2020, at 22:50, Francois Cartegnie wrote:
> >> ---
> >>   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 d868c21f23..998a2816bb 100644
> >> --- a/src/input/decoder.c
> >> +++ b/src/input/decoder.c
> >> @@ -650,7 +650,8 @@ static picture_t *ModuleThread_NewVideoBuffer(
> >> decoder_t *p_dec )
> >>   {
> >>       struct decoder_owner *p_owner = dec_get_owner( p_dec );
> >>       assert( p_owner->p_vout );
> >> -
> >> +    if( p_owner->out_pool == NULL )
> >> +        return NULL;
> > 
> > 
> > The big question is : why is the out_pool destroyed before the decoder module in DeleteDecoder ?
> 
> It makes sense to remove the pictures allocated by the decoder before 
> the decoder module is gone and possibly DLLs not accessible anymore to 
> do it (DVXA.DLL for example).
> 
> In practice this pool is a shadow of the actual surfaces allocated by 
> the decoder in GPU decoding. It may not hold any "context" at this 
> point. It depends if some pictures are in-flight (sent by the decoder to 
> be used) or not. In CPU decoding it's not an issue. This pool could also 
> go away if each decoder managed its pool but that is not the case yet.
> 
> > So that every future decoder_NewPicture fails ? In that case, this patch is OK for me.
> > 
> > Steve ?
> > 
> >>       picture_t *pic = picture_pool_Wait( p_owner->out_pool );
> >>       if (pic)
> >>           picture_Reset( pic );
> >> -- 
> >> 2.24.1
> >>
> >> _______________________________________________
> >> vlc-devel mailing list
> >> To unsubscribe or modify your subscription options:
> >> https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel