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

[Timothy Arceri]

On Mon, 2016-01-18 at 09:45 +1100, Timothy Arceri wrote:
> On Mon, 2016-01-18 at 09:34 +1100, Timothy Arceri 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 imple+      if (uni == NULL) {> > > +     
> >   
> >   if (++i < count)> > > +            continue;> > > +         else>
> > >
> >  > +            break;menting 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;
> 
> Actually no that wouldn't work, the spec makes it legal to attempt to
> set inactive locations there is no guarantee that any of the
> locations
> secified by the user are active in which case your suggestion would
> send them into an infinite loop.
> 
> If it makes people happy I'll just change this to:
> 
>       if (uni == NULL) {
>          i++;
>          if (i < count)
>             continue;
>          else
>             break;
>       }
> 
> Which is what I had originally but IMO it just look ugly.

I thought I'd test this in the piglit test I wrote, and it might be
safe after all but I'd still like to go ahead with the above code
anyway just to be safe. I wanted to see what would happen if I used a
subroutine uniform array with an explicit location but that cause a
segfault in the linker.


> 
> > 
> > 
> > 
> > > > +      }
> > > > +
> > > >        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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev