[PATCH] powerpc/cell: strncpy does not null terminate string

[PATCH] powerpc/cell: strncpy does not null terminate string

[Roel Kluin]

With `sizeof(string) - 1` strncpy() will null terminate the string.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
To test this:

#include <stdio.h>
#include <string.h>

char a[10];
char b[10];

int main()
{
        const char* str = "0123456789012";
        strncpy(a, str, sizeof(a));
        strncpy(b, str, sizeof(b) - 1);
        printf("String a was %s, b was %s\n", a, b);

        return 0;
}

Output:
String a was 0123456789012345678, b was 012345678

diff --git a/arch/powerpc/platforms/cell/celleb_setup.c b/arch/powerpc/platforms/cell/celleb_setup.c
index 07c234f..cfdbadb 100644
--- a/arch/powerpc/platforms/cell/celleb_setup.c
+++ b/arch/powerpc/platforms/cell/celleb_setup.c
@@ -80,7 +80,7 @@ static void celleb_show_cpuinfo(struct seq_file *m)
 
 static int __init celleb_machine_type_hack(char *ptr)
 {
-	strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
+	strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type) - 1);
 	celleb_machine_type[sizeof(celleb_machine_type)-1] = 0;
 	return 0;
 }

Re: [PATCH] powerpc/cell: strncpy does not null terminate string

[Roel Kluin]

strlcpy() will always null terminate the string.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Please use this one instead

diff --git a/arch/powerpc/platforms/cell/celleb_setup.c b/arch/powerpc/platforms/cell/celleb_setup.c
index 07c234f..1896cd8 100644
--- a/arch/powerpc/platforms/cell/celleb_setup.c
+++ b/arch/powerpc/platforms/cell/celleb_setup.c
@@ -80,7 +80,7 @@ static void celleb_show_cpuinfo(struct seq_file *m)
 
 static int __init celleb_machine_type_hack(char *ptr)
 {
-	strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
+	strlcpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
 	celleb_machine_type[sizeof(celleb_machine_type)-1] = 0;
 	return 0;
 }

Re: [PATCH] powerpc/cell: strncpy does not null terminate string

[Arnd Bergmann]

On Friday 17 July 2009, Roel Kluin wrote:
> With `sizeof(string) - 1` strncpy() will null terminate the string.

No, it won't. See the 'Warning' part of the strncpy man page.

> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> To test this:
> 
> #include <stdio.h>
> #include <string.h>
> 
> char a[10];
> char b[10];
> 
> int main()
> {
>         const char* str = "0123456789012";
>         strncpy(a, str, sizeof(a));
>         strncpy(b, str, sizeof(b) - 1);
>         printf("String a was %s, b was %s\n", a, b);
> 
>         return 0;
> }
> 
> Output:
> String a was 0123456789012345678, b was 012345678

This is an invalid test case, it relies on b being zero-filled by the
compiler, which is not true for programs in general.

> diff --git a/arch/powerpc/platforms/cell/celleb_setup.c b/arch/powerpc/platforms/cell/celleb_setup.c
> index 07c234f..cfdbadb 100644
> --- a/arch/powerpc/platforms/cell/celleb_setup.c
> +++ b/arch/powerpc/platforms/cell/celleb_setup.c
> @@ -80,7 +80,7 @@ static void celleb_show_cpuinfo(struct seq_file *m)
>  
>  static int __init celleb_machine_type_hack(char *ptr)
>  {
> -	strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
> +	strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type) - 1);
>  	celleb_machine_type[sizeof(celleb_machine_type)-1] = 0;
>  	return 0;
>  }

See the line after the strncpy. This is still required for proper zero-termination.

Your patch tries to address a problem that doesn't exist, and does not have any
effect at all after celleb_machine_type_hack has completed.

	Arnd <><

Re: [PATCH] powerpc/cell: strncpy does not null terminate string

[roel kluin]

>> With `sizeof(string) - 1` strncpy() will null terminate the string.
>
> No, it won't.

> See the line after the strncpy. This is still required for proper zero-termination.

You're right, sorry for the noise.

Roel

[PATCH] dm: strncpy does not null terminate string

[Roel Kluin]

strlcpy() will always null terminate the string.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 7f77f18..8208b3a 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -634,9 +634,9 @@ static struct mapped_device *find_device(struct dm_ioctl *param)
 		 * Sneakily write in both the name and the uuid
 		 * while we have the cell.
 		 */
-		strncpy(param->name, hc->name, sizeof(param->name));
+		strlcpy(param->name, hc->name, sizeof(param->name));
 		if (hc->uuid)
-			strncpy(param->uuid, hc->uuid, sizeof(param->uuid)-1);
+			strlcpy(param->uuid, hc->uuid, sizeof(param->uuid));
 		else
 			param->uuid[0] = '\0';
 

[PATCH] media: strncpy does not null terminate string

[Roel Kluin]

strlcpy() will always null terminate the string.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-i2c.c b/drivers/media/dvb/dvb-usb/dvb-usb-i2c.c
index 326f760..cead089 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-i2c.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-i2c.c
@@ -19,7 +19,7 @@ int dvb_usb_i2c_init(struct dvb_usb_device *d)
 		return -EINVAL;
 	}
 
-	strncpy(d->i2c_adap.name, d->desc->name, sizeof(d->i2c_adap.name));
+	strlcpy(d->i2c_adap.name, d->desc->name, sizeof(d->i2c_adap.name));
 	d->i2c_adap.class = I2C_CLASS_TV_DIGITAL,
 	d->i2c_adap.algo      = d->props.i2c_algo;
 	d->i2c_adap.algo_data = NULL;
diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 2876ce0..bdb4ced 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -1033,7 +1033,7 @@ long pwc_video_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 			if (std->index != 0)
 				return -EINVAL;
 			std->id = V4L2_STD_UNKNOWN;
-			strncpy(std->name, "webcam", sizeof(std->name));
+			strlcpy(std->name, "webcam", sizeof(std->name));
 			return 0;
 		}
 
diff --git a/drivers/media/video/zoran/zoran_card.c b/drivers/media/video/zoran/zoran_card.c
index 03dc2f3..9f43695 100644
--- a/drivers/media/video/zoran/zoran_card.c
+++ b/drivers/media/video/zoran/zoran_card.c
@@ -1169,7 +1169,7 @@ zoran_setup_videocodec (struct zoran *zr,
 	m->type = 0;
 
 	m->flags = CODEC_FLAG_ENCODER | CODEC_FLAG_DECODER;
-	strncpy(m->name, ZR_DEVNAME(zr), sizeof(m->name));
+	strlcpy(m->name, ZR_DEVNAME(zr), sizeof(m->name));
 	m->data = zr;
 
 	switch (type)

[PATCH] media: strncpy does not null terminate string

[Roel Kluin]

strlcpy() will always null terminate the string. Also use the
sizeof(version) to strlcopy() the version string.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/net/b44.c b/drivers/net/b44.c
index 36d4d37..1f7f015 100644
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -1756,15 +1756,15 @@ static void b44_get_drvinfo (struct net_device *dev, struct ethtool_drvinfo *inf
 	struct b44 *bp = netdev_priv(dev);
 	struct ssb_bus *bus = bp->sdev->bus;
 
-	strncpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
-	strncpy(info->version, DRV_MODULE_VERSION, sizeof(info->driver));
+	strlcpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_MODULE_VERSION, sizeof(info->version));
 	switch (bus->bustype) {
 	case SSB_BUSTYPE_PCI:
-		strncpy(info->bus_info, pci_name(bus->host_pci), sizeof(info->bus_info));
+		strlcpy(info->bus_info, pci_name(bus->host_pci), sizeof(info->bus_info));
 		break;
 	case SSB_BUSTYPE_PCMCIA:
 	case SSB_BUSTYPE_SSB:
-		strncpy(info->bus_info, "SSB", sizeof(info->bus_info));
+		strlcpy(info->bus_info, "SSB", sizeof(info->bus_info));
 		break;
 	}
 }

Re: [PATCH] powerpc/cell: strncpy does not null terminate string

[Arnd Bergmann]

On Friday 17 July 2009, Roel Kluin wrote:
> 
>  static int __init celleb_machine_type_hack(char *ptr)
>  {
> -       strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
> +       strlcpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
>         celleb_machine_type[sizeof(celleb_machine_type)-1] = 0;
>         return 0;

This still is pointless as long as you keep the explicit null-termination
in the next line, the patch still doesn't change anything significant.

The file is maintained by Ishizaki Kou, if he would prefer to take a
patch replacing the two lines with one, that's fine with me, otherwise
I just wouldn't bother. You still only gain a few bytes of inittext, but
that is discarded at boot time.

	Arnd <><

[PATCH] b44: strncpy does not null terminate string

[Roel Kluin]

strlcpy() will always null terminate the string. Also use the
sizeof(version) to strlcopy() the version string.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/net/b44.c b/drivers/net/b44.c
index 36d4d37..1f7f015 100644
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -1756,15 +1756,15 @@ static void b44_get_drvinfo (struct net_device *dev, struct ethtool_drvinfo *inf
 	struct b44 *bp = netdev_priv(dev);
 	struct ssb_bus *bus = bp->sdev->bus;
 
-	strncpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
-	strncpy(info->version, DRV_MODULE_VERSION, sizeof(info->driver));
+	strlcpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_MODULE_VERSION, sizeof(info->version));
 	switch (bus->bustype) {
 	case SSB_BUSTYPE_PCI:
-		strncpy(info->bus_info, pci_name(bus->host_pci), sizeof(info->bus_info));
+		strlcpy(info->bus_info, pci_name(bus->host_pci), sizeof(info->bus_info));
 		break;
 	case SSB_BUSTYPE_PCMCIA:
 	case SSB_BUSTYPE_SSB:
-		strncpy(info->bus_info, "SSB", sizeof(info->bus_info));
+		strlcpy(info->bus_info, "SSB", sizeof(info->bus_info));
 		break;
 	}
 }

Re: [PATCH] b44: strncpy does not null terminate string

[David Miller]

From: Roel Kluin <roel.kluin@gmail.com>
Date: Fri, 17 Jul 2009 20:01:54 +0200

> strlcpy() will always null terminate the string. Also use the
> sizeof(version) to strlcopy() the version string.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>

Applied to net-next-2.6

Re: [PATCH] powerpc/cell: strncpy does not null terminate string

[Ken Kawakami]

Arnd-san, Roel-san,

Thanks for pointing us to the redundant cord portion.

> On Friday 17 July 2009, Roel Kluin wrote:
> > 
> >  static int __init celleb_machine_type_hack(char *ptr)
> >  {
> > -       strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
> > +       strlcpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
> >         celleb_machine_type[sizeof(celleb_machine_type)-1] = 0;
> >         return 0;
> 
> This still is pointless as long as you keep the explicit null-termination
> in the next line, the patch still doesn't change anything significant.
> 
> The file is maintained by Ishizaki Kou, if he would prefer to take a
> patch replacing the two lines with one, that's fine with me, otherwise
> I just wouldn't bother. You still only gain a few bytes of inittext, but
> that is discarded at boot time.

We prefer to take the patch which is replacing the two lines with one.
-       strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
+       strlcpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
-       celleb_machine_type[sizeof(celleb_machine_type)-1] = 0;

Thanks,
Ken Kawakami

[PATCH] powerpc/cell: replace strncpy by strlcpy

[Roel Kluin]

Replace strncpy() and explicit null-termination by strlcpy()

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Arnd-san, Ken-san,

Thanks for reviewing,

> We prefer to take the patch which is replacing the two lines with one.

Doozo.

diff --git a/arch/powerpc/platforms/cell/celleb_setup.c b/arch/powerpc/platforms/cell/celleb_setup.c
index 07c234f..e538455 100644
--- a/arch/powerpc/platforms/cell/celleb_setup.c
+++ b/arch/powerpc/platforms/cell/celleb_setup.c
@@ -80,8 +80,7 @@ static void celleb_show_cpuinfo(struct seq_file *m)
 
 static int __init celleb_machine_type_hack(char *ptr)
 {
-	strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
-	celleb_machine_type[sizeof(celleb_machine_type)-1] = 0;
+	strlcpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
 	return 0;
 }
 

Re: [PATCH] powerpc/cell: replace strncpy by strlcpy

[Ken Kawakami]

Arnd-san, Roel-san,

It works fine. Thanks.

---
Regards,
Ken Kawakami

> Replace strncpy() and explicit null-termination by strlcpy()
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> Arnd-san, Ken-san,
> 
> Thanks for reviewing,
> 
> > We prefer to take the patch which is replacing the two lines with one.
> 
> Doozo.
> 
> diff --git a/arch/powerpc/platforms/cell/celleb_setup.c b/arch/powerpc/platforms/cell/celleb_setup.c
> index 07c234f..e538455 100644
> --- a/arch/powerpc/platforms/cell/celleb_setup.c
> +++ b/arch/powerpc/platforms/cell/celleb_setup.c
> @@ -80,8 +80,7 @@ static void celleb_show_cpuinfo(struct seq_file *m)
>  
>  static int __init celleb_machine_type_hack(char *ptr)
>  {
> -	strncpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
> -	celleb_machine_type[sizeof(celleb_machine_type)-1] = 0;
> +	strlcpy(celleb_machine_type, ptr, sizeof(celleb_machine_type));
>  	return 0;
>  }
>  
> 

Re: [PATCH] dm: strncpy does not null terminate string

[Alasdair G Kergon]

On Fri, Jul 17, 2009 at 05:35:59PM +0200, Roel Kluin wrote:
> strlcpy() will always null terminate the string.
 
The code is already supposed to ensure correct NULL-termination.

Did you find a bug?  If so, please give details of the code path 
concerned in the patch header and I'll need to deal with it promptly
and submit it to stable too.

Or are you simply proposing this as a precaution against future bugs,
in which case it is not urgent?

Alasdair