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

[Jason Ekstrand]

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.

> 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/20160117/8c8222e4/attachment.html>