[Mesa-dev] [PATCH] i965: miptree: prevent potential NULL pointer access

[Pohjolainen, Topi]

On Thu, Nov 10, 2016 at 07:30:17AM +0000, Chris Wilson wrote:
> On Thu, Nov 10, 2016 at 09:21:40AM +0200, Pohjolainen, Topi wrote:
> > On Wed, Nov 09, 2016 at 04:38:43PM +0000, Lionel Landwerlin wrote:
> > > If the mcs buffer allocation fails we might get a NULL pointer. This
> > > was reported by Coverity and should only happen if we run out of
> > > memory.
> > > 
> > > CID: 1394290
> > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> > 
> > Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > 
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 6c81ffb..2115e09 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -1486,6 +1486,8 @@ intel_miptree_init_mcs(struct brw_context *brw,
> > >                         struct intel_mipmap_tree *mt,
> > >                         int init_value)
> > >  {
> > > +   assert(mt->mcs_buf != NULL);
> > > +
> > >     /* From the Ivy Bridge PRM, Vol 2 Part 1 p326:
> > >      *
> > >      *     When MCS buffer is enabled and bound to MSRT, it is required that it
> > > @@ -1605,7 +1607,8 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> > >                                     mt->logical_height0,
> > >                                     MIPTREE_LAYOUT_ACCELERATED_UPLOAD);
> > >  
> > > -   intel_miptree_init_mcs(brw, mt, 0xFF);
> > > +   if (mt->mcs_buf)
> > > +      intel_miptree_init_mcs(brw, mt, 0xFF);
> 
> Just return the failure at the point of allocation. Then fix
> intel_miptree_init_mcs(), it's error handling is broken - perhaps it
> would be best if init_msc() didn't try to free a resource it didn't
> allocate but reported the error back to the routine that did.

I agree. Thanks Chris!