Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch

2013-07-02 Thread Brian Paul
OK, so I actually tested it (and fixed it) now and will post a patch for 
review shortly.


-Brian


On 07/01/2013 04:09 PM, Myles C. Maxfield wrote:

Looks good to me. Thanks for fixing it up. Do the prospects look good
for getting this committed?

It would be cool if my name was attached to the patch, but since you
really ended up writing it, its fine with me if you're marked as the author.

Thanks,
Myles


On Mon, Jul 1, 2013 at 2:08 PM, Brian Paul mailto:bri...@vmware.com>> wrote:

I took a closer look at your patch and I think it's incorrect.

Note that the 'values' pointer is incremented by 4 in each loop
iteration and size is decremented by 4.  So accessing values[i*4+j]
will eventually go out of bounds.

I think something like this would work.

diff --git a/src/mesa/program/prog___parameter.c
b/src/mesa/program/prog___parameter.c
index 95b153e..1b16feb 100644
--- a/src/mesa/program/prog___parameter.c
+++ b/src/mesa/program/prog___parameter.c
@@ -155,7 +155,17 @@ _mesa_add_parameter(struct
gl_program_parameter_list *paramList,

   p->Size = size;
   p->DataType = datatype;
   if (values) {
-COPY_4V(paramList->__ParameterValues[oldNum + i], values);
+if (size >= 4) {

+   COPY_4V(paramList->__ParameterValues[oldNum + i],
values);
+}
+else {

+   for (j = 0; j < size; j++) {
+  paramList->ParameterValues[__oldNum + i][j] =
values[j];
+   }
+   for (; j < 4; j++) {
+  paramList->ParameterValues[__oldNum + i][j] = 0.0f;

+   }
+}
  values += 4;
  p->Initialized = GL_TRUE;
   }

-Brian



On 06/19/2013 01:47 AM, Myles C. Maxfield wrote:

Any word on this?

Thanks,
Myles


On Mon, Jun 17, 2013 at 12:09 PM, Myles C. Maxfield
mailto:myles.maxfi...@gmail.com>
>> wrote:

 Sure. I was under the impression that |size| couldn't be both
 greater than 4 and a non-multiple of 4, but I've reworked
the patch
 to incorporate this and to be a little more straightforward.

 Is the only way to replace "ASAN" with "Address Sanitizer"
to change
 the subject of this email thread?

 Anyway, here's a similar but modified patch:

 From: Myles C. Maxfield mailto:my...@amazon.com> >>

 Date: Mon, 17 Jun 2013 11:50:05 -0700
 Subject: [PATCH] Appeasing Address Sanitizer

 ---
   src/mesa/program/prog___parameter.c | 13 -
   1 file changed, 12 insertions(+), 1 deletion(-)

 diff --git a/src/mesa/program/prog___parameter.c
 b/src/mesa/program/prog___parameter.c
 index 95b153e..1d46476 100644
 --- a/src/mesa/program/prog___parameter.c
 +++ b/src/mesa/program/prog___parameter.c
 @@ -155,7 +155,18 @@ _mesa_add_parameter(struct
 gl_program_parameter_list *paramList,
p->Size = size;
p->DataType = datatype;
if (values) {
 -COPY_4V(paramList->__ParameterValues[oldNum +
i], values);
 +if (size >= (i+1)*4) {
 +
  COPY_4V(paramList->__ParameterValues[oldNum + i],
 values);
 +} else {
 +/* silence asan */
 +for (j = 0; j < 4; j++) {
 +if (i*4+j < size) {
 +
  paramList->ParameterValues[__oldNum + i][j] =
 values[i*4+j];
 +} else {
 +
  paramList->ParameterValues[__oldNum + i][j].f
 = 0.0f;
 +}
 +}
 +}
   values += 4;
   p->Initialized = GL_TRUE;
}
 --
 1.7.12.4 (Apple Git-37)


 On Mon, Jun 17, 2013 at 8:13 AM, Brian Paul
mailto:bri...@vmware.com>
 >> wrote:

 On 06/14/2013 05:12 PM, Myles C. Maxfield wrote:

 Sorry for the triple post; I received a bounce
email the
 first time and got sent to the spam folder the
second time,
 so I'm trying a third time.

 Hello, all. I was running Mesa with Address
Sanitizer [1]
 turne

Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch

2013-07-01 Thread Myles C. Maxfield
Looks good to me. Thanks for fixing it up. Do the prospects look good for
getting this committed?

It would be cool if my name was attached to the patch, but since you really
ended up writing it, its fine with me if you're marked as the author.

Thanks,
Myles


On Mon, Jul 1, 2013 at 2:08 PM, Brian Paul  wrote:

> I took a closer look at your patch and I think it's incorrect.
>
> Note that the 'values' pointer is incremented by 4 in each loop iteration
> and size is decremented by 4.  So accessing values[i*4+j] will eventually
> go out of bounds.
>
> I think something like this would work.
>
> diff --git a/src/mesa/program/prog_**parameter.c b/src/mesa/program/prog_*
> *parameter.c
> index 95b153e..1b16feb 100644
> --- a/src/mesa/program/prog_**parameter.c
> +++ b/src/mesa/program/prog_**parameter.c
> @@ -155,7 +155,17 @@ _mesa_add_parameter(struct gl_program_parameter_list
> *paramList,
>
>   p->Size = size;
>   p->DataType = datatype;
>   if (values) {
> -COPY_4V(paramList->**ParameterValues[oldNum + i], values);
> +if (size >= 4) {
>
> +   COPY_4V(paramList->**ParameterValues[oldNum + i], values);
> +}
> +else {
>
> +   for (j = 0; j < size; j++) {
> +  paramList->ParameterValues[**oldNum + i][j] =
> values[j];
> +   }
> +   for (; j < 4; j++) {
> +  paramList->ParameterValues[**oldNum + i][j] = 0.0f;
>
> +   }
> +}
>  values += 4;
>  p->Initialized = GL_TRUE;
>   }
>
> -Brian
>
>
>
> On 06/19/2013 01:47 AM, Myles C. Maxfield wrote:
>
>> Any word on this?
>>
>> Thanks,
>> Myles
>>
>>
>> On Mon, Jun 17, 2013 at 12:09 PM, Myles C. Maxfield
>> > >
>> wrote:
>>
>> Sure. I was under the impression that |size| couldn't be both
>> greater than 4 and a non-multiple of 4, but I've reworked the patch
>> to incorporate this and to be a little more straightforward.
>>
>> Is the only way to replace "ASAN" with "Address Sanitizer" to change
>> the subject of this email thread?
>>
>> Anyway, here's a similar but modified patch:
>>
>> From: Myles C. Maxfield mailto:my...@amazon.com>>
>>
>> Date: Mon, 17 Jun 2013 11:50:05 -0700
>> Subject: [PATCH] Appeasing Address Sanitizer
>>
>> ---
>>   src/mesa/program/prog_**parameter.c | 13 -
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/program/prog_**parameter.c
>> b/src/mesa/program/prog_**parameter.c
>> index 95b153e..1d46476 100644
>> --- a/src/mesa/program/prog_**parameter.c
>> +++ b/src/mesa/program/prog_**parameter.c
>> @@ -155,7 +155,18 @@ _mesa_add_parameter(struct
>> gl_program_parameter_list *paramList,
>>p->Size = size;
>>p->DataType = datatype;
>>if (values) {
>> -COPY_4V(paramList->**ParameterValues[oldNum + i],
>> values);
>> +if (size >= (i+1)*4) {
>> +COPY_4V(paramList->**ParameterValues[oldNum + i],
>> values);
>> +} else {
>> +/* silence asan */
>> +for (j = 0; j < 4; j++) {
>> +if (i*4+j < size) {
>> +paramList->ParameterValues[**oldNum + i][j]
>> =
>> values[i*4+j];
>> +} else {
>> +paramList->ParameterValues[**oldNum +
>> i][j].f
>> = 0.0f;
>> +}
>> +}
>> +}
>>   values += 4;
>>   p->Initialized = GL_TRUE;
>>}
>> --
>> 1.7.12.4 (Apple Git-37)
>>
>>
>> On Mon, Jun 17, 2013 at 8:13 AM, Brian Paul > > wrote:
>>
>> On 06/14/2013 05:12 PM, Myles C. Maxfield wrote:
>>
>> Sorry for the triple post; I received a bounce email the
>> first time and got sent to the spam folder the second time,
>> so I'm trying a third time.
>>
>> Hello, all. I was running Mesa with Address Sanitizer [1]
>> turned on, and found one place where ASAN pointed out a
>> read-before-initialized problem. In particular, in
>> _mesa_add_parameter, in prog_parameter.c, |values|
>> represents an array holding a variable number of values.
>> These values get copied out of the array 4 at a time with
>> the COPY_4V macro, however, the array might only contain a
>> single element. In this case, ASAN reports a
>> read-before-initialize because the last 3 of the 4 elements
>> haven't been written to yet. I was hoping to contribute a
>> patch that will silence this problem that ASAN reports. I'm
>> happy to incorporate any feedback anyone has into

Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch

2013-07-01 Thread Brian Paul

I took a closer look at your patch and I think it's incorrect.

Note that the 'values' pointer is incremented by 4 in each loop 
iteration and size is decremented by 4.  So accessing values[i*4+j] will 
eventually go out of bounds.


I think something like this would work.

diff --git a/src/mesa/program/prog_parameter.c 
b/src/mesa/program/prog_parameter.c

index 95b153e..1b16feb 100644
--- a/src/mesa/program/prog_parameter.c
+++ b/src/mesa/program/prog_parameter.c
@@ -155,7 +155,17 @@ _mesa_add_parameter(struct 
gl_program_parameter_list *paramList,

  p->Size = size;
  p->DataType = datatype;
  if (values) {
-COPY_4V(paramList->ParameterValues[oldNum + i], values);
+if (size >= 4) {
+   COPY_4V(paramList->ParameterValues[oldNum + i], values);
+}
+else {
+   for (j = 0; j < size; j++) {
+  paramList->ParameterValues[oldNum + i][j] = values[j];
+   }
+   for (; j < 4; j++) {
+  paramList->ParameterValues[oldNum + i][j] = 0.0f;
+   }
+}
 values += 4;
 p->Initialized = GL_TRUE;
  }

-Brian


On 06/19/2013 01:47 AM, Myles C. Maxfield wrote:

Any word on this?

Thanks,
Myles


On Mon, Jun 17, 2013 at 12:09 PM, Myles C. Maxfield
mailto:myles.maxfi...@gmail.com>> wrote:

Sure. I was under the impression that |size| couldn't be both
greater than 4 and a non-multiple of 4, but I've reworked the patch
to incorporate this and to be a little more straightforward.

Is the only way to replace "ASAN" with "Address Sanitizer" to change
the subject of this email thread?

Anyway, here's a similar but modified patch:

From: Myles C. Maxfield mailto:my...@amazon.com>>
Date: Mon, 17 Jun 2013 11:50:05 -0700
Subject: [PATCH] Appeasing Address Sanitizer

---
  src/mesa/program/prog_parameter.c | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/mesa/program/prog_parameter.c
b/src/mesa/program/prog_parameter.c
index 95b153e..1d46476 100644
--- a/src/mesa/program/prog_parameter.c
+++ b/src/mesa/program/prog_parameter.c
@@ -155,7 +155,18 @@ _mesa_add_parameter(struct
gl_program_parameter_list *paramList,
   p->Size = size;
   p->DataType = datatype;
   if (values) {
-COPY_4V(paramList->ParameterValues[oldNum + i], values);
+if (size >= (i+1)*4) {
+COPY_4V(paramList->ParameterValues[oldNum + i],
values);
+} else {
+/* silence asan */
+for (j = 0; j < 4; j++) {
+if (i*4+j < size) {
+paramList->ParameterValues[oldNum + i][j] =
values[i*4+j];
+} else {
+paramList->ParameterValues[oldNum + i][j].f
= 0.0f;
+}
+}
+}
  values += 4;
  p->Initialized = GL_TRUE;
   }
--
1.7.12.4 (Apple Git-37)


On Mon, Jun 17, 2013 at 8:13 AM, Brian Paul mailto:bri...@vmware.com>> wrote:

On 06/14/2013 05:12 PM, Myles C. Maxfield wrote:

Sorry for the triple post; I received a bounce email the
first time and got sent to the spam folder the second time,
so I'm trying a third time.

Hello, all. I was running Mesa with Address Sanitizer [1]
turned on, and found one place where ASAN pointed out a
read-before-initialized problem. In particular, in
_mesa_add_parameter, in prog_parameter.c, |values|
represents an array holding a variable number of values.
These values get copied out of the array 4 at a time with
the COPY_4V macro, however, the array might only contain a
single element. In this case, ASAN reports a
read-before-initialize because the last 3 of the 4 elements
haven't been written to yet. I was hoping to contribute a
patch that will silence this problem that ASAN reports. I'm
happy to incorporate any feedback anyone has into this patch.

Thanks,
Myles C. Maxfield

[1]https://code.google.com/p/__address-sanitizer/


diff --git a/src/mesa/program/prog___parameter.c
b/src/mesa/program/prog___parameter.c
index 2018fa5..63915fb 100644
--- a/src/mesa/program/prog___parameter.c
+++ b/src/mesa/program/prog___parameter.c
@@ -158,7 +158,17 @@ _mesa_add_parameter(struct
gl_program_parameter_list *paramList,
p->DataType = datatype;
p->Flags = flags;
if

Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch

2013-06-28 Thread Myles C. Maxfield
Friendly ping regarding this patch :-)

--Myles


On Wed, Jun 19, 2013 at 12:47 AM, Myles C. Maxfield <
myles.maxfi...@gmail.com> wrote:

> Any word on this?
>
> Thanks,
> Myles
>
>
> On Mon, Jun 17, 2013 at 12:09 PM, Myles C. Maxfield <
> myles.maxfi...@gmail.com> wrote:
>
>> Sure. I was under the impression that |size| couldn't be both greater
>> than 4 and a non-multiple of 4, but I've reworked the patch to incorporate
>> this and to be a little more straightforward.
>>
>> Is the only way to replace "ASAN" with "Address Sanitizer" to change the
>> subject of this email thread?
>>
>> Anyway, here's a similar but modified patch:
>>
>> From: Myles C. Maxfield 
>> Date: Mon, 17 Jun 2013 11:50:05 -0700
>> Subject: [PATCH] Appeasing Address Sanitizer
>>
>> ---
>>  src/mesa/program/prog_parameter.c | 13 -
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/program/prog_parameter.c
>> b/src/mesa/program/prog_parameter.c
>> index 95b153e..1d46476 100644
>> --- a/src/mesa/program/prog_parameter.c
>> +++ b/src/mesa/program/prog_parameter.c
>> @@ -155,7 +155,18 @@ _mesa_add_parameter(struct gl_program_parameter_list
>> *paramList,
>>   p->Size = size;
>>   p->DataType = datatype;
>>   if (values) {
>> -COPY_4V(paramList->ParameterValues[oldNum + i], values);
>> +if (size >= (i+1)*4) {
>> +COPY_4V(paramList->ParameterValues[oldNum + i], values);
>> +} else {
>> +/* silence asan */
>> +for (j = 0; j < 4; j++) {
>> +if (i*4+j < size) {
>> +paramList->ParameterValues[oldNum + i][j] =
>> values[i*4+j];
>> +} else {
>> +paramList->ParameterValues[oldNum + i][j].f =
>> 0.0f;
>> +}
>> +}
>> +}
>>  values += 4;
>>  p->Initialized = GL_TRUE;
>>   }
>> --
>> 1.7.12.4 (Apple Git-37)
>>
>>
>> On Mon, Jun 17, 2013 at 8:13 AM, Brian Paul  wrote:
>>
>>> On 06/14/2013 05:12 PM, Myles C. Maxfield wrote:
>>>
 Sorry for the triple post; I received a bounce email the first time and
 got sent to the spam folder the second time, so I'm trying a third time.

 Hello, all. I was running Mesa with Address Sanitizer [1] turned on,
 and found one place where ASAN pointed out a read-before-initialized
 problem. In particular, in _mesa_add_parameter, in prog_parameter.c,
 |values| represents an array holding a variable number of values. These
 values get copied out of the array 4 at a time with the COPY_4V macro,
 however, the array might only contain a single element. In this case, ASAN
 reports a read-before-initialize because the last 3 of the 4 elements
 haven't been written to yet. I was hoping to contribute a patch that will
 silence this problem that ASAN reports. I'm happy to incorporate any
 feedback anyone has into this patch.

 Thanks,
 Myles C. Maxfield

 [1]https://code.google.com/p/**address-sanitizer/

 diff --git a/src/mesa/program/prog_**parameter.c
 b/src/mesa/program/prog_**parameter.c
 index 2018fa5..63915fb 100644
 --- a/src/mesa/program/prog_**parameter.c
 +++ b/src/mesa/program/prog_**parameter.c
 @@ -158,7 +158,17 @@ _mesa_add_parameter(struct
 gl_program_parameter_list *paramList,
p->DataType = datatype;
p->Flags = flags;
if (values) {
 -COPY_4V(paramList->**ParameterValues[oldNum + i], values);
 +if (size & 3) {
 +  for (j = 0; j < size; j++) {
 +paramList->ParameterValues[**oldNum + i][j] =
 values[j];
 +  }
 +  /* silence asan */
 +  for (j = size; j < 4; j++) {
 +paramList->ParameterValues[**oldNum + i][j].f = 0;
 +  }
 +} else {
 +  COPY_4V(paramList->**ParameterValues[oldNum + i],
 values);
 +}
   values += 4;
   p->Initialized = GL_TRUE;
}

>>>
>>> The value of 'size' can actually be greater than 4 (IIRC, and the
>>> function comment are still correct).  For example, for a matrix, size=16.
>>>  So the first for-loop should be fixed, just to be safe.
>>>
>>> In the commit message, let's not use "ASAN" since it's not obvious that
>>> it means Address Sanitizer.
>>>
>>> -Brian
>>>
>>>
>>>
>>>
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch

2013-06-19 Thread Myles C. Maxfield
Any word on this?

Thanks,
Myles


On Mon, Jun 17, 2013 at 12:09 PM, Myles C. Maxfield <
myles.maxfi...@gmail.com> wrote:

> Sure. I was under the impression that |size| couldn't be both greater than
> 4 and a non-multiple of 4, but I've reworked the patch to incorporate this
> and to be a little more straightforward.
>
> Is the only way to replace "ASAN" with "Address Sanitizer" to change the
> subject of this email thread?
>
> Anyway, here's a similar but modified patch:
>
> From: Myles C. Maxfield 
> Date: Mon, 17 Jun 2013 11:50:05 -0700
> Subject: [PATCH] Appeasing Address Sanitizer
>
> ---
>  src/mesa/program/prog_parameter.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/program/prog_parameter.c
> b/src/mesa/program/prog_parameter.c
> index 95b153e..1d46476 100644
> --- a/src/mesa/program/prog_parameter.c
> +++ b/src/mesa/program/prog_parameter.c
> @@ -155,7 +155,18 @@ _mesa_add_parameter(struct gl_program_parameter_list
> *paramList,
>   p->Size = size;
>   p->DataType = datatype;
>   if (values) {
> -COPY_4V(paramList->ParameterValues[oldNum + i], values);
> +if (size >= (i+1)*4) {
> +COPY_4V(paramList->ParameterValues[oldNum + i], values);
> +} else {
> +/* silence asan */
> +for (j = 0; j < 4; j++) {
> +if (i*4+j < size) {
> +paramList->ParameterValues[oldNum + i][j] =
> values[i*4+j];
> +} else {
> +paramList->ParameterValues[oldNum + i][j].f =
> 0.0f;
> +}
> +}
> +}
>  values += 4;
>  p->Initialized = GL_TRUE;
>   }
> --
> 1.7.12.4 (Apple Git-37)
>
>
> On Mon, Jun 17, 2013 at 8:13 AM, Brian Paul  wrote:
>
>> On 06/14/2013 05:12 PM, Myles C. Maxfield wrote:
>>
>>> Sorry for the triple post; I received a bounce email the first time and
>>> got sent to the spam folder the second time, so I'm trying a third time.
>>>
>>> Hello, all. I was running Mesa with Address Sanitizer [1] turned on, and
>>> found one place where ASAN pointed out a read-before-initialized problem.
>>> In particular, in _mesa_add_parameter, in prog_parameter.c, |values|
>>> represents an array holding a variable number of values. These values get
>>> copied out of the array 4 at a time with the COPY_4V macro, however, the
>>> array might only contain a single element. In this case, ASAN reports a
>>> read-before-initialize because the last 3 of the 4 elements haven't been
>>> written to yet. I was hoping to contribute a patch that will silence this
>>> problem that ASAN reports. I'm happy to incorporate any feedback anyone has
>>> into this patch.
>>>
>>> Thanks,
>>> Myles C. Maxfield
>>>
>>> [1]https://code.google.com/p/**address-sanitizer/
>>>
>>> diff --git a/src/mesa/program/prog_**parameter.c
>>> b/src/mesa/program/prog_**parameter.c
>>> index 2018fa5..63915fb 100644
>>> --- a/src/mesa/program/prog_**parameter.c
>>> +++ b/src/mesa/program/prog_**parameter.c
>>> @@ -158,7 +158,17 @@ _mesa_add_parameter(struct
>>> gl_program_parameter_list *paramList,
>>>p->DataType = datatype;
>>>p->Flags = flags;
>>>if (values) {
>>> -COPY_4V(paramList->**ParameterValues[oldNum + i], values);
>>> +if (size & 3) {
>>> +  for (j = 0; j < size; j++) {
>>> +paramList->ParameterValues[**oldNum + i][j] =
>>> values[j];
>>> +  }
>>> +  /* silence asan */
>>> +  for (j = size; j < 4; j++) {
>>> +paramList->ParameterValues[**oldNum + i][j].f = 0;
>>> +  }
>>> +} else {
>>> +  COPY_4V(paramList->**ParameterValues[oldNum + i],
>>> values);
>>> +}
>>>   values += 4;
>>>   p->Initialized = GL_TRUE;
>>>}
>>>
>>
>> The value of 'size' can actually be greater than 4 (IIRC, and the
>> function comment are still correct).  For example, for a matrix, size=16.
>>  So the first for-loop should be fixed, just to be safe.
>>
>> In the commit message, let's not use "ASAN" since it's not obvious that
>> it means Address Sanitizer.
>>
>> -Brian
>>
>>
>>
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch

2013-06-17 Thread Myles C. Maxfield
Sure. I was under the impression that |size| couldn't be both greater than
4 and a non-multiple of 4, but I've reworked the patch to incorporate this
and to be a little more straightforward.

Is the only way to replace "ASAN" with "Address Sanitizer" to change the
subject of this email thread?

Anyway, here's a similar but modified patch:

From: Myles C. Maxfield 
Date: Mon, 17 Jun 2013 11:50:05 -0700
Subject: [PATCH] Appeasing Address Sanitizer

---
 src/mesa/program/prog_parameter.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/mesa/program/prog_parameter.c
b/src/mesa/program/prog_parameter.c
index 95b153e..1d46476 100644
--- a/src/mesa/program/prog_parameter.c
+++ b/src/mesa/program/prog_parameter.c
@@ -155,7 +155,18 @@ _mesa_add_parameter(struct gl_program_parameter_list
*paramList,
  p->Size = size;
  p->DataType = datatype;
  if (values) {
-COPY_4V(paramList->ParameterValues[oldNum + i], values);
+if (size >= (i+1)*4) {
+COPY_4V(paramList->ParameterValues[oldNum + i], values);
+} else {
+/* silence asan */
+for (j = 0; j < 4; j++) {
+if (i*4+j < size) {
+paramList->ParameterValues[oldNum + i][j] =
values[i*4+j];
+} else {
+paramList->ParameterValues[oldNum + i][j].f = 0.0f;
+}
+}
+}
 values += 4;
 p->Initialized = GL_TRUE;
  }
-- 
1.7.12.4 (Apple Git-37)


On Mon, Jun 17, 2013 at 8:13 AM, Brian Paul  wrote:

> On 06/14/2013 05:12 PM, Myles C. Maxfield wrote:
>
>> Sorry for the triple post; I received a bounce email the first time and
>> got sent to the spam folder the second time, so I'm trying a third time.
>>
>> Hello, all. I was running Mesa with Address Sanitizer [1] turned on, and
>> found one place where ASAN pointed out a read-before-initialized problem.
>> In particular, in _mesa_add_parameter, in prog_parameter.c, |values|
>> represents an array holding a variable number of values. These values get
>> copied out of the array 4 at a time with the COPY_4V macro, however, the
>> array might only contain a single element. In this case, ASAN reports a
>> read-before-initialize because the last 3 of the 4 elements haven't been
>> written to yet. I was hoping to contribute a patch that will silence this
>> problem that ASAN reports. I'm happy to incorporate any feedback anyone has
>> into this patch.
>>
>> Thanks,
>> Myles C. Maxfield
>>
>> [1]https://code.google.com/p/**address-sanitizer/
>>
>> diff --git a/src/mesa/program/prog_**parameter.c b/src/mesa/program/prog_
>> **parameter.c
>> index 2018fa5..63915fb 100644
>> --- a/src/mesa/program/prog_**parameter.c
>> +++ b/src/mesa/program/prog_**parameter.c
>> @@ -158,7 +158,17 @@ _mesa_add_parameter(struct gl_program_parameter_list
>> *paramList,
>>p->DataType = datatype;
>>p->Flags = flags;
>>if (values) {
>> -COPY_4V(paramList->**ParameterValues[oldNum + i], values);
>> +if (size & 3) {
>> +  for (j = 0; j < size; j++) {
>> +paramList->ParameterValues[**oldNum + i][j] = values[j];
>> +  }
>> +  /* silence asan */
>> +  for (j = size; j < 4; j++) {
>> +paramList->ParameterValues[**oldNum + i][j].f = 0;
>> +  }
>> +} else {
>> +  COPY_4V(paramList->**ParameterValues[oldNum + i], values);
>> +}
>>   values += 4;
>>   p->Initialized = GL_TRUE;
>>}
>>
>
> The value of 'size' can actually be greater than 4 (IIRC, and the function
> comment are still correct).  For example, for a matrix, size=16.  So the
> first for-loop should be fixed, just to be safe.
>
> In the commit message, let's not use "ASAN" since it's not obvious that it
> means Address Sanitizer.
>
> -Brian
>
>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch

2013-06-17 Thread Brian Paul

On 06/14/2013 05:12 PM, Myles C. Maxfield wrote:

Sorry for the triple post; I received a bounce email the first time and got 
sent to the spam folder the second time, so I'm trying a third time.

Hello, all. I was running Mesa with Address Sanitizer [1] turned on, and found 
one place where ASAN pointed out a read-before-initialized problem. In 
particular, in _mesa_add_parameter, in prog_parameter.c, |values| represents an 
array holding a variable number of values. These values get copied out of the 
array 4 at a time with the COPY_4V macro, however, the array might only contain 
a single element. In this case, ASAN reports a read-before-initialize because 
the last 3 of the 4 elements haven't been written to yet. I was hoping to 
contribute a patch that will silence this problem that ASAN reports. I'm happy 
to incorporate any feedback anyone has into this patch.

Thanks,
Myles C. Maxfield

[1]https://code.google.com/p/address-sanitizer/

diff --git a/src/mesa/program/prog_parameter.c 
b/src/mesa/program/prog_parameter.c
index 2018fa5..63915fb 100644
--- a/src/mesa/program/prog_parameter.c
+++ b/src/mesa/program/prog_parameter.c
@@ -158,7 +158,17 @@ _mesa_add_parameter(struct gl_program_parameter_list 
*paramList,
   p->DataType = datatype;
   p->Flags = flags;
   if (values) {
-COPY_4V(paramList->ParameterValues[oldNum + i], values);
+if (size & 3) {
+  for (j = 0; j < size; j++) {
+paramList->ParameterValues[oldNum + i][j] = values[j];
+  }
+  /* silence asan */
+  for (j = size; j < 4; j++) {
+paramList->ParameterValues[oldNum + i][j].f = 0;
+  }
+} else {
+  COPY_4V(paramList->ParameterValues[oldNum + i], values);
+}
  values += 4;
  p->Initialized = GL_TRUE;
   }


The value of 'size' can actually be greater than 4 (IIRC, and the 
function comment are still correct).  For example, for a matrix, 
size=16.  So the first for-loop should be fixed, just to be safe.


In the commit message, let's not use "ASAN" since it's not obvious that 
it means Address Sanitizer.


-Brian



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


[Mesa-dev] [PATCH] prog_parameter.c ASAN Patch

2013-06-14 Thread Myles C. Maxfield
Sorry for the triple post; I received a bounce email the first time
and got sent to the spam folder the second time, so I'm trying a third
time.

Hello, all. I was running Mesa with Address Sanitizer [1] turned on,
and found one place where ASAN pointed out a read-before-initialized
problem. In particular, in _mesa_add_parameter, in prog_parameter.c,
|values| represents an array holding a variable number of values.
These values get copied out of the array 4 at a time with the COPY_4V
macro, however, the array might only contain a single element. In this
case, ASAN reports a read-before-initialize because the last 3 of the
4 elements haven't been written to yet. I was hoping to contribute a
patch that will silence this problem that ASAN reports. I'm happy to
incorporate any feedback anyone has into this patch.

Thanks,
Myles C. Maxfield

[1] https://code.google.com/p/address-sanitizer/

diff --git a/src/mesa/program/prog_parameter.c
b/src/mesa/program/prog_parameter.c
index 2018fa5..63915fb 100644
--- a/src/mesa/program/prog_parameter.c
+++ b/src/mesa/program/prog_parameter.c
@@ -158,7 +158,17 @@ _mesa_add_parameter(struct
gl_program_parameter_list *paramList,
  p->DataType = datatype;
  p->Flags = flags;
  if (values) {
-COPY_4V(paramList->ParameterValues[oldNum + i], values);
+if (size & 3) {
+  for (j = 0; j < size; j++) {
+paramList->ParameterValues[oldNum + i][j] = values[j];
+  }
+  /* silence asan */
+  for (j = size; j < 4; j++) {
+paramList->ParameterValues[oldNum + i][j].f = 0;
+  }
+} else {
+  COPY_4V(paramList->ParameterValues[oldNum + i], values);
+}
 values += 4;
 p->Initialized = GL_TRUE;
  }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev