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

[Tapani Pälli]

On 08/14/2015 04:01 PM, Timothy Arceri wrote:
> 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().

OK, how about land your patch first so that we can test for regressions. 
If nothing regresses after your changes then I can safely land mine.

> 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.

Yes, maybe I'm too pessimistic that nothing would break. It's just that 
typically something does and I've been really hard trying to not break 
ES 3.0 conformance (which includes ES 2.0 tests too) while making ES 3.1 
changes. We are very close to being ES 3.0 conformant (just one test 
failing ATM).

> 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,