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 <bri...@vmware.com
<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
        <myles.maxfi...@gmail.com <mailto:myles.maxfi...@gmail.com>
        <mailto:myles.maxfield@gmail.__com
        <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 <my...@amazon.com
        <mailto:my...@amazon.com> <mailto:my...@amazon.com
        <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
        <bri...@vmware.com <mailto:bri...@vmware.com>
             <mailto:bri...@vmware.com <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/
        <https://code.google.com/p/__address-sanitizer/>
                     <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








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

Reply via email to