[Mesa-stable] [Mesa-dev] [PATCH] mesa: fix segfault in glUniformSubroutinesuiv()

[Ilia Mirkin]

On Sun, Jan 17, 2016 at 5:34 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Sun, 2016-01-17 at 08:27 -0800, Jason Ekstrand wrote:
>>
>> On Jan 16, 2016 9:15 PM, "Timothy Arceri" <
>> timothy.arceri at collabora.com> wrote:
>> >
>> > From Section 7.9 (SUBROUTINE UNIFORM VARIABLES) of the OpenGL
>> > 4.5 Core spec:
>> >
>> >    "The command
>> >
>> >        void UniformSubroutinesuiv(enum shadertype, sizei count,
>> >                                   const uint *indices);
>> >
>> >    will load all active subroutine uniforms for shader stage
>> >    shadertype with subroutine indices from indices, storing
>> >    indices[i] into the uniform at location i. The indices for
>> >    any locations between zero and the value of
>> >    ACTIVE_SUBROUTINE_UNIFORM_LOCATIONS minus one which are not
>> >    used will be ignored."
>> >
>> > Cc: "11.0 11.1" mesa-stable at lists.freedesktop.org
>> > https://bugs.freedesktop.org/show_bug.cgi?id=93731
>> > ---
>> >  src/mesa/main/shaderapi.c | 14 ++++++++++++++
>> >  1 file changed, 14 insertions(+)
>> >
>> > diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>> > index cdc85f3..918c5f9 100644
>> > --- a/src/mesa/main/shaderapi.c
>> > +++ b/src/mesa/main/shaderapi.c
>> > @@ -2530,6 +2530,13 @@ _mesa_UniformSubroutinesuiv(GLenum
>> shadertype, GLsizei count,
>> >     i = 0;
>> >     do {
>> >        struct gl_uniform_storage *uni = sh
>> ->SubroutineUniformRemapTable[i];
>> > +      if (uni == NULL) {
>> > +         if (++i < count)
>> > +            continue;
>> > +         else
>> > +            break;
>> Just
>> i++;
>> continue;
>> Will do the same thing and without repeating the looping condition or
>> making the user think about pre- vs. post-increment.  I guess it does
>> make them think about continues in a do-while, but you can't win
>> everything. :-)
>
> I don't feel great about implementing the possibility of an infinite
> loop. Sure the code above should work but this stuff doesn't really
> have extensive tests.
>
> What about at the very least:
>
> i++;
> assert(i < count);
> continue;
>

You'd have to stick the assert before i++.

>
>
>> > +      }
>> > +
>> >        int uni_count = uni->array_elements ? uni->array_elements :
>> 1;
>> >        int j, k;
>> >
>> > @@ -2557,6 +2564,13 @@ _mesa_UniformSubroutinesuiv(GLenum
>> shadertype, GLsizei count,
>> >     i = 0;
>> >     do {
>> >        struct gl_uniform_storage *uni = sh
>> ->SubroutineUniformRemapTable[i];
>> > +      if (uni == NULL) {
>> > +         if (++i < count)
>> > +            continue;
>> > +         else
>> > +            break;
>> > +      }
>> > +
>> >        int uni_count = uni->array_elements ? uni->array_elements :
>> 1;
>> >
>> >        memcpy(&uni->storage[0], &indices[i],
>> > --
>> > 2.4.3
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev