Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch
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 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.maxfield@gmail.**commyles.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 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 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
Re: [Mesa-dev] [PATCH] prog_parameter.c ASAN Patch
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 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 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 ___ 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
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 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 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 ___ 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
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 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 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 ___ 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
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