[PATCH] scripts: recordmcount: fix incorrect use of sprintf

[PATCH] scripts: recordmcount: fix incorrect use of sprintf

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Fix build warning:

scripts/recordmcount.c:589:4: warning: format not a string
literal and no format arguments [-Wformat-security]
    sprintf("%s: failed\n", file);

Fixes: a50bd43935586 ("ftrace/scripts: Have recordmcount copy the object file")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 scripts/recordmcount.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 301d70b..e1675927 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -586,7 +586,7 @@ main(int argc, char *argv[])
 			do_file(file);
 			break;
 		case SJ_FAIL:    /* error in do_file or below */
-			sprintf("%s: failed\n", file);
+			fprintf(stderr, "%s: failed\n", file);
 			++n_error;
 			break;
 		case SJ_SUCCEED:    /* premature success */
-- 
2.6.4

Re: [PATCH] scripts: recordmcount: fix incorrect use of sprintf

[Steven Rostedt]

On Wed, 30 Dec 2015 23:06:41 +0000
Colin King <colin.king@canonical.com> wrote:

> From: Colin Ian King <colin.king@canonical.com>
> 
> Fix build warning:
> 
> scripts/recordmcount.c:589:4: warning: format not a string
> literal and no format arguments [-Wformat-security]
>     sprintf("%s: failed\n", file);
> 
> Fixes: a50bd43935586 ("ftrace/scripts: Have recordmcount copy the object file")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  scripts/recordmcount.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
> index 301d70b..e1675927 100644
> --- a/scripts/recordmcount.c
> +++ b/scripts/recordmcount.c
> @@ -586,7 +586,7 @@ main(int argc, char *argv[])
>  			do_file(file);
>  			break;
>  		case SJ_FAIL:    /* error in do_file or below */
> -			sprintf("%s: failed\n", file);
> +			fprintf(stderr, "%s: failed\n", file);

Paper bag bug. I'm not sure how this passed my tests? My tests check
for warnings. And I even got a "BUILD SUCCESS" from Fengguang Wu's
kbuild test robot.

I'll apply it, test it, and push it out soon.

Thanks!

-- Steve


>  			++n_error;
>  			break;
>  		case SJ_SUCCEED:    /* premature success */

Re: [PATCH] scripts: recordmcount: fix incorrect use of sprintf

[Fengguang Wu]

Hi Steven,

On Mon, Jan 04, 2016 at 10:42:46AM -0500, Steven Rostedt wrote:
> On Wed, 30 Dec 2015 23:06:41 +0000
> Colin King <colin.king@canonical.com> wrote:
> 
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Fix build warning:
> > 
> > scripts/recordmcount.c:589:4: warning: format not a string
> > literal and no format arguments [-Wformat-security]
> >     sprintf("%s: failed\n", file);
> > 
> > Fixes: a50bd43935586 ("ftrace/scripts: Have recordmcount copy the object file")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  scripts/recordmcount.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
> > index 301d70b..e1675927 100644
> > --- a/scripts/recordmcount.c
> > +++ b/scripts/recordmcount.c
> > @@ -586,7 +586,7 @@ main(int argc, char *argv[])
> >  			do_file(file);
> >  			break;
> >  		case SJ_FAIL:    /* error in do_file or below */
> > -			sprintf("%s: failed\n", file);
> > +			fprintf(stderr, "%s: failed\n", file);
> 
> Paper bag bug. I'm not sure how this passed my tests? My tests check
> for warnings. And I even got a "BUILD SUCCESS" from Fengguang Wu's
> kbuild test robot.

Because the error will only show up on "gcc -Wformat-security".

It tend to raise false positives, so Kees setup a dedicated tree
which enables -Wformat-security as well as quieting the common false
positives. In that way Kees can catch such problems from time to time,
however the limitation is, only upstreamed code can be tested in Kees'
tree.

> I'll apply it, test it, and push it out soon.

Thanks,
Fengguang

Re: [PATCH] scripts: recordmcount: fix incorrect use of sprintf

[Kees Cook]

On Wed, Jan 6, 2016 at 3:28 AM, Fengguang Wu <fengguang.wu@intel.com> wrote:
> Hi Steven,
>
> On Mon, Jan 04, 2016 at 10:42:46AM -0500, Steven Rostedt wrote:
>> On Wed, 30 Dec 2015 23:06:41 +0000
>> Colin King <colin.king@canonical.com> wrote:
>>
>> > From: Colin Ian King <colin.king@canonical.com>
>> >
>> > Fix build warning:
>> >
>> > scripts/recordmcount.c:589:4: warning: format not a string
>> > literal and no format arguments [-Wformat-security]
>> >     sprintf("%s: failed\n", file);
>> >
>> > Fixes: a50bd43935586 ("ftrace/scripts: Have recordmcount copy the object file")
>> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> > ---
>> >  scripts/recordmcount.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
>> > index 301d70b..e1675927 100644
>> > --- a/scripts/recordmcount.c
>> > +++ b/scripts/recordmcount.c
>> > @@ -586,7 +586,7 @@ main(int argc, char *argv[])
>> >                     do_file(file);
>> >                     break;
>> >             case SJ_FAIL:    /* error in do_file or below */
>> > -                   sprintf("%s: failed\n", file);
>> > +                   fprintf(stderr, "%s: failed\n", file);
>>
>> Paper bag bug. I'm not sure how this passed my tests? My tests check
>> for warnings. And I even got a "BUILD SUCCESS" from Fengguang Wu's
>> kbuild test robot.
>
> Because the error will only show up on "gcc -Wformat-security".

I wish GCC were smarter about this. I would have hoped at least ONE of
the various -Wformat... options would warn about "you did not actually
specify a buffer for sprintf". But yeah, this comes from
-Wformat-security (a subset of -Wformat-nonliteral), but can't be
turned on by default, as mentioned by Fengguang.

> It tend to raise false positives, so Kees setup a dedicated tree
> which enables -Wformat-security as well as quieting the common false
> positives. In that way Kees can catch such problems from time to time,
> however the limitation is, only upstreamed code can be tested in Kees'
> tree.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security