shader_runner: Check sscanf return value.

Submitted by Vinson Lee on Nov. 4, 2016, 11:30 p.m.

Details

Message ID 1478302239-16384-1-git-send-email-vlee@freedesktop.org
State New
Headers show
Series "shader_runner: Check sscanf return value." ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Vinson Lee Nov. 4, 2016, 11:30 p.m.
Fix Coverity unchecked return value defect.

CID: 1373655
Signed-off-by: Vinson Lee <vlee@freedesktop.org>
---
 tests/shaders/shader_runner.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
index 02a6cd7cda82..62fc8120a30c 100644
--- a/tests/shaders/shader_runner.c
+++ b/tests/shaders/shader_runner.c
@@ -2769,8 +2769,13 @@  handle_texparameter(const char *line)
 		return;
 	} else if (string_match("border_color ", line)) {
 		float bc[4];
+		int count;
 		line += strlen("border_color ");
-		sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
+		count = sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
+		if (count != 4) {
+			fprintf(stderr, "Could not parse border_color texture parameter.\n");
+			piglit_report_result(PIGLIT_FAIL);
+		}
 		glTexParameterfv(target, GL_TEXTURE_BORDER_COLOR, bc);
 		return;
 	} else if (string_match("swizzle_r ", line)) {

Comments

On 11/04/2016 05:30 PM, Vinson Lee wrote:
> Fix Coverity unchecked return value defect.
>
> CID: 1373655
> Signed-off-by: Vinson Lee <vlee@freedesktop.org>
> ---
>   tests/shaders/shader_runner.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> index 02a6cd7cda82..62fc8120a30c 100644
> --- a/tests/shaders/shader_runner.c
> +++ b/tests/shaders/shader_runner.c
> @@ -2769,8 +2769,13 @@ handle_texparameter(const char *line)
>   		return;
>   	} else if (string_match("border_color ", line)) {
>   		float bc[4];
> +		int count;
>   		line += strlen("border_color ");
> -		sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
> +		count = sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
> +		if (count != 4) {
> +			fprintf(stderr, "Could not parse border_color texture parameter.\n");
> +			piglit_report_result(PIGLIT_FAIL);
> +		}
>   		glTexParameterfv(target, GL_TEXTURE_BORDER_COLOR, bc);
>   		return;
>   	} else if (string_match("swizzle_r ", line)) {
>

Reviewed-by: Brian Paul <brianp@vmware.com>
Quoting Vinson Lee (2016-11-04 16:30:39)
> Fix Coverity unchecked return value defect.
> 
> CID: 1373655
> Signed-off-by: Vinson Lee <vlee@freedesktop.org>
> ---
>  tests/shaders/shader_runner.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> index 02a6cd7cda82..62fc8120a30c 100644
> --- a/tests/shaders/shader_runner.c
> +++ b/tests/shaders/shader_runner.c
> @@ -2769,8 +2769,13 @@ handle_texparameter(const char *line)
>                 return;
>         } else if (string_match("border_color ", line)) {
>                 float bc[4];
> +               int count;
>                 line += strlen("border_color ");
> -               sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
> +               count = sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
> +               if (count != 4) {
> +                       fprintf(stderr, "Could not parse border_color texture parameter.\n");
> +                       piglit_report_result(PIGLIT_FAIL);

This breaks running more than one shader test at a time.
This should be "result = PIGLIT_FAIL;"

> +               }
>                 glTexParameterfv(target, GL_TEXTURE_BORDER_COLOR, bc);
>                 return;
>         } else if (string_match("swizzle_r ", line)) {
> -- 
> 2.7.4
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
On Mon, Nov 7, 2016 at 10:03 AM, Dylan Baker <dylan@pnwbakers.com> wrote:
> Quoting Vinson Lee (2016-11-04 16:30:39)
>> Fix Coverity unchecked return value defect.
>>
>> CID: 1373655
>> Signed-off-by: Vinson Lee <vlee@freedesktop.org>
>> ---
>>  tests/shaders/shader_runner.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
>> index 02a6cd7cda82..62fc8120a30c 100644
>> --- a/tests/shaders/shader_runner.c
>> +++ b/tests/shaders/shader_runner.c
>> @@ -2769,8 +2769,13 @@ handle_texparameter(const char *line)
>>                 return;
>>         } else if (string_match("border_color ", line)) {
>>                 float bc[4];
>> +               int count;
>>                 line += strlen("border_color ");
>> -               sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
>> +               count = sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
>> +               if (count != 4) {
>> +                       fprintf(stderr, "Could not parse border_color texture parameter.\n");
>> +                       piglit_report_result(PIGLIT_FAIL);
>
> This breaks running more than one shader test at a time.
> This should be "result = PIGLIT_FAIL;"
>

There is no result to be returned by handle_texparameter though and
the final else statement also reports PIGLIT_FAIL.

>> +               }
>>                 glTexParameterfv(target, GL_TEXTURE_BORDER_COLOR, bc);
>>                 return;
>>         } else if (string_match("swizzle_r ", line)) {
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Piglit mailing list
>> Piglit@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/piglit
Quoting Vinson Lee (2016-11-07 14:03:41)
> On Mon, Nov 7, 2016 at 10:03 AM, Dylan Baker <dylan@pnwbakers.com> wrote:
> > Quoting Vinson Lee (2016-11-04 16:30:39)
> >> Fix Coverity unchecked return value defect.
> >>
> >> CID: 1373655
> >> Signed-off-by: Vinson Lee <vlee@freedesktop.org>
> >> ---
> >>  tests/shaders/shader_runner.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> >> index 02a6cd7cda82..62fc8120a30c 100644
> >> --- a/tests/shaders/shader_runner.c
> >> +++ b/tests/shaders/shader_runner.c
> >> @@ -2769,8 +2769,13 @@ handle_texparameter(const char *line)
> >>                 return;
> >>         } else if (string_match("border_color ", line)) {
> >>                 float bc[4];
> >> +               int count;
> >>                 line += strlen("border_color ");
> >> -               sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
> >> +               count = sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
> >> +               if (count != 4) {
> >> +                       fprintf(stderr, "Could not parse border_color texture parameter.\n");
> >> +                       piglit_report_result(PIGLIT_FAIL);
> >
> > This breaks running more than one shader test at a time.
> > This should be "result = PIGLIT_FAIL;"
> >
> 
> There is no result to be returned by handle_texparameter though and
> the final else statement also reports PIGLIT_FAIL.
> 
> >> +               }
> >>                 glTexParameterfv(target, GL_TEXTURE_BORDER_COLOR, bc);
> >>                 return;
> >>         } else if (string_match("swizzle_r ", line)) {
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> Piglit mailing list
> >> Piglit@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/piglit

Right, but currently the function doesn't piglit_report_result except with
PIGLIT_SKIP, which has an explicit workaround because it can get raised in low
levels of the framework. This will break running more than one shader test at a
time if it hits the piglit_report_result you're adding.
On Mon, Nov 7, 2016 at 2:29 PM, Dylan Baker <dylan@pnwbakers.com> wrote:
> Quoting Vinson Lee (2016-11-07 14:03:41)
>> On Mon, Nov 7, 2016 at 10:03 AM, Dylan Baker <dylan@pnwbakers.com> wrote:
>> > Quoting Vinson Lee (2016-11-04 16:30:39)
>> >> Fix Coverity unchecked return value defect.
>> >>
>> >> CID: 1373655
>> >> Signed-off-by: Vinson Lee <vlee@freedesktop.org>
>> >> ---
>> >>  tests/shaders/shader_runner.c | 7 ++++++-
>> >>  1 file changed, 6 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
>> >> index 02a6cd7cda82..62fc8120a30c 100644
>> >> --- a/tests/shaders/shader_runner.c
>> >> +++ b/tests/shaders/shader_runner.c
>> >> @@ -2769,8 +2769,13 @@ handle_texparameter(const char *line)
>> >>                 return;
>> >>         } else if (string_match("border_color ", line)) {
>> >>                 float bc[4];
>> >> +               int count;
>> >>                 line += strlen("border_color ");
>> >> -               sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
>> >> +               count = sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
>> >> +               if (count != 4) {
>> >> +                       fprintf(stderr, "Could not parse border_color texture parameter.\n");
>> >> +                       piglit_report_result(PIGLIT_FAIL);
>> >
>> > This breaks running more than one shader test at a time.
>> > This should be "result = PIGLIT_FAIL;"
>> >
>>
>> There is no result to be returned by handle_texparameter though and
>> the final else statement also reports PIGLIT_FAIL.
>>
>> >> +               }
>> >>                 glTexParameterfv(target, GL_TEXTURE_BORDER_COLOR, bc);
>> >>                 return;
>> >>         } else if (string_match("swizzle_r ", line)) {
>> >> --
>> >> 2.7.4
>> >>
>> >> _______________________________________________
>> >> Piglit mailing list
>> >> Piglit@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/piglit
>
> Right, but currently the function doesn't piglit_report_result except with
> PIGLIT_SKIP, which has an explicit workaround because it can get raised in low
> levels of the framework. This will break running more than one shader test at a
> time if it hits the piglit_report_result you're adding.

PIGLIT_FAIL is reported several lines down in shader_runner.c:2783.
Quoting Vinson Lee (2016-11-07 14:34:23)
> On Mon, Nov 7, 2016 at 2:29 PM, Dylan Baker <dylan@pnwbakers.com> wrote:
> > Quoting Vinson Lee (2016-11-07 14:03:41)
> >> On Mon, Nov 7, 2016 at 10:03 AM, Dylan Baker <dylan@pnwbakers.com> wrote:
> >> > Quoting Vinson Lee (2016-11-04 16:30:39)
> >> >> Fix Coverity unchecked return value defect.
> >> >>
> >> >> CID: 1373655
> >> >> Signed-off-by: Vinson Lee <vlee@freedesktop.org>
> >> >> ---
> >> >>  tests/shaders/shader_runner.c | 7 ++++++-
> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> >> >> index 02a6cd7cda82..62fc8120a30c 100644
> >> >> --- a/tests/shaders/shader_runner.c
> >> >> +++ b/tests/shaders/shader_runner.c
> >> >> @@ -2769,8 +2769,13 @@ handle_texparameter(const char *line)
> >> >>                 return;
> >> >>         } else if (string_match("border_color ", line)) {
> >> >>                 float bc[4];
> >> >> +               int count;
> >> >>                 line += strlen("border_color ");
> >> >> -               sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
> >> >> +               count = sscanf(line, "%f %f %f %f", &bc[0], &bc[1], &bc[2], &bc[3]);
> >> >> +               if (count != 4) {
> >> >> +                       fprintf(stderr, "Could not parse border_color texture parameter.\n");
> >> >> +                       piglit_report_result(PIGLIT_FAIL);
> >> >
> >> > This breaks running more than one shader test at a time.
> >> > This should be "result = PIGLIT_FAIL;"
> >> >
> >>
> >> There is no result to be returned by handle_texparameter though and
> >> the final else statement also reports PIGLIT_FAIL.
> >>
> >> >> +               }
> >> >>                 glTexParameterfv(target, GL_TEXTURE_BORDER_COLOR, bc);
> >> >>                 return;
> >> >>         } else if (string_match("swizzle_r ", line)) {
> >> >> --
> >> >> 2.7.4
> >> >>
> >> >> _______________________________________________
> >> >> Piglit mailing list
> >> >> Piglit@lists.freedesktop.org
> >> >> https://lists.freedesktop.org/mailman/listinfo/piglit
> >
> > Right, but currently the function doesn't piglit_report_result except with
> > PIGLIT_SKIP, which has an explicit workaround because it can get raised in low
> > levels of the framework. This will break running more than one shader test at a
> > time if it hits the piglit_report_result you're adding.
> 
> PIGLIT_FAIL is reported several lines down in shader_runner.c:2783.

Ah, that's another bug. Both Marek and I must have missed that. Grepping for
piglit_report_result there are a few other places it's being used in
shader_runner.c it shouldn't. A lot of them actually.

I'll retract my comments. I think there's a bigger problem than an error
checking case.

Dylan