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

[Timothy Arceri]

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.

> 
> 
> 
> > > +      }
> > > +
> > >        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