On Wed, 1 Aug 2007, Segher Boessenkool wrote:
> > > > > > + strncpy(info->driver_name, i2c_devices[i].i2c_driver,
> > > > > > KOBJ_NAME_LEN);
> > > > > > + strncpy(info->type, i2c_devices[i].i2c_type,
> > > > > > I2C_NAME_SIZE);
> > > > >
> > > > > Why not just strcpy(), btw?
> > > >
> > > > Because target strings are finite length, and sources are just pointers
> > > > to
> > > > some constant strings, which one might make arbitrarily long.
> > >
> > > So it's no problem if the name or type string gets cut short?
> > > Just checking :-)
> >
> > Then it just won't match.
>
> strncpy() won't put a terminating zero on there, is everything
> that uses the resulting string okay with that?
Ok, this might be a problem, I guess. E.g., in
drivers/i2c/i2c-core.c::i2c_device_uevent():
if (add_uevent_var(envp, num_envp, &i, buffer, buffer_size, &length,
"MODALIAS=%s", client->driver_name))
and in a couple more places.
> Also, if the
> name gets cut short, it might match some _other_ expected name.
Don't think this could happen. At least the generic strcmp compares the
'\0' too.
Now, the bad news (for me at least, as well as for some on #mklinux). To
fix the bug I wrote the patch below. Yes, it is lacking a "Signed-..." -
on purpose. Here's why. Having written it, I verified, that
c[4] = "01234";
causes a nice compiler warning like:
warning: initializer-string for array of chars is too long
Then I asked myself what happens if you do
c[4] = "0123";
? To my surprise, there was no warning. So, for example, declaring a
struct like
struct t {
char c[4];
int i;
} z;
and doing
strcpy(z.c, c);
silently corrupts z.i. Verified with gcc 3.3.x, 3.4.5, 4.1.2. 4.2 Does
emit a warning... Now, am I right that there are lots of places in
the kernel where we just initialize arrays of chars like above and then
use ctrcpy to copy it to another equally-long string? Which means, if the
initialization string is exactly the array length - without the '\0' - we
get random memory corruption... And the only safe way is
strncpy(z.c, c, 3);
z.c[3] = '\0';
with compilers < 4.2?...
Thanks
Guennadi
---
Fix a bug introduced by my earlier patch, whereby strncpy of too long a
line could leave the result unterminated. Instead, just use fixed-size
arrays and let the compiler verify the length for us. No need to try to
save a few bytes of __initdata anyway. Thanks to Segher Boessenkool for
pointing out.
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 727453d..1e567d5 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -309,8 +309,8 @@ arch_initcall(gfar_of_init);
#include <linux/i2c.h>
struct i2c_driver_device {
char *of_device;
- char *i2c_driver;
- char *i2c_type;
+ char i2c_driver[KOBJ_NAME_LEN];
+ char i2c_type[I2C_NAME_SIZE];
};
static struct i2c_driver_device i2c_devices[] __initdata = {
@@ -327,8 +327,8 @@ static int __init of_find_i2c_driver(struct device_node
*node, struct i2c_board_
for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
if (!of_device_is_compatible(node, i2c_devices[i].of_device))
continue;
- strncpy(info->driver_name, i2c_devices[i].i2c_driver,
KOBJ_NAME_LEN);
- strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+ strcpy(info->driver_name, i2c_devices[i].i2c_driver);
+ strcpy(info->type, i2c_devices[i].i2c_type);
return 0;
}
return -ENODEV;
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
