Date: Fri, 20 Nov 1998 21:37:21 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: Archie Cobbs <archie@whistle.com>, grog@lemis.com, rnordier@nordier.com, freebsd-current@FreeBSD.ORG Subject: Re: snprintf() in the kernel Message-ID: <199811210537.VAA20822@apollo.backplane.com> References: <199811210312.TAA25314@bubba.whistle.com> <199811210407.UAA20321@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I've gone through your diffs. I found what I believe to be one genuine
bug, but read the notes and stuff below... you may have a problem with
some of the strncpy()'s you converted to snprintf's.
General comments:
* found a few minor bugs.
* noticed a number of 'scary uses' in original code were fixed, kudos!
* noticed a few bugs in the original code were fixed, kudos! For
example, a number of strncpy()'s assume zero-termination. strncpy()
does not zero-terminate in the exact-length case.
Notes:
It should be noted that the FreeBSD strncpy() will zero-fill the
destination. snprintf() does not. This may or may not matter. It
didn't seem to be important in what I saw in my review but you
should review the disklabel-related snprintf()'s.
Also review zero-termination cases for disklabels... make sure
disklabel isn't designed to allow a full-length string (without
zero-termination) else your snprintf()'s have broke something.
I noticed a number of strncpy()'s were using sizeof(the_buffer_name),
which might have been on purpose! snprintf() will break that.
-Matt
alpha/alpha/db_disasm.c
line 196, static char unk[8]; ... buffer is not big enough. Needs to be
at least 11. The snprintf() prevents a possible overflow (a bug fixed!),
but the buffer should probably be made larger anyway.
cam/scsi/scsi_all.c
line 1566, char holdstr[8] was totally broken anyway, it wasn't big
enough (needs to be 10). Your patch removes it which is good.
A second bug fixed!
dev/buslogin/bt.c
Boy, weren't we lucky that all BT model names were 4 characters!
dev/dpt/dpt_control.c
This doesn't look right, you forgot to remove the strncpy and replace it
with the snprintf, but added the extra sizeof() argument as if you had.
@@ -782,7 +783,8 @@
compat_softc.ha_npend = dpt->submitted_ccbs_coun
t;
compat_softc.ha_active_jobs = dpt->waiting_ccbs_
count;
strncpy(compat_softc.ha_fw_version,
- dpt->board_data.firmware, 4);
+ dpt->board_data.firmware,
+ sizeof(compat_softc.ha_fw_version));
compat_softc.ha_ccb = NULL;
compat_softc.ha_cblist = NULL;
i386/isa/mcd.c
(Looks like you fixed a bug here... the strncpy was assuming zero
termination)
netatm/spans/spans_print.c
(Hey, nice, you fixed another bug, at least if someone ever made
saddr and daddr differently sized).
- strncpy(daddr, spans_addr_print(&stind_p->stind_es_addr),
- sizeof(daddr));
- strncpy(saddr, spans_addr_print(&stind_p->stind_sw_addr),
- sizeof(daddr));
+ snprintf(daddr, sizeof(daddr),
+ "%s", spans_addr_print(&stind_p->stind_es_addr));
+ snprintf(saddr, sizeof(daddr),
+ "%s", spans_addr_print(&stind_p->stind_sw_addr));
printf("sw_epoch=0x%lx, es_addr=%s, sw_addr=0x%s",
...
netatm/spans/spans_util.c:
I'm not sure about these ntohl() calls. Some of the defines just
macro the arguments through, others are routines, but historically
ntohl() has operated on a 32 bit quantity so we should probably
cast to ntohl() results to (long) in the s*printf() calls as well
as do the sprintf()->snprintf() conversion.
RCS file: /cvs/freebsd/src/sys/netatm/spans/spans_util.c,v
retrieving revision 1.3
diff -u -r1.3 spans_util.c
--- spans_util.c 1998/10/31 20:06:56 1.3
+++ spans_util.c 1998/11/21 03:03:09
@@ -441,7 +441,7 @@
/*
* Print and return the string
*/
- sprintf(strbuff, "%lx.%lx", ntohl(u1.w), ntohl(u2.w));
+ snprintf(strbuff, sizeof(strbuff), "%lx.%lx", ntohl(u1.w), ntohl(u2.w));
return(strbuff);
}
netinet/ip_divert.c:
You might have to zero-fill unused space in sin_zero. I'm not sure.
@@ -217,9 +216,9 @@
* this iface name will come along for the ride.
* (see div_output for the other half of this.)
*/
- sprintf(name, "%s%d",
- m->m_pkthdr.rcvif->if_name, m->m_pkthdr.rcvif->if_unit);
- strncpy(divsrc.sin_zero, name, 7);
+ snprintf(divsrc.sin_zero, sizeof(divsrc.sin_zero),
+ "%s%d", m->m_pkthdr.rcvif->if_name,
+ m->m_pkthdr.rcvif->if_unit);
}
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199811210537.VAA20822>