Bug 102530

Are you able to provide a stack trace?

Just a guess but at least it looks possible for "UniformRemapTable[location]" to access garbage, there should be a check for location value before using it.

(In reply to Tapani Pälli from comment #2)
> Just a guess but at least it looks possible for
> "UniformRemapTable[location]" to access garbage, there should be a check for
> location value before using it.

Disclaimer: I did not realize that undefined behaviour including a crash in that situation is actually OK by the no_error spec so forget about this comment :)

(In reply to Tapani Pälli from comment #3)
> (In reply to Tapani Pälli from comment #2)
> > Just a guess but at least it looks possible for
> > "UniformRemapTable[location]" to access garbage, there should be a check for
> > location value before using it.
> 
> Disclaimer: I did not realize that undefined behaviour including a crash in
> that situation is actually OK by the no_error spec so forget about this
> comment :)

Created attachment 133967 [details]
Kodi's log after segfault

This is the core dump produced by Kodi when segfaulting.
/usr/bin/kodi: line 175: 16779 Segmentation fault      (core dumped) "$LIBDIR/${bin_name}/${bin_name}.bin" $SAVED_ARGS

If you need something else, let me know.

(In reply to Alexandre Demers from comment #5)
> Created attachment 133967 [details]
> Kodi's log after segfault
> 
> This is the core dump produced by Kodi when segfaulting.
> /usr/bin/kodi: line 175: 16779 Segmentation fault      (core dumped)
> "$LIBDIR/${bin_name}/${bin_name}.bin" $SAVED_ARGS
> 
> If you need something else, let me know.

So that looks like the stack trace from having NO_ERROR enabled can you provide one for the crash that happens when NO_ERROR is disabled.

Timothy Arceri: In either case, it looks like the handling of location = -1 is incorrect in the no-error case. I think we should ignore the command in this case...

Something like this?

---8<---

diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
index c6488325a8..642c93270f 100644
--- a/src/mesa/main/uniform_query.cpp
+++ b/src/mesa/main/uniform_query.cpp
@@ -911,6 +911,9 @@ _mesa_uniform(GLint location, GLsizei count, const GLvoid *values,
 
    struct gl_uniform_storage *uni;
    if (_mesa_is_no_error_enabled(ctx)) {
+      if (location == -1)
+         return;
+
       uni = shProg->UniformRemapTable[location];
 
       /* The array index specified by the uniform location is just the

---8<---

(In reply to Erik Faye-Lund from comment #8)
> Something like this?
> 
> ---8<---
> 
> diff --git a/src/mesa/main/uniform_query.cpp
> b/src/mesa/main/uniform_query.cpp
> index c6488325a8..642c93270f 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
> @@ -911,6 +911,9 @@ _mesa_uniform(GLint location, GLsizei count, const
> GLvoid *values,
>  
>     struct gl_uniform_storage *uni;
>     if (_mesa_is_no_error_enabled(ctx)) {
> +      if (location == -1)
> +         return;
> +
>        uni = shProg->UniformRemapTable[location];
>  
>        /* The array index specified by the uniform location is just the
> 
> ---8<---

This was already fixed.

(In reply to Timothy Arceri from comment #6)
> (In reply to Alexandre Demers from comment #5)
> > Created attachment 133967 [details]
> > Kodi's log after segfault
> > 
> > This is the core dump produced by Kodi when segfaulting.
> > /usr/bin/kodi: line 175: 16779 Segmentation fault      (core dumped)
> > "$LIBDIR/${bin_name}/${bin_name}.bin" $SAVED_ARGS
> > 
> > If you need something else, let me know.
> 
> So that looks like the stack trace from having NO_ERROR enabled can you
> provide one for the crash that happens when NO_ERROR is disabled.

I'll do it later when I'll get back from work.

(In reply to Timothy Arceri from comment #9)
> (In reply to Erik Faye-Lund from comment #8)
> > Something like this?
> > 
> > ---8<---
> > 
> > diff --git a/src/mesa/main/uniform_query.cpp
> > b/src/mesa/main/uniform_query.cpp
> > index c6488325a8..642c93270f 100644
> > --- a/src/mesa/main/uniform_query.cpp
> > +++ b/src/mesa/main/uniform_query.cpp
> > @@ -911,6 +911,9 @@ _mesa_uniform(GLint location, GLsizei count, const
> > GLvoid *values,
> >  
> >     struct gl_uniform_storage *uni;
> >     if (_mesa_is_no_error_enabled(ctx)) {
> > +      if (location == -1)
> > +         return;
> > +
> >        uni = shProg->UniformRemapTable[location];
> >  
> >        /* The array index specified by the uniform location is just the
> > 
> > ---8<---
> 
> This was already fixed.

OK, then I guess this commit was a false positive when bisecting, because it wasn't fixed in the commit this was tested against.

(In reply to Erik Faye-Lund from comment #11)
> (In reply to Timothy Arceri from comment #9)
> > (In reply to Erik Faye-Lund from comment #8)
> > > Something like this?
> > > 
> > > ---8<---
> > > 
> > > diff --git a/src/mesa/main/uniform_query.cpp
> > > b/src/mesa/main/uniform_query.cpp
> > > index c6488325a8..642c93270f 100644
> > > --- a/src/mesa/main/uniform_query.cpp
> > > +++ b/src/mesa/main/uniform_query.cpp
> > > @@ -911,6 +911,9 @@ _mesa_uniform(GLint location, GLsizei count, const
> > > GLvoid *values,
> > >  
> > >     struct gl_uniform_storage *uni;
> > >     if (_mesa_is_no_error_enabled(ctx)) {
> > > +      if (location == -1)
> > > +         return;
> > > +
> > >        uni = shProg->UniformRemapTable[location];
> > >  
> > >        /* The array index specified by the uniform location is just the
> > > 
> > > ---8<---
> > 
> > This was already fixed.
> 
> OK, then I guess this commit was a false positive when bisecting, because it
> wasn't fixed in the commit this was tested against.

By the way, if it was fixed, the fix may be occulted by Bug 102502. I first reported 102502, which kills Kodi at launch, and I ended up finding the current bug (102530) while bisecting the first one. Fixing 102502 may show that the current bug is already fix then.

Created attachment 133983 [details]
Kodi segfault with MESA_NO_ERROR=0

Core dump produced by Kodi when MESA_NO_ERROR=0

(In reply to Alexandre Demers from comment #13)
> Created attachment 133983 [details]
> Kodi segfault with MESA_NO_ERROR=0
> 
> Core dump produced by Kodi when MESA_NO_ERROR=0

Have you tried it Kodi works with current Mesa? This backtrace is also made with old Mesa so whatever the problem is it might have been fixed already.

(In reply to Alexandre Demers from comment #13)
> Created attachment 133983 [details]
> Kodi segfault with MESA_NO_ERROR=0
> 
> Core dump produced by Kodi when MESA_NO_ERROR=0


You still somehow ended up inside the _mesa_is_no_error_enabled-code-path. So I guess Kodi enables the no-error extension. In either case, this problem was fixed in commit f25b2f76b03.

AFAIK MESA_NO_ERROR=0 currently enables the "no error" behaviour — the code only checks that the environment variable is set to any value.

Ideally, MESA_NO_ERROR=0 should disable the "no error" behaviour, even if the application tries to enable it. Or there needs to be some other mechanism for that.

(In reply to Tapani Pälli from comment #14)
> (In reply to Alexandre Demers from comment #13)
> > Created attachment 133983 [details]
> > Kodi segfault with MESA_NO_ERROR=0
> > 
> > Core dump produced by Kodi when MESA_NO_ERROR=0
> 
> Have you tried it Kodi works with current Mesa? This backtrace is also made
> with old Mesa so whatever the problem is it might have been fixed already.

Yes, but as stated previously, bug 102502 prevents me from being sure f25b2f76b03 fixed the issue.

(In reply to Erik Faye-Lund from comment #15)
> (In reply to Alexandre Demers from comment #13)
> > Created attachment 133983 [details]
> > Kodi segfault with MESA_NO_ERROR=0
> > 
> > Core dump produced by Kodi when MESA_NO_ERROR=0
> 
> 
> You still somehow ended up inside the _mesa_is_no_error_enabled-code-path.
> So I guess Kodi enables the no-error extension. In either case, this problem
> was fixed in commit f25b2f76b03.

Good to know, it had been committed after commit associated with bug 102502, so this could be indeed fixed.

(In reply to Michel Dänzer from comment #16)
> AFAIK MESA_NO_ERROR=0 currently enables the "no error" behaviour — the code
> only checks that the environment variable is set to any value.
> 

This is a problem.

> Ideally, MESA_NO_ERROR=0 should disable the "no error" behaviour, even if
> the application tries to enable it. Or there needs to be some other
> mechanism for that.

Indeed.

Created attachment 134015 [details] [review]
Enable KHR_NO_ERROR only if MESA_NO_ERROR's value is different from 0

From Michel Dänzer's comment, KHR_NO_ERROR is enabled whenever MESA_NO_ERROR is set with no regard to its actual value (which is not the case when set through driconf).

This should take care of the problem, activating KHR_NO_ERROR only if MESA_NO_ERROR is different than 0.

If this is fine, could someone commit it for me?

The attached patch from Alexandre seems to be a noop.

This series from Eric treats MESA_NO_ERROR as boolean.
https://patchwork.freedesktop.org/series/29888/

(In reply to Emil Velikov from comment #21)
> The attached patch from Alexandre seems to be a noop.
> 
> This series from Eric treats MESA_NO_ERROR as boolean.
> https://patchwork.freedesktop.org/series/29888/

I like the boolean usage... and maybe prefer it to my proposed patch.

This bug is indeed fixed in latest git commit. And I propose my reviewed-by to Eric's patch if needed for dealing with the actuel MESA_NO_ERROR's value.