Mailing List Archive

snprintf conversion patch
Here is a patch to convert usage of sprintf to snprintf within the code
diff'd from cvs
just a few ago. I've isolated this patch to just deal with snprintf.

Phil.

--
It's a frail sad line between optimism and delusion.
Re: snprintf conversion patch [ In reply to ]
On Thu, Apr 08, 2004, Phil Oleson <oz@nixil.net> wrote:
> Index: clamd/clamd.c
> ===================================================================
> RCS file: /cvsroot/clamav/clamav-devel/clamd/clamd.c,v
> retrieving revision 1.21
> diff -u -b -r1.21 clamd.c
> --- clamd/clamd.c 30 Mar 2004 21:11:25 -0000 1.21
> +++ clamd/clamd.c 8 Apr 2004 06:44:50 -0000
> @@ -199,7 +199,7 @@
> /* set the temporary dir */
> if((cpt = cfgopt(copt, "TemporaryDirectory"))) {
> var = (char *) mcalloc(8 + strlen(cpt->strarg), sizeof(char));
> - sprintf(var, "TMPDIR=%s", cpt->strarg);
> + snprintf(var, sizeof(var), "TMPDIR=%s", cpt->strarg);
> if(!putenv(var))
> logg("Setting %s as global temporary directory\n", cpt->strarg);
> else

This can't possible be correct. sizeof(var) will be the size of a
pointer (4 or 8 bytes on modern architectures).

> Index: clamdscan/client.c
> ===================================================================
> RCS file: /cvsroot/clamav/clamav-devel/clamdscan/client.c,v
> retrieving revision 1.14
> diff -u -b -r1.14 client.c
> --- clamdscan/client.c 6 Apr 2004 22:43:43 -0000 1.14
> +++ clamdscan/client.c 8 Apr 2004 06:44:50 -0000
> @@ -215,21 +215,21 @@
> } else {
> file = mcalloc(200 + strlen(opt->filename) + 10, sizeof(char));
> #ifdef C_CYGWIN
> - sprintf(file, "%s", opt->filename);
> + snprintf(file, sizeof(file), "%s", opt->filename);
> #else
> /* we need the full path to the file */
> if(!getcwd(cwd, 200)) {
> mprintf("@Can't get the absolute pathname of the current working directory.\n");
> return 2;
> }
> - sprintf(file, "%s/%s", cwd, opt->filename);
> + snprintf(file, sizeof(file), "%s/%s", cwd, opt->filename);
> #endif
> }
> }
>
>
> scancmd = mcalloc(strlen(file) + 20, sizeof(char));
> - sprintf(scancmd, "CONTSCAN %s", file);
> + snprintf(scancmd, sizeof(scancmd), "CONTSCAN %s", file);
> free(file);
>
> if(write(sockd, scancmd, strlen(scancmd)) <= 0) {

Same here for these three cases.

> Index: clamscan/treewalk.c
> ===================================================================
> RCS file: /cvsroot/clamav/clamav-devel/clamscan/treewalk.c,v
> retrieving revision 1.3
> diff -u -b -r1.3 treewalk.c
> --- clamscan/treewalk.c 29 Mar 2004 00:00:58 -0000 1.3
> +++ clamscan/treewalk.c 8 Apr 2004 06:44:50 -0000
> @@ -61,7 +61,7 @@
> if(strcmp(dent->d_name, ".") && strcmp(dent->d_name, "..")) {
> /* build the full name */
> fname = mcalloc(strlen(dirname) + strlen(dent->d_name) + 2, sizeof(char));
> - sprintf(fname, "%s/%s", dirname, dent->d_name);
> + snprintf(fname, sizeof(fname), "%s/%s", dirname, dent->d_name);
>
> /* stat the file */
> if(lstat(fname, &statbuf) != -1) {
> @@ -106,7 +106,7 @@
> if(dent->d_ino) {
> if(strcmp(dent->d_name, ".") && strcmp(dent->d_name, "..")) {
> fname = mcalloc(strlen(dirname) + strlen(dent->d_name) + 2, sizeof(char));
> - sprintf(fname, "%s/%s", dirname, dent->d_name);
> + snprintf(fname, sizeof(fname), "%s/%s", dirname, dent->d_name);
>
> /* stat the file */
> if(lstat(fname, &statbuf) != -1) {
> @@ -192,7 +192,7 @@
> if(strcmp(dent->d_name, ".") && strcmp(dent->d_name, "..")) {
> /* build full name */
> fname = mcalloc(strlen(dirname) + strlen(dent->d_name) + 2, sizeof(char));
> - sprintf(fname, "%s/%s", dirname, dent->d_name);
> + snprintf(fname, sizeof(fname), "%s/%s", dirname, dent->d_name);
>
> /* stat the file */
> if(lstat(fname, &statbuf) != -1) {
> @@ -237,7 +237,7 @@
>
> /* build the full name */
> fname = mcalloc(strlen(dirname) + strlen(dent->d_name) + 2, sizeof(char));
> - sprintf(fname, "%s/%s", dirname, dent->d_name);
> + snprintf(fname, sizeof(fname), "%s/%s", dirname, dent->d_name);
>
> /* stat the file */
> if(lstat(fname, &statbuf) != -1) {

Again here.

> Index: freshclam/manager.c
> ===================================================================
> RCS file: /cvsroot/clamav/clamav-devel/freshclam/manager.c,v
> retrieving revision 1.24
> diff -u -b -r1.24 manager.c
> --- freshclam/manager.c 29 Mar 2004 00:00:58 -0000 1.24
> +++ freshclam/manager.c 8 Apr 2004 06:44:50 -0000
> @@ -354,33 +354,24 @@
>
> if(proxy) {
> remotename = mmalloc(strlen(hostname) + 8);
> - sprintf(remotename, "http://%s", hostname);
> + snprintf(remotename, sizeof(remotename), "http://%s", hostname);
>
> if(user) {
> int len;
> char *buf = mmalloc((strlen(pass) + strlen(user)) * 2 + 4);
> char *userpass = mmalloc(strlen(user) + strlen(pass) + 2);
> - sprintf(userpass, "%s:%s", user, pass);
> - len=fmt_base64(buf,userpass,strlen(userpass));
> + snprintf(userpass, sizeof(userpass), "%s:%s", user, pass);
> + len = fmt_base64(buf, userpass, strlen(userpass));
> free(userpass);
> buf[len]='\0';
> authorization = mmalloc(strlen(buf) + 30);
> - sprintf(authorization, "Proxy-Authorization: Basic %s\r\n", buf);
> + snprintf(authorization, sizeof(authorization), "Proxy-Authorization: Basic %s\r\n", buf);
> free(buf);
> }
> }

And these.

> @@ -454,18 +444,18 @@
>
> if(proxy) {
> remotename = mmalloc(strlen(hostname) + 8);
> - sprintf(remotename, "http://%s", hostname);
> + snprintf(remotename, sizeof(remotename), "http://%s", hostname);
>
> if(user) {
> int len;
> char *buf = mmalloc((strlen(pass) + strlen(user)) * 2 + 4);
> char *userpass = mmalloc(strlen(user) + strlen(pass) + 2);
> - sprintf(userpass, "%s:%s", user, pass);
> + snprintf(userpass, sizeof(userpass), "%s:%s", user, pass);
> len=fmt_base64(buf,userpass,strlen(userpass));
> free(userpass);
> buf[len]='\0';
> authorization = mmalloc(strlen(buf) + 30);
> - sprintf(authorization, "Proxy-Authorization: Basic %s\r\n", buf);
> + snprintf(authorization, sizeof(authorization), "Proxy-Authorization: Basic %s\r\n", buf);
> free(buf);
> }
> }

And again.

> Index: libclamav/vba_extract.c
> ===================================================================
> RCS file: /cvsroot/clamav/clamav-devel/libclamav/vba_extract.c,v
> retrieving revision 1.20
> diff -u -b -r1.20 vba_extract.c
> --- libclamav/vba_extract.c 5 Apr 2004 09:03:53 -0000 1.20
> +++ libclamav/vba_extract.c 8 Apr 2004 06:44:51 -0000
> @@ -265,7 +265,7 @@
> cli_dbgmsg("in vba56_dir_read()\n");
>
> fullname = (char *) cli_malloc(strlen(dir) + 15);
> - sprintf(fullname, "%s/_VBA_PROJECT", dir);
> + snprintf(fullname, sizeof(fullname), "%s/_VBA_PROJECT", dir);
> fd = open(fullname, O_RDONLY);
>
> if (fd == -1) {

Again, sizeof is incorrect.

> @@ -479,7 +483,7 @@
>
> int build(struct optstruct *opt)
> {
> - int ret, no = 0, realno = 0, bytes, itmp;
> + int ret, no = 0, realno = 0, bytes, itmp, hlen, slen;
> struct stat foo;
> char buffer[FILEBUFF], *tarfile = NULL, *gzfile = NULL, header[257],
> smbuff[30], *pt;
> @@ -575,7 +579,7 @@
>
>
> /* try to read cvd header of old database */
> - sprintf(buffer, "%s/%s", cl_retdbdir(), getargc(opt, 'b'));
> + snprintf(buffer, FILEBUFF, "%s/%s", cl_retdbdir(), getargc(opt, 'b'));
> if((oldcvd = cl_cvdhead(buffer)) == NULL)
> mprintf("WARNING: CAN'T READ CVD HEADER OF CURRENT DATABASE %s\n", buffer);
>

sizeof(buffer) is ideal here since if you change the definition of
buffer, then you don't necessarily have to change the call to snprintf
too.

> @@ -583,6 +587,8 @@
>
> /* magic string */
>
> + slen = sizeof(smbuff);
> + hlen = sizeof(header);
> strcpy(header, "ClamAV-VDB:");
>
> /* time */
> @@ -590,7 +596,7 @@
> time(&timet);
> brokent = localtime(&timet);
> setlocale(LC_TIME, "C");
> - strftime(smbuff, 30, "%d %b %Y %H-%M %z", brokent);
> + strftime(smbuff, slen, "%d %b %Y %H-%M %z", brokent);
> strcat(header, smbuff);
>
> /* version number */
> @@ -598,12 +604,12 @@
> /* ... increment version number by one */
>
> if(oldcvd) {
> - sprintf(smbuff, ":%d:", oldcvd->version + 1);
> + snprintf(smbuff, slen, ":%d:", oldcvd->version + 1);
> } else {
> fflush(stdin);
> mprintf("Version number: ");
> scanf("%d", &itmp);
> - sprintf(smbuff, "%d:", itmp);
> + snprintf(smbuff, slen,"%d:", itmp);
> }
> strcat(header, smbuff);
>

Don't really need slen, you can use sizeof(smbuff), and hlen isn't used
at all?

It looks like some of the fixes for snprintf() were done correctly, but
a lot weren't done correctly.

JE
Re: snprintf conversion patch [ In reply to ]
Fun.. I love it when I publically mess up.. heh.. ugh.. thanks for
pointing out my gross oversight..
I do know better.. *shrug* I'll not blame lack of sleep.. dunno why I
did that..

Here is a fixed up patch.. noticed a couple mmalloc()s that made some
assumptions
that I changed to mcalloc().

Johannes Erdfelt wrote:

>On Thu, Apr 08, 2004, Phil Oleson <oz@nixil.net> wrote:
>
>
>>@@ -479,7 +483,7 @@
>>
>> int build(struct optstruct *opt)
>> {
>>- int ret, no = 0, realno = 0, bytes, itmp;
>>+ int ret, no = 0, realno = 0, bytes, itmp, hlen, slen;
>> struct stat foo;
>> char buffer[FILEBUFF], *tarfile = NULL, *gzfile = NULL, header[257],
>> smbuff[30], *pt;
>>@@ -575,7 +579,7 @@
>>
>>
>> /* try to read cvd header of old database */
>>- sprintf(buffer, "%s/%s", cl_retdbdir(), getargc(opt, 'b'));
>>+ snprintf(buffer, FILEBUFF, "%s/%s", cl_retdbdir(), getargc(opt, 'b'));
>> if((oldcvd = cl_cvdhead(buffer)) == NULL)
>> mprintf("WARNING: CAN'T READ CVD HEADER OF CURRENT DATABASE %s\n", buffer);
>>
>>
>>
>
>sizeof(buffer) is ideal here since if you change the definition of
>buffer, then you don't necessarily have to change the call to snprintf
>too.
>
>
It's not going to hurt.. no lookups needed.. but whatever..

>>@@ -583,6 +587,8 @@
>>
>> /* magic string */
>>
>>+ slen = sizeof(smbuff);
>>+ hlen = sizeof(header);
>> strcpy(header, "ClamAV-VDB:");
>>
>> /* time */
>>@@ -590,7 +596,7 @@
>> time(&timet);
>> brokent = localtime(&timet);
>> setlocale(LC_TIME, "C");
>>- strftime(smbuff, 30, "%d %b %Y %H-%M %z", brokent);
>>+ strftime(smbuff, slen, "%d %b %Y %H-%M %z", brokent);
>> strcat(header, smbuff);
>>
>> /* version number */
>>@@ -598,12 +604,12 @@
>> /* ... increment version number by one */
>>
>> if(oldcvd) {
>>- sprintf(smbuff, ":%d:", oldcvd->version + 1);
>>+ snprintf(smbuff, slen, ":%d:", oldcvd->version + 1);
>> } else {
>> fflush(stdin);
>> mprintf("Version number: ");
>> scanf("%d", &itmp);
>>- sprintf(smbuff, "%d:", itmp);
>>+ snprintf(smbuff, slen,"%d:", itmp);
>> }
>> strcat(header, smbuff);
>>
>>
>>
>
>Don't really need slen, you can use sizeof(smbuff), and hlen isn't used
>at all?
>
>It looks like some of the fixes for snprintf() were done correctly, but
>a lot weren't done correctly.
>
>JE
>
>
>
>
>
hlen was from the strlcpy reversion that happened.. I just didnt clean
it up enough..
I left the slen in cuz it reduced the # to times sizeof is calculated.
*shrug..


Phil.
Re: snprintf conversion patch [ In reply to ]
Blah.. I just dont listen sometimes.. scrap the last rev..
Re: snprintf conversion patch [ In reply to ]
On Thu, Apr 08, 2004, Phil Oleson <oz@nixil.net> wrote:
> hlen was from the strlcpy reversion that happened.. I just didnt clean
> it up enough..

Ahh, that makes sense.

> I left the slen in cuz it reduced the # to times sizeof is calculated.
> *shrug..

sizeof() is calculated at compile time, so I wouldn't worry about any
performance issues.

JE
Re: snprintf conversion patch [ In reply to ]
I was really going to leave it at the last patch.. but I realized my
patch wasnt clean.. blah..
I tweaked some final bits.. should be able to let it be with this one.

Phil.