<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt">Should have mentioned it does pass piglit, but now that I look I don't see any tests generating an out-of-bounds array index. I also made my own test program which does go out of bounds, and I'll submit that for inclusion in piglit. <div class="gmail_quote">On Fri, Nov 2, 2012 at 4:51 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On 11/02/2012 01:12 PM, Frank Henigman wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> validate_uniform_parameters now checks that the array index is valid. This means if an index is out of bounds, glGetUniform* now fails with GL_INVALID_OPERATION, as it should. _mesa_uniform and _mesa_uniform_matrix also call validate_uniform_parameters so the bounds checks there became redundant and were removed. The test in glGetUniformLocation is modified to check array bounds so it now returns GL_INVALID_INDEX (-1) if you ask for the location of a non-existent array element, as it should. </blockquote> </div> Do we have piglit tests for either of these cases? Do all of the existing piglit tests that pass still pass with this change? It seems like they should...<div><div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- src/mesa/main/uniform_query.cpp | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp index bddb8f9..66a2399 100644 --- a/src/mesa/main/uniform_query.cpp +++ b/src/mesa/main/uniform_query.cpp @@ -237,11 +237,14 @@ validate_uniform_parameters(struct gl_context *ctx, return false; } - /* This case should be impossible. The implication is that a call like - * glGetUniformLocation(prog, "foo[8]") was successful but "foo" is not an - * array. + /* If the uniform is an array, check that array_index is in bounds. + * If not an array, check that array_index is zero. + * array_index is unsigned so no need to check for less than zero. */ - if (*array_index != 0 && shProg->UniformStorage[*loc].array_elements == 0) { + unsigned limit = shProg->UniformStorage[*loc].array_elements; + if (limit == 0) + limit = 1; + if (*array_index >= limit) { _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)", caller, location); return false; @@ -728,9 +731,6 @@ _mesa_uniform(struct gl_context *ctx, struct gl_shader_program *shProg, * will have already generated an error. */ if (uni->array_elements != 0) { - if (offset >= uni->array_elements) - return; - count = MIN2(count, (int) (uni->array_elements - offset)); } @@ -885,9 +885,6 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct gl_shader_program *shProg, * will have already generated an error. */ if (uni->array_elements != 0) { - if (offset >= uni->array_elements) - return; - count = MIN2(count, (int) (uni->array_elements - offset)); } @@ -1021,10 +1018,13 @@ _mesa_get_uniform_location(struct gl_context *ctx, if (!found) return GL_INVALID_INDEX; - /* Since array_elements is 0 for non-arrays, this causes look-ups of 'a[0]' - * to (correctly) fail if 'a' is not an array. + /* If the uniform is an array, fail if the index is out of bounds. + * (A negative index is caught above.) This also fails if the uniform + * is not an array, but the user is trying to index it, because + * array_elements is zero and offset >= 0. */ - if (array_lookup && shProg->UniformStorage[location].array_elements == 0) { + if (array_lookup + && offset >= shProg->UniformStorage[location].array_elements) { return GL_INVALID_INDEX; } </blockquote> </div></div></blockquote></div> </div>