Re: [Piglit] [PATCH] Fix out of bounds read in the AMD_performance_monitor measure test.

2013-09-24 Thread Ian Romanick
On 09/19/2013 03:41 PM, Kenneth Graunke wrote:
 If the last counter returned is a 32-bit value, reading a uint64_t might
 go past the end of the buffer.  So, delay reading the 64-bit value until
 we know the type is 64-bit.
 
 Signed-off-by: Kenneth Graunke kenn...@whitecape.org

Reviewed-by: Ian Romanick ian.d.roman...@intel.com

Some semi-random thoughts below...

 ---
  tests/spec/amd_performance_monitor/measure.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/tests/spec/amd_performance_monitor/measure.c 
 b/tests/spec/amd_performance_monitor/measure.c
 index bc39c84..61c98b1 100644
 --- a/tests/spec/amd_performance_monitor/measure.c
 +++ b/tests/spec/amd_performance_monitor/measure.c
 @@ -194,7 +194,6 @@ test_basic_measurement(unsigned group)
   /* Counter values */
   uint32_t u32 = p[2];
   float f = ((float *) p)[2];
 - uint64_t u64 = ((uint64_t *) p)[1];
  
   /* Query results */
   GLenum counter_type = GL_NONE;
 @@ -226,6 +225,7 @@ test_basic_measurement(unsigned group)
   break;
   }
   case GL_UNSIGNED_INT64_AMD: {
 + uint64_t u64 = ((uint64_t *) p)[1];

This looks so weird to me, given that the others use [2].  The
alternative is also a face only a mother could love:

uint64_t u64 = *(uint64_t *)(p + 2);

Of course, then the other usages could follow the same pattern:

/* Counter values */
uint32_t u32 = *(uint32_t *)(p + 2);
float f = *(float *)(p + 2);

What do you think?

   verify(u64 = range[0]);
   verify(u64 = range[1]);
   break;
 

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit


[Piglit] [PATCH] Fix out of bounds read in the AMD_performance_monitor measure test.

2013-09-19 Thread Kenneth Graunke
If the last counter returned is a 32-bit value, reading a uint64_t might
go past the end of the buffer.  So, delay reading the 64-bit value until
we know the type is 64-bit.

Signed-off-by: Kenneth Graunke kenn...@whitecape.org
---
 tests/spec/amd_performance_monitor/measure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/spec/amd_performance_monitor/measure.c 
b/tests/spec/amd_performance_monitor/measure.c
index bc39c84..61c98b1 100644
--- a/tests/spec/amd_performance_monitor/measure.c
+++ b/tests/spec/amd_performance_monitor/measure.c
@@ -194,7 +194,6 @@ test_basic_measurement(unsigned group)
/* Counter values */
uint32_t u32 = p[2];
float f = ((float *) p)[2];
-   uint64_t u64 = ((uint64_t *) p)[1];
 
/* Query results */
GLenum counter_type = GL_NONE;
@@ -226,6 +225,7 @@ test_basic_measurement(unsigned group)
break;
}
case GL_UNSIGNED_INT64_AMD: {
+   uint64_t u64 = ((uint64_t *) p)[1];
verify(u64 = range[0]);
verify(u64 = range[1]);
break;
-- 
1.8.3.4

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit