[Mesa-dev] [PATCH] prog_parameter.c ASAN Patch
Brian Paul
brianp at vmware.com
Tue Jul 2 13:57:41 PDT 2013
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 <brianp at vmware.com
> <mailto:brianp at 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.maxfield at gmail.com <mailto:myles.maxfield at gmail.com>
> <mailto: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> <mailto: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>
> <mailto: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/>
> <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