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

Brian Paul brianp at vmware.com
Mon Jul 1 14:08:32 PDT 2013


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
> <myles.maxfield at gmail.com <mailto:myles.maxfield at 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 <mymax at amazon.com <mailto:mymax at 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 <brianp at vmware.com
>     <mailto:brianp at 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/
>             <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
>
>
>
>
>



More information about the mesa-dev mailing list