<div dir="ltr">Looks good to me. Thanks for fixing it up. Do the prospects look good for getting this committed?<div> </div><div style>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.</div> <div style> </div><div style>Thanks,</div><div style>Myles</div></div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Jul 1, 2013 at 2:08 PM, Brian Paul <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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,<div class="im"> p->Size = size; p->DataType = datatype; if (values) { - COPY_4V(paramList->ParameterValues[oldNum + i], values); </div> + if (size >= 4) {<div class="im"> + COPY_4V(paramList->ParameterValues[oldNum + i], values); + } </div> + else {<div class="im"> + for (j = 0; j < size; j++) { + paramList->ParameterValues[oldNum + i][j] = values[j]; + } </div> + for (; j < 4; j++) { + paramList->ParameterValues[oldNum + i][j] = 0.0f;<div class="im"> + } + } values += 4; p->Initialized = GL_TRUE; } </div> -Brian<div class="im"> On 06/19/2013 01:47 AM, Myles C. Maxfield wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"> Any word on this? Thanks, Myles On Mon, Jun 17, 2013 at 12:09 PM, Myles C. Maxfield </div><div class="im"> <<a href="mailto:myles.maxfield@gmail.com" target="_blank">myles.maxfield@gmail.com</a> <mailto:<a href="mailto:myles.maxfield@gmail.com" target="_blank">myles.maxfield@gmail.com</a>>> 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: </div> From: Myles C. Maxfield <<a href="mailto:mymax@amazon.com" target="_blank">mymax@amazon.com</a> <mailto:<a href="mailto:mymax@amazon.com" target="_blank">mymax@amazon.com</a>>><div><div class="h5"> 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 <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a> </div></div><div class="im"> <mailto:<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>>> 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 </div> [1]<a href="https://code.google.com/p/__address-sanitizer/" target="_blank">https://code.google.com/p/__address-sanitizer/</a> <<a href="https://code.google.com/p/address-sanitizer/" target="_blank">https://code.google.com/p/address-sanitizer/</a>> 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<div class="im"> @@ -158,7 +158,17 @@ _mesa_add_parameter(struct gl_program_parameter_list *paramList, p->DataType = datatype; p->Flags = flags; if (values) { </div> - COPY_4V(paramList->__ParameterValues[oldNum +<div class="im"> i], values); + if (size & 3) { + for (j = 0; j < size; j++) { </div> + paramList->ParameterValues[__oldNum + i][j]<div class="im"> = values[j]; + } + /* silence asan */ + for (j = size; j < 4; j++) { </div> + paramList->ParameterValues[__oldNum +<div class="im"> i][j].f = 0; + } + } else { </div> + COPY_4V(paramList->__ParameterValues[oldNum +<div class="im"> 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 </div></blockquote> </blockquote></div> </div>