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

[Timothy Arceri]

On Sun, 2016-01-17 at 15:20 -0800, Jason Ekstrand wrote:
> 
> On Jan 17, 2016 2:45 PM, "Timothy Arceri" <t_arceri at yahoo.com.au>
> 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.
> 
> No, it wouldn't.  What I suggested is identical to what you had before.  A continue in a do-while loop jumps to the branching condition, not the top of the loop.  The fact that you repeat the branching condition is redundant.
My concern was that it could be possible for the last element of
SubroutineUniformRemapTable to be NULL, however looking over the
uniform linking code that doesn't seem possible. 
> 
> > 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
> 

> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20160118/026b6bfd/attachment-0001.html>