[Mesa-dev] [PATCH v2 2/3] mesa: validate size parameters for glTexStorage*Multisample

[Timothy Arceri]

On Fri, 2015-08-14 at 08:55 +0300, Tapani Pälli wrote:
> 
> On 08/13/2015 11:54 AM, Timothy Arceri wrote:
> > I've sent a couple of follow-up patches I notice when reviewing 
> > this.
> > 
> > 
> > On Thu, 2015-08-13 at 09:30 +0300, Tapani Pälli wrote:
> > > v2: code cleanup
> > > 
> > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > ---
> > >   src/mesa/main/teximage.c | 24 ++++++++++++++++++++++++
> > >   1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> > > index d35dc12..add7438 100644
> > > --- a/src/mesa/main/teximage.c
> > > +++ b/src/mesa/main/teximage.c
> > > @@ -5782,6 +5782,18 @@ _mesa_TexImage3DMultisample(GLenum target, 
> > > GLsizei
> > > samples,
> > >                                      "glTexImage3DMultisample");
> > >   }
> > > 
> > > +static bool
> > > +valid_texstorage_ms_parameters(GLsizei width, GLsizei height, 
> > > GLsizei
> > > depth,
> > > +                               GLsizei samples, unsigned dims)
> > > +{
> > > +   GET_CURRENT_CONTEXT(ctx);
> > > +
> > > +   if (_mesa_tex_storage_invalid_dim(width, height, depth) || 
> > > samples < 1)
> > 
> > 
> > Rather than do the samples < 1 check here you should move it to
> >   _mesa_texture_image_multisample as the spec says "An 
> > INVALID_VALUE error is
> > generated ifsamplesis zero." for glTexImage*Multisample too.
> > 
> > With that change you could change all the calls below to something 
> > like
> > 
> >     if (_mesa_tex_storage_invalid_dim(width, height, depth)) {
> >        _mesa_error(ctx, GL_INVALID_VALUE, 
> > "glTexStorage2DMultisample()"
> >        return;
> >     }
> 
> Would it be OK if we do possible unification for checking samples 
> parameter after these patches? My goal here is to do check for 
> glTexStorage*Multisample only. I don't feel comfortable changing 
> checks 
> for other functions at the same time, that should deserve it's own 
> commit so it's easier to bisect later.
> 

There is no reason to split the change into two patches. The correct
change is to update _mesa_texture_image_multisample (which I will
rename texture_image_multisample once your patch lands) to do the
samples check. The only other user is image*Multisample and the spec
clearly states that this is also an error when 0. That spec says it
undefined what happens when this is negative but we already throw an
error via _mesa_check_sample_count(). 

You don't have to touch _mesa_check_sample_count() that change should
be in another patch which I already sent [1] feel free not to review it
:)

Doing it the way your suggesting is code churn for no gain, it just
means your code needs to be deleted and replaced with the change I'm
suggesting later anyway, so why not just fix it right the first time.

Also feel free to keep your valid_texstorage_ms_parameters() function I
was just suggesting an alternative to having another function call.


[1] 
http://lists.freedesktop.org/archives/mesa-dev/2015-August/091588.html
> > > {
> > > +      _mesa_error(ctx, GL_INVALID_VALUE, 
> > > "glTexStorage%uDMultisample()",
> > > dims);
> > > +      return false;
> > > +   }
> > > +   return true;
> > > +}
> > >   void GLAPIENTRY
> > >   _mesa_TexStorage2DMultisample(GLenum target, GLsizei samples,
> > > @@ -5795,6 +5807,9 @@ _mesa_TexStorage2DMultisample(GLenum 
> > > target, GLsizei
> > > samples,
> > >      if (!texObj)
> > >         return;
> > > 
> > > +   if (!valid_texstorage_ms_parameters(width, height, 1, 
> > > samples, 2))
> > > +      return;
> > > +
> > >      _mesa_texture_image_multisample(ctx, 2, texObj, target, 
> > > samples,
> > >                                      internalformat, width, 
> > > height, 1,
> > >                                      fixedsamplelocations, 
> > > GL_TRUE,
> > > @@ -5814,6 +5829,9 @@ _mesa_TexStorage3DMultisample(GLenum 
> > > target, GLsizei
> > > samples,
> > >      if (!texObj)
> > >         return;
> > > 
> > > +   if (!valid_texstorage_ms_parameters(width, height, depth, 
> > > samples, 3))
> > > +      return;
> > > +
> > >      _mesa_texture_image_multisample(ctx, 3, texObj, target, 
> > > samples,
> > >                                      internalformat, width, 
> > > height, depth,
> > >                                      fixedsamplelocations, 
> > > GL_TRUE,
> > > @@ -5834,6 +5852,9 @@ _mesa_TextureStorage2DMultisample(GLuint 
> > > texture,
> > > GLsizei samples,
> > >      if (!texObj)
> > >         return;
> > > 
> > > +   if (!valid_texstorage_ms_parameters(width, height, 1, 
> > > samples, 2))
> > > +      return;
> > > +
> > >      _mesa_texture_image_multisample(ctx, 2, texObj, texObj
> > > ->Target, samples,
> > >                                      internalformat, width, 
> > > height, 1,
> > >                                      fixedsamplelocations, 
> > > GL_TRUE,
> > > @@ -5855,6 +5876,9 @@ _mesa_TextureStorage3DMultisample(GLuint 
> > > texture,
> > > GLsizei samples,
> > >      if (!texObj)
> > >         return;
> > > 
> > > +   if (!valid_texstorage_ms_parameters(width, height, depth, 
> > > samples, 3))
> > > +      return;
> > > +
> > >      _mesa_texture_image_multisample(ctx, 3, texObj, texObj
> > > ->Target, samples,
> > >                                      internalformat, width, 
> > > height, depth,
> > >                                      fixedsamplelocations, 
> > > GL_TRUE,