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

[Floris Kraak]

On Fri, Feb 6, 2009 at 12:42 AM, Robert Hancock <hancockrwd@xxxxxxxxx> wrote:
> Roland Dreier wrote:
>>
>>  > Here's the patch that I get when I blindly patch every single location
>>  > that emits this warning.
>>
>> 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
>

Applied this to my tree.

> "const char* const" text would likely work as well..
>>
>> 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);
>
> Well, in that case, printk("foo") would be the obvious solution :-)
>

Neither of that works for __initdata sections though.
Take for instance the patch below. Yes, it gets rid of the warning
without using %s .. but we lose the __initdata attribute.
I've tried using __initconst instead - but that doesn't remove the
warning either.

Signed-off-by: Floris Kraak <randakar@xxxxxxxxx>
---
diff --git a/drivers/char/hw_random/intel-rng.c
b/drivers/char/hw_random/intel-rng.c
index 5dcbe60..aa84183 100644
--- a/drivers/char/hw_random/intel-rng.c
+++ b/drivers/char/hw_random/intel-rng.c
@@ -304,15 +304,15 @@ static int __init intel_init_hw_struct(struct
intel_rng_hw *intel_rng_hw,
 	if ((intel_rng_hw->bios_cntl_val &
 	     (BIOS_CNTL_LOCK_ENABLE_MASK|BIOS_CNTL_WRITE_ENABLE_MASK))
 	    == BIOS_CNTL_LOCK_ENABLE_MASK) {
-		static __initdata /*const*/ char warning[] =
+
+		if (no_fwh_detect)
+			return -ENODEV;
+		printk(
 			KERN_WARNING PFX "Firmware space is locked read-only. If you can't or\n"
 			KERN_WARNING PFX "don't want to disable this in firmware setup, and if\n"
 			KERN_WARNING PFX "you are certain that your system has a functional\n"
-			KERN_WARNING PFX "RNG, try using the 'no_fwh_detect' option.\n";
+			KERN_WARNING PFX "RNG, try using the 'no_fwh_detect' option.\n");

-		if (no_fwh_detect)
-			return -ENODEV;
-		printk(warning);
 		return -EBUSY;
 	}
---
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/