* [PATCH] Don't call sprintf() with overlapping input and output.
@ 2008-12-07 11:31 Anders Kaseorg
2008-12-07 12:04 ` Tom Spink
2008-12-07 22:31 ` [PATCH] Don't call snprintf() " Anders Kaseorg
0 siblings, 2 replies; 3+ messages in thread
From: Anders Kaseorg @ 2008-12-07 11:31 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel
The use of sprintf() to append to a buffer, as in
sprintf(buf, "%sEntry: %d\n", buf, i)
is not valid according to C99 ("If copying takes place between objects
that overlap, the behavior is undefined."). It breaks at least in
userspace under gcc -D_FORTIFY_SOURCE. Replace this construct with
sprintf(buf + strlen(buf), "Entry: %d\n", i)
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
Documentation/ia64/err_inject.txt | 2 +-
arch/ia64/sn/kernel/io_common.c | 2 +-
arch/s390/kernel/early.c | 6 +++---
arch/x86/kernel/cpu/intel_cacheinfo.c | 9 ++++-----
drivers/media/video/se401.c | 2 +-
drivers/scsi/gdth_proc.c | 2 +-
drivers/usb/atm/usbatm.c | 2 +-
7 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/Documentation/ia64/err_inject.txt b/Documentation/ia64/err_inject.txt
index 223e4f0..62d0028 100644
--- a/Documentation/ia64/err_inject.txt
+++ b/Documentation/ia64/err_inject.txt
@@ -376,7 +376,7 @@ int create_sem(int cpu)
int sid;
sprintf(fn, PATH_FORMAT, cpu);
- sprintf(fn, "%s/%s", fn, "err_type_info");
+ sprintf(fn + strlen(fn), "/%s", "err_type_info");
if ((key[cpu] = ftok(fn, 'e')) == -1) {
perror("ftok");
return -1;
diff --git a/arch/ia64/sn/kernel/io_common.c b/arch/ia64/sn/kernel/io_common.c
index 8a924a5..656e201 100644
--- a/arch/ia64/sn/kernel/io_common.c
+++ b/arch/ia64/sn/kernel/io_common.c
@@ -441,7 +441,7 @@ void sn_generate_path(struct pci_bus *pci_bus, char *address)
bricktype = MODULE_GET_BTYPE(moduleid);
if ((bricktype == L1_BRICKTYPE_191010) ||
(bricktype == L1_BRICKTYPE_1932))
- sprintf(address, "%s^%d", address, geo_slot(geoid));
+ sprintf(address + strlen(address), "^%d", geo_slot(geoid));
}
void __devinit
diff --git a/arch/s390/kernel/early.c b/arch/s390/kernel/early.c
index 2a2ca26..addee93 100644
--- a/arch/s390/kernel/early.c
+++ b/arch/s390/kernel/early.c
@@ -108,13 +108,13 @@ static noinline __init void create_kernel_nss(void)
sinitrd_pfn = PFN_DOWN(__pa(INITRD_START));
einitrd_pfn = PFN_UP(__pa(INITRD_START + INITRD_SIZE));
min_size = einitrd_pfn << 2;
- sprintf(defsys_cmd, "%s EW %.5X-%.5X", defsys_cmd,
+ sprintf(defsys_cmd + strlen(defsys_cmd), " EW %.5X-%.5X",
sinitrd_pfn, einitrd_pfn);
}
#endif
- sprintf(defsys_cmd, "%s EW MINSIZE=%.7iK PARMREGS=0-13",
- defsys_cmd, min_size);
+ sprintf(defsys_cmd + strlen(defsys_cmd),
+ " EW MINSIZE=%.7iK PARMREGS=0-13", min_size);
sprintf(savesys_cmd, "SAVESYS %s \n IPL %s",
kernel_nss_name, kernel_nss_name);
diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 3f46afb..396b720 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -709,13 +709,12 @@ static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf)
pci_read_config_dword(dev, 0x1BC + i * 4, ®);
- ret += sprintf(buf, "%sEntry: %d\n", buf, i);
- ret += sprintf(buf, "%sReads: %s\tNew Entries: %s\n",
- buf,
+ ret += sprintf(buf + strlen(buf), "Entry: %d\n", i);
+ ret += sprintf(buf + strlen(buf), "Reads: %s\tNew Entries: %s\n",
reg & 0x80000000 ? "Disabled" : "Allowed",
reg & 0x40000000 ? "Disabled" : "Allowed");
- ret += sprintf(buf, "%sSubCache: %x\tIndex: %x\n",
- buf, (reg & 0x30000) >> 16, reg & 0xfff);
+ ret += sprintf(buf + strlen(buf), "SubCache: %x\tIndex: %x\n",
+ (reg & 0x30000) >> 16, reg & 0xfff);
}
return ret;
}
diff --git a/drivers/media/video/se401.c b/drivers/media/video/se401.c
index 044a2e9..b8d2f03 100644
--- a/drivers/media/video/se401.c
+++ b/drivers/media/video/se401.c
@@ -1276,7 +1276,7 @@ static int se401_init(struct usb_se401 *se401, int button)
}
sprintf (temp, "%s Sizes:", temp);
for (i=0; i<se401->sizes; i++) {
- sprintf(temp, "%s %dx%d", temp, se401->width[i], se401->height[i]);
+ sprintf(temp + strlen(temp), " %dx%d", se401->width[i], se401->height[i]);
}
dev_info(&se401->dev->dev, "%s\n", temp);
se401->maxframesize=se401->width[se401->sizes-1]*se401->height[se401->sizes-1]*3;
diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c
index 59349a3..5748198 100644
--- a/drivers/scsi/gdth_proc.c
+++ b/drivers/scsi/gdth_proc.c
@@ -196,7 +196,7 @@ static int gdth_get_info(char *buffer,char **start,off_t offset,int length,
for (i = 1; i < MAX_RES_ARGS; i++) {
if (reserve_list[i] == 0xff)
break;
- sprintf(hrec,"%s,%d", hrec, reserve_list[i]);
+ sprintf(hrec + strlen(hrec), ",%d", reserve_list[i]);
}
}
size = sprintf(buffer+len,
diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index 06dd114..f52cfa5 100644
--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -1393,7 +1393,7 @@ static int usbatm_print_packet(const unsigned char *data, int len)
buffer[0] = '\0';
sprintf(buffer, "%.3d :", i);
for (j = 0; (j < 16) && (i < len); j++, i++) {
- sprintf(buffer, "%s %2.2x", buffer, data[i]);
+ sprintf(buffer + strlen(buffer), " %2.2x", data[i]);
}
dbg("%s", buffer);
}
--
1.6.0.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Don't call sprintf() with overlapping input and output.
2008-12-07 11:31 [PATCH] Don't call sprintf() with overlapping input and output Anders Kaseorg
@ 2008-12-07 12:04 ` Tom Spink
2008-12-07 22:31 ` [PATCH] Don't call snprintf() " Anders Kaseorg
1 sibling, 0 replies; 3+ messages in thread
From: Tom Spink @ 2008-12-07 12:04 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Linus Torvalds, linux-kernel
2008/12/7 Anders Kaseorg <andersk@mit.edu>:
> The use of sprintf() to append to a buffer, as in
> sprintf(buf, "%sEntry: %d\n", buf, i)
> is not valid according to C99 ("If copying takes place between objects
> that overlap, the behavior is undefined."). It breaks at least in
> userspace under gcc -D_FORTIFY_SOURCE. Replace this construct with
> sprintf(buf + strlen(buf), "Entry: %d\n", i)
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
> Documentation/ia64/err_inject.txt | 2 +-
> arch/ia64/sn/kernel/io_common.c | 2 +-
> arch/s390/kernel/early.c | 6 +++---
> arch/x86/kernel/cpu/intel_cacheinfo.c | 9 ++++-----
> drivers/media/video/se401.c | 2 +-
> drivers/scsi/gdth_proc.c | 2 +-
> drivers/usb/atm/usbatm.c | 2 +-
> 7 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/ia64/err_inject.txt b/Documentation/ia64/err_inject.txt
> index 223e4f0..62d0028 100644
> --- a/Documentation/ia64/err_inject.txt
> +++ b/Documentation/ia64/err_inject.txt
> @@ -376,7 +376,7 @@ int create_sem(int cpu)
> int sid;
>
> sprintf(fn, PATH_FORMAT, cpu);
> - sprintf(fn, "%s/%s", fn, "err_type_info");
> + sprintf(fn + strlen(fn), "/%s", "err_type_info");
> if ((key[cpu] = ftok(fn, 'e')) == -1) {
> perror("ftok");
> return -1;
> diff --git a/arch/ia64/sn/kernel/io_common.c b/arch/ia64/sn/kernel/io_common.c
> index 8a924a5..656e201 100644
> --- a/arch/ia64/sn/kernel/io_common.c
> +++ b/arch/ia64/sn/kernel/io_common.c
> @@ -441,7 +441,7 @@ void sn_generate_path(struct pci_bus *pci_bus, char *address)
> bricktype = MODULE_GET_BTYPE(moduleid);
> if ((bricktype == L1_BRICKTYPE_191010) ||
> (bricktype == L1_BRICKTYPE_1932))
> - sprintf(address, "%s^%d", address, geo_slot(geoid));
> + sprintf(address + strlen(address), "^%d", geo_slot(geoid));
> }
>
> void __devinit
> diff --git a/arch/s390/kernel/early.c b/arch/s390/kernel/early.c
> index 2a2ca26..addee93 100644
> --- a/arch/s390/kernel/early.c
> +++ b/arch/s390/kernel/early.c
> @@ -108,13 +108,13 @@ static noinline __init void create_kernel_nss(void)
> sinitrd_pfn = PFN_DOWN(__pa(INITRD_START));
> einitrd_pfn = PFN_UP(__pa(INITRD_START + INITRD_SIZE));
> min_size = einitrd_pfn << 2;
> - sprintf(defsys_cmd, "%s EW %.5X-%.5X", defsys_cmd,
> + sprintf(defsys_cmd + strlen(defsys_cmd), " EW %.5X-%.5X",
> sinitrd_pfn, einitrd_pfn);
> }
> #endif
>
> - sprintf(defsys_cmd, "%s EW MINSIZE=%.7iK PARMREGS=0-13",
> - defsys_cmd, min_size);
> + sprintf(defsys_cmd + strlen(defsys_cmd),
> + " EW MINSIZE=%.7iK PARMREGS=0-13", min_size);
> sprintf(savesys_cmd, "SAVESYS %s \n IPL %s",
> kernel_nss_name, kernel_nss_name);
>
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 3f46afb..396b720 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -709,13 +709,12 @@ static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf)
>
> pci_read_config_dword(dev, 0x1BC + i * 4, ®);
>
> - ret += sprintf(buf, "%sEntry: %d\n", buf, i);
> - ret += sprintf(buf, "%sReads: %s\tNew Entries: %s\n",
> - buf,
> + ret += sprintf(buf + strlen(buf), "Entry: %d\n", i);
> + ret += sprintf(buf + strlen(buf), "Reads: %s\tNew Entries: %s\n",
> reg & 0x80000000 ? "Disabled" : "Allowed",
> reg & 0x40000000 ? "Disabled" : "Allowed");
> - ret += sprintf(buf, "%sSubCache: %x\tIndex: %x\n",
> - buf, (reg & 0x30000) >> 16, reg & 0xfff);
> + ret += sprintf(buf + strlen(buf), "SubCache: %x\tIndex: %x\n",
> + (reg & 0x30000) >> 16, reg & 0xfff);
> }
> return ret;
> }
> diff --git a/drivers/media/video/se401.c b/drivers/media/video/se401.c
> index 044a2e9..b8d2f03 100644
> --- a/drivers/media/video/se401.c
> +++ b/drivers/media/video/se401.c
> @@ -1276,7 +1276,7 @@ static int se401_init(struct usb_se401 *se401, int button)
> }
> sprintf (temp, "%s Sizes:", temp);
> for (i=0; i<se401->sizes; i++) {
> - sprintf(temp, "%s %dx%d", temp, se401->width[i], se401->height[i]);
> + sprintf(temp + strlen(temp), " %dx%d", se401->width[i], se401->height[i]);
> }
> dev_info(&se401->dev->dev, "%s\n", temp);
> se401->maxframesize=se401->width[se401->sizes-1]*se401->height[se401->sizes-1]*3;
> diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c
> index 59349a3..5748198 100644
> --- a/drivers/scsi/gdth_proc.c
> +++ b/drivers/scsi/gdth_proc.c
> @@ -196,7 +196,7 @@ static int gdth_get_info(char *buffer,char **start,off_t offset,int length,
> for (i = 1; i < MAX_RES_ARGS; i++) {
> if (reserve_list[i] == 0xff)
> break;
> - sprintf(hrec,"%s,%d", hrec, reserve_list[i]);
> + sprintf(hrec + strlen(hrec), ",%d", reserve_list[i]);
> }
> }
> size = sprintf(buffer+len,
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> index 06dd114..f52cfa5 100644
> --- a/drivers/usb/atm/usbatm.c
> +++ b/drivers/usb/atm/usbatm.c
> @@ -1393,7 +1393,7 @@ static int usbatm_print_packet(const unsigned char *data, int len)
> buffer[0] = '\0';
> sprintf(buffer, "%.3d :", i);
> for (j = 0; (j < 16) && (i < len); j++, i++) {
> - sprintf(buffer, "%s %2.2x", buffer, data[i]);
> + sprintf(buffer + strlen(buffer), " %2.2x", data[i]);
> }
> dbg("%s", buffer);
> }
> --
> 1.6.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Hi,
That to me looks like it's begging for 'strcatf'... :)
--
Tom Spink
Zsa Zsa Gabor - "A man in love is incomplete until he has married.
Then he's finished."
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] Don't call snprintf() with overlapping input and output.
2008-12-07 11:31 [PATCH] Don't call sprintf() with overlapping input and output Anders Kaseorg
2008-12-07 12:04 ` Tom Spink
@ 2008-12-07 22:31 ` Anders Kaseorg
1 sibling, 0 replies; 3+ messages in thread
From: Anders Kaseorg @ 2008-12-07 22:31 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel
The use of snprintf() to append to a buffer, as in
snprintf(codec->name, sizeof(codec->name),
"%s[%d]", codec->name, h->attached)
is not valid according to C99 ("If copying takes place between objects
that overlap, the behavior is undefined."). It breaks at least in
userspace under gcc -D_FORTIFY_SOURCE. Replace this construct with
snprintf(codec->name + strlen(codec->name),
sizeof(codec->name) - strlen(codec->name),
"[%d]", h->attached);
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
arch/mips/bcm47xx/prom.c | 5 +++--
drivers/input/joystick/analog.c | 5 +++--
drivers/input/misc/ati_remote.c | 5 +++--
drivers/media/video/zoran/videocodec.c | 5 +++--
4 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/arch/mips/bcm47xx/prom.c b/arch/mips/bcm47xx/prom.c
index 079e33d..c91e3b2 100644
--- a/arch/mips/bcm47xx/prom.c
+++ b/arch/mips/bcm47xx/prom.c
@@ -118,8 +118,9 @@ static __init void prom_init_cmdline(void)
strcpy(buf, "uart0");
/* Compute the new command line */
- snprintf(arcs_cmdline, CL_SIZE, "%s console=ttyS%c,115200",
- arcs_cmdline, buf[4]);
+ snprintf(arcs_cmdline + strlen(arcs_cmdline),
+ CL_SIZE - strlen(arcs_cmdline),
+ " console=ttyS%c,115200", buf[4]);
}
}
diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 356b3a2..2fe178e 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -412,8 +412,9 @@ static void analog_name(struct analog *analog)
hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) + !!(analog->mask & ANALOG_HBTN_CHF) * 4);
if (analog->mask & ANALOG_HATS_ALL)
- snprintf(analog->name, sizeof(analog->name), "%s %d-hat",
- analog->name, hweight16(analog->mask & ANALOG_HATS_ALL));
+ snprintf(analog->name + strlen(analog->name),
+ sizeof(analog->name) - strlen(analog->name),
+ " %d-hat", hweight16(analog->mask & ANALOG_HATS_ALL));
if (analog->mask & ANALOG_HAT_FCS)
strlcat(analog->name, " FCS", sizeof(analog->name));
diff --git a/drivers/input/misc/ati_remote.c b/drivers/input/misc/ati_remote.c
index e290fde..f46ce58 100644
--- a/drivers/input/misc/ati_remote.c
+++ b/drivers/input/misc/ati_remote.c
@@ -772,8 +772,9 @@ static int ati_remote_probe(struct usb_interface *interface, const struct usb_de
strlcpy(ati_remote->name, udev->manufacturer, sizeof(ati_remote->name));
if (udev->product)
- snprintf(ati_remote->name, sizeof(ati_remote->name),
- "%s %s", ati_remote->name, udev->product);
+ snprintf(ati_remote->name + strlen(ati_remote->name),
+ sizeof(ati_remote->name) - strlen(ati_remote->name),
+ " %s", udev->product);
if (!strlen(ati_remote->name))
snprintf(ati_remote->name, sizeof(ati_remote->name),
diff --git a/drivers/media/video/zoran/videocodec.c b/drivers/media/video/zoran/videocodec.c
index cf24956..dbcfaef 100644
--- a/drivers/media/video/zoran/videocodec.c
+++ b/drivers/media/video/zoran/videocodec.c
@@ -117,8 +117,9 @@ videocodec_attach (struct videocodec_master *master)
}
memcpy(codec, h->codec, sizeof(struct videocodec));
- snprintf(codec->name, sizeof(codec->name),
- "%s[%d]", codec->name, h->attached);
+ snprintf(codec->name + strlen(codec->name),
+ sizeof(codec->name) - strlen(codec->name),
+ "[%d]", h->attached);
codec->master_data = master;
res = codec->setup(codec);
if (res == 0) {
--
1.6.0.4
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-12-07 22:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-12-07 11:31 [PATCH] Don't call sprintf() with overlapping input and output Anders Kaseorg
2008-12-07 12:04 ` Tom Spink
2008-12-07 22:31 ` [PATCH] Don't call snprintf() " Anders Kaseorg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®