powerpc/cell: strncpy does not null terminate string

PATCH In review Roel Kluin 5 participants 14 messages Jul 17, 2009 – Jul 22, 2009

Subsystems (6)

Networking Network drivers Media (V4L/DVB) PowerPC +2 more subsystems

lore

Share & tools

lore

Roel Kluin

author

2009-07-17 12:39 UTC #0 lore raw

PATCH 1/1 powerpc/cell: strncpy does not null terminate string

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

Signed-off-by Roel Kluin <[email protected]>

---
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

arch/powerpc/platforms/cell/celleb_setup.c +1 -1

index 07c234f..cfdbadb 100644
@@ -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;
 }

Roel Kluin

author

2009-07-17 14:33 UTC #1 lore raw

strlcpy() will always null terminate the string.

Signed-off-by Roel Kluin <[email protected]>

---
Please use this one instead

arch/powerpc/platforms/cell/celleb_setup.c +1 -1

index 07c234f..1896cd8 100644
@@ -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;
 }

Arnd Bergmann

2009-07-17 15:56 UTC #2 lore raw

arch/powerpc/platforms/cell/celleb_setup.c static void celleb_show_cpuinfo(struct seq_file *m) · line 85

Quoted 7 lines · Roel Kluin

static int __init celleb_machine_type_hack(char *ptr) {

arch/powerpc/platforms/cell/celleb_setup.c +1 -1

-       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 <><

Ken Kawakami

2009-07-21 09:31 UTC #3 lore raw

Arnd-san, Roel-san,

Thanks for pointing us to the redundant cord portion.

Quoted 9 lines · Roel Kluin

arch/powerpc/platforms/cell/celleb_setup.c static void celleb_show_cpuinfo(struct seq_file *m) · line 85
Quoted 7 lines
static int __init celleb_machine_type_hack(char *ptr) {
arch/powerpc/platforms/cell/celleb_setup.c +1 -1
-       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;

Quoted 7 lines

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

Arnd Bergmann

2009-07-17 15:05 UTC #4 lore raw

In reply to Roel Kluin

Quoted 1 line · Roel Kluin

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

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

Quoted 20 lines

Signed-off-by Roel Kluin <[email protected]>
---
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;
}

Quoted 2 lines

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.

arch/powerpc/platforms/cell/celleb_setup.c static void celleb_show_cpuinfo(struct seq_file *m) · line 86

Quoted 13 lines

arch/powerpc/platforms/cell/celleb_setup.c +1 -1

index 07c234f..cfdbadb 100644
@@ -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 <><

roel kluin

author

2009-07-17 15:19 UTC #5 lore raw

Quoted 5 lines

Quoted 1 line
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

Roel Kluin

author

2009-07-17 15:35 UTC #6 lore raw

PATCH 1/1 dm: strncpy does not null terminate string

In reply to Roel Kluin

strlcpy() will always null terminate the string.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/md/dm-ioctl.c +2 -2

index 7f77f18..8208b3a 100644
@@ -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';

Alasdair G Kergon

maintainer

2009-07-22 12:29 UTC #7 lore raw

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

Quoted 1 line · Roel Kluin

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

Roel Kluin

author

2009-07-17 15:39 UTC #8 lore raw

PATCH 1/1 media: strncpy does not null terminate string

In reply to Roel Kluin

strlcpy() will always null terminate the string.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/media/dvb/dvb-usb/dvb-usb-i2c.c +1 -1

index 326f760..cead089 100644
@@ -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;

drivers/media/video/pwc/pwc-v4l.c +1 -1

index 2876ce0..bdb4ced 100644
@@ -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;
 		}

drivers/media/video/zoran/zoran_card.c +1 -1

index 03dc2f3..9f43695 100644
@@ -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)

Roel Kluin

author

2009-07-17 15:52 UTC #9 lore raw

PATCH 1/1 media: strncpy does not null terminate string

In reply to Roel Kluin

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

Signed-off-by Roel Kluin <[email protected]>

---

drivers/net/b44.c +4 -4

index 36d4d37..1f7f015 100644
@@ -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;
 	}
 }

Roel Kluin

author

2009-07-17 18:00 UTC #10 lore raw

PATCH 1/1 b44: strncpy does not null terminate string

In reply to Roel Kluin

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

Signed-off-by Roel Kluin <[email protected]>

---

drivers/net/b44.c +4 -4

index 36d4d37..1f7f015 100644
@@ -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;
 	}
 }

David Miller

maintainer Applied

2009-07-20 15:03 UTC #11 lore raw

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

From: Roel Kluin <[email protected]>
Date: Fri, 17 Jul 2009 20:01:54 +0200

Quoted 4 lines

strlcpy() will always null terminate the string. Also use the sizeof(version) to strlcopy() the version string.
Signed-off-by Roel Kluin <[email protected]>

Applied to net-next-2.6

Roel Kluin

author

2009-07-21 10:15 UTC #12 lore raw

PATCH 1/1 powerpc/cell: replace strncpy by strlcpy

In reply to Roel Kluin

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

Signed-off-by Roel Kluin <[email protected]>

---
Arnd-san, Ken-san,

Thanks for reviewing,

Quoted 1 line

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

Doozo.

arch/powerpc/platforms/cell/celleb_setup.c +1 -2

index 07c234f..e538455 100644
@@ -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;
 }

Ken Kawakami

2009-07-22 04:22 UTC #13 lore raw

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

Arnd-san, Roel-san,

It works fine. Thanks.

---
Regards,
Ken Kawakami

Quoted 27 lines

Replace strncpy() and explicit null-termination by strlcpy()
Signed-off-by Roel Kluin <[email protected]>
---
Arnd-san, Ken-san,
Thanks for reviewing,
Quoted 1 line
We prefer to take the patch which is replacing the two lines with one.
Doozo.
arch/powerpc/platforms/cell/celleb_setup.c +1 -2
index 07c234f..e538455 100644
@@ -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;
 }
 