Re: [PATCH]: Cleanup: Remove gcc format string warnings when compiling with -Wformat-security

[Floris Kraak]

On Thu, Feb 5, 2009 at 11:13 PM, Roland Dreier <rdreier@xxxxxxxxx> wrote:
>
> I would strongly prefer to do this with a little more care.  For example
> the b43/main.c change:
>
>  > --- a/drivers/net/wireless/b43/main.c
>  > +++ b/drivers/net/wireless/b43/main.c
>  > @@ -2005,9 +2005,9 @@ static void b43_print_fw_helptext(struct b43_wl
>  > *wl, bool error)
>  >             "http://linuxwireless.org/en/users/Drivers/b43#devicefirmware "
>  >             "and download the latest firmware (version 4).\n";
>  >      if (error)
>  > -            b43err(wl, text);
>  > +            b43err(wl, "%s", text);
>  >      else
>  > -            b43warn(wl, text);
>  > +            b43warn(wl, "%s", text);
>  >  }
> would probably be better solved by doing
>
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -1999,11 +1999,11 @@ static void b43_release_firmware(struct b43_wldev *dev)
>
>  static void b43_print_fw_helptext(struct b43_wl *wl, bool error)
>  {
> -       const char *text;
> +       static const char text[] =
> +               "You must go to "
> +               "http://linuxwireless.org/en/users/Drivers/b43#devicefirmware "
> +               "and download the latest firmware (version 4).\n";
>
> -       text = "You must go to "
> -              "http://linuxwireless.org/en/users/Drivers/b43#devicefirmware "
> -              "and download the latest firmware (version 4).\n";
>        if (error)
>                b43err(wl, text);
>        else
>

Fair enough. I'm going to have to break this down into categories of
bugs probably anyway. - "trivial just makes the compiler stops
whining" vs "omg security hole" and the shades in between. From what
I've seen so far the vast majority of this falls into the first
category though ;-)

> and in any case I'm not totally convinced that we want to add the bloat
> for trivial cases like
>
>        char *safe = "foo";
>        printk(safe);
>
> Would be nice to think of a cleverer way to handle that...
>

It would help if GCC was a little smarter in the detection, right now
it gives a lot of false positives even on code like that.
When I was playing around with coccinelle tonight in a (probably
misguided) attempt to automate this I found a few new possible cases
where macro wrappers around printk() seem to make avoiding this check
possible. Haven't looked deeply yet, I'm doing this on my spare time
which I don't have tons of ;-)

Regards,
Floris
---
"They that give up essential liberty to obtain temporary safety,
deserve neither liberty nor safety."
  -- Ben Franklin

"The course of history shows that as a government grows, liberty
decreases."
  -- Thomas Jefferson
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/