[Piglit] [PATCH 08/11] arb_get_program_binary: Verify glGet* doesn't over-run the buffer for binary format queries
Paul Berry
stereotype441 at gmail.com
Fri Oct 4 11:42:54 PDT 2013
On 21 August 2013 09:08, Ian Romanick <idr at freedesktop.org> wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> This test works with both GL_PROGRAM_BINARY_FORMATS (from
> ARB_get_program_binary) and GL_SHADER_BINARY_FORMATS (from
> ARB_ES2_compatibility). Mesa fails GL_SHADER_BINARY_FORMATS because it
> generates an error (!) for the queries. Fix the error causes it to
> over-run the buffer. I suspect the over-run will happen in an ES2 with
> Mesa as-is.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>
I'm not a fan of the one-word function names in this patch, especially
"munge". How about something like:
munge -> fill_with_garbage
check -> check_overrun
try -> test_params
> ---
> tests/all.tests | 7 +
> tests/spec/CMakeLists.txt | 1 +
> .../spec/arb_get_program_binary/CMakeLists.gl.txt | 14 ++
> tests/spec/arb_get_program_binary/CMakeLists.txt | 1 +
> tests/spec/arb_get_program_binary/overrun.c | 156
> +++++++++++++++++++++
> 5 files changed, 179 insertions(+)
> create mode 100644 tests/spec/arb_get_program_binary/CMakeLists.gl.txt
> create mode 100644 tests/spec/arb_get_program_binary/CMakeLists.txt
> create mode 100644 tests/spec/arb_get_program_binary/overrun.c
>
> diff --git a/tests/all.tests b/tests/all.tests
> index ab75f99..fa3812f 100644
> --- a/tests/all.tests
> +++ b/tests/all.tests
> @@ -1031,6 +1031,13 @@ arb_es2_compatibility['FBO blit to missing
> attachment (ES2 completeness rules)']
> arb_es2_compatibility['FBO blit from missing attachment (ES2 completeness
> rules)'] = PlainExecTest(['fbo-missing-attachment-blit', '-auto', 'es2',
> 'from'])
> add_fbo_formats_tests('spec/ARB_ES2_compatibility',
> 'GL_ARB_ES2_compatibility')
> add_texwrap_format_tests(arb_es2_compatibility,
> 'GL_ARB_ES2_compatibility')
> +arb_es2_compatibility['NUM_SHADER_BINARY_FORMATS over-run check'] =
> concurrent_test('arb_get_program_binary-overrun shader')
> +
> +
> +# Group ARB_get_program_binary
> +arb_get_program_binary = Group()
> +spec['ARB_get_program_binary'] = arb_get_program_binary
> +arb_get_program_binary['NUM_PROGRAM_BINARY_FORMATS over-run check'] =
> concurrent_test('arb_get_program_binary-overrun program')
>
> arb_depth_clamp = Group()
> spec['ARB_depth_clamp'] = arb_depth_clamp
> diff --git a/tests/spec/CMakeLists.txt b/tests/spec/CMakeLists.txt
> index 77ed821..9016c9e 100644
> --- a/tests/spec/CMakeLists.txt
> +++ b/tests/spec/CMakeLists.txt
> @@ -7,6 +7,7 @@ add_subdirectory (arb_es2_compatibility)
> add_subdirectory (arb_framebuffer_object)
> add_subdirectory (arb_framebuffer_srgb)
> add_subdirectory (arb_geometry_shader4)
> +add_subdirectory (arb_get_program_binary)
> add_subdirectory (arb_instanced_arrays)
> add_subdirectory (arb_internalformat_query)
> add_subdirectory (arb_map_buffer_alignment)
> diff --git a/tests/spec/arb_get_program_binary/CMakeLists.gl.txt
> b/tests/spec/arb_get_program_binary/CMakeLists.gl.txt
> new file mode 100644
> index 0000000..be52044
> --- /dev/null
> +++ b/tests/spec/arb_get_program_binary/CMakeLists.gl.txt
> @@ -0,0 +1,14 @@
> +include_directories(
> + ${GLEXT_INCLUDE_DIR}
> + ${OPENGL_INCLUDE_PATH}
> +)
> +
> +link_libraries (
> + piglitutil_${piglit_target_api}
> + ${OPENGL_gl_LIBRARY}
> + ${OPENGL_glu_LIBRARY}
> +)
> +
> +piglit_add_executable (arb_get_program_binary-overrun overrun.c)
> +
> +# vim: ft=cmake:
> diff --git a/tests/spec/arb_get_program_binary/CMakeLists.txt
> b/tests/spec/arb_get_program_binary/CMakeLists.txt
> new file mode 100644
> index 0000000..144a306
> --- /dev/null
> +++ b/tests/spec/arb_get_program_binary/CMakeLists.txt
> @@ -0,0 +1 @@
> +piglit_include_target_api()
> diff --git a/tests/spec/arb_get_program_binary/overrun.c
> b/tests/spec/arb_get_program_binary/overrun.c
> new file mode 100644
> index 0000000..8416677
> --- /dev/null
> +++ b/tests/spec/arb_get_program_binary/overrun.c
> @@ -0,0 +1,156 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +/**
> + * \file overrun.c
> + * Verify that queries don't over-run the size of the supplied buffer.
> + */
> +
> +#include "piglit-util-gl-common.h"
> +#include <stdlib.h>
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> + config.window_visual = PIGLIT_GL_VISUAL_RGB;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + return PIGLIT_FAIL;
> +}
> +
> +/**
> + * Fill the real buffer and the scratch buffer with the same set of
> + * garbage data.
> + */
> +static void
> +munge(void *buffer, void *buffer_scratch, size_t buffer_size_in_bytes)
>
+{
> + unsigned i;
> + uint32_t *scratch = (uint32_t *) buffer;
> +
> + assert(buffer_size_in_bytes % sizeof(uint32_t) == 0);
> + for (i = 0; i < (buffer_size_in_bytes / sizeof(uint32_t)); i++)
> + do {
> + scratch[i] = rand();
>
In my experience, tests that rely on rand() are generally a bad idea
because they can lead to inconsistent results from one run to the next due
to concidences. I realize that's unlikely with this test, but I would feel
much more comfortable if we would just fill the buffer with a recognizable
bit pattern (e.g. 0xcc) rather than random data.
> + } while (scratch[i] == 0);
> +
> + memcpy(buffer_scratch, buffer, buffer_size_in_bytes);
> +}
> +
> +static bool
> +check(const void *buffer, const void *buffer_scratch,
> + size_t buffer_size_in_bytes, size_t data_size_in_bytes,
> + const char *getter_name, const char *enum_name)
> +{
> + if (memcmp(buffer + data_size_in_bytes,
> + buffer_scratch + data_size_in_bytes,
> + buffer_size_in_bytes - data_size_in_bytes) == 0)
> + return true;
> +
> + fprintf(stderr, "%s(%s) over-ran the buffer\n",
> + getter_name, enum_name);
> + return false;
> +}
> +
> +static bool
> +try(GLenum pname1, GLenum pname2)
>
This function would be a lot easier to read if we renamed pname1 and pname2
to reflect how they are used, e.g.:
pname1 -> num_binary_formats_enum
pname2 -> binary_formats_enum
> +{
> + bool pass = true;
> + GLint count = 0;
> + char *buffer;
> + char *buffer_scratch;
> + unsigned buffer_size_in_elements;
> + size_t buffer_size_in_bytes;
> +
> +
> + glGetIntegerv(pname1, &count);
> +
> + if (!piglit_check_gl_error(0))
> + return false;
> +
> + if (count < 0) {
> + fprintf(stderr, "%s returned %d\n",
> + piglit_get_gl_enum_name(pname1),
> + count);
> + return false;
> + }
> +
> + /* Both of the queries can return zero, so make sure that at least
> one
> + * element is allocated. We need to check that when the first
> query
> + * returns zero, the second query doesn't write any data.
> + */
> + buffer_size_in_elements = (count + 1) * 2;
> + buffer_size_in_bytes = buffer_size_in_elements * sizeof(GLint64);
> + buffer = malloc(buffer_size_in_bytes);
> + buffer_scratch = malloc(buffer_size_in_bytes);
> +
> +#define TEST_CASE(fn, type) \
> + do { \
> + munge(buffer, buffer_scratch, buffer_size_in_bytes); \
> + fn (pname2, (type *) buffer); \
> + pass = piglit_check_gl_error(0) && pass; \
> + pass = check(buffer, buffer_scratch, \
> + buffer_size_in_bytes, \
> + sizeof(type) * count, \
> + # fn, \
> + piglit_get_gl_enum_name(pname2)) \
> + && pass; \
> + } while (0)
> +
> + TEST_CASE(glGetBooleanv, GLboolean);
> + TEST_CASE(glGetIntegerv, GLint);
> + TEST_CASE(glGetInteger64v, GLint64);
> + TEST_CASE(glGetFloatv, GLfloat);
> + TEST_CASE(glGetDoublev, GLdouble);
> +
> + free(buffer);
> + free(buffer_scratch);
> + return pass;
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + bool pass = true;
> +
> + if (argc == 1) {
> + fprintf(stderr, "Usage: %s [shader|program]\n", argv[0]);
> + piglit_report_result(PIGLIT_FAIL);
> + }
> +
> + if (strcmp(argv[1], "shader") == 0) {
> + piglit_require_extension("GL_ARB_ES2_compatibility");
> + pass = try(GL_NUM_SHADER_BINARY_FORMATS,
> + GL_SHADER_BINARY_FORMATS);
> + } else {
> + piglit_require_extension("GL_ARB_get_program_binary");
> + pass = try(GL_NUM_PROGRAM_BINARY_FORMATS,
> + GL_PROGRAM_BINARY_FORMATS);
> + }
> +
> + piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
> +}
> --
> 1.8.1.4
>
With those changes, the patch is:
Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20131004/8fefa90b/attachment.html>
More information about the Piglit
mailing list