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

[Rémi Denis-Courmont]

Le tiistaina 10. maaliskuuta 2020, 9.44.16 EET Thomas Guillem a écrit :
> 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.

Me neither. This should be an assertion IMO.

> > 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 );
> > >> 
> > >> _______________________________________________
> > >> 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
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


-- 
Rémi Denis-Courmont
http://www.remlab.net/