Mailing List Archive

patch for libclamav/scanners.c
Hello,
I have attached a patch to libclamav/scanners.c (against the latest
clamav-devel checkout), without which I have
varying levels of failure with clamd/clam(d)scan

There are two groups of changes:
- fixup error cases, particularly the return/use of error values to
upper layers. Without this, recursion/size limits failed in various
ways, particularly when scanning mailboxes
- malloc a buffer in the cli_scan[bzip,zip,gzip] functions, rather than
alloc it on the stack. Without this, on freebsd I have to set the
stack limit (via pthread_attr_setstacksize) to a very large value (at
least on a 5.1-current freebsd machine, I was crashing with the
default big-stack value of 64*1024, had to increase this to 1024*1024
in clamd/server.c). My chance seems to be in the same vein as the
allocations done in cli_scandesc.

Any feedback on these fixes, particularly with the aim of getting them
modified so they will be accepted for commiting into the tree, would be
appreciated. I've done a fair amount of testing and cannot see any
obvious problems, but would welcome any adverse reports.

Thanks.
Re: patch for libclamav/scanners.c [ In reply to ]
Apologies for reply to myself, but I've now attached a second, slightly
cleaned-up patch to this email. The only changes are that I've removed
some commented-out code to make it slightly more readable.
Re: patch for libclamav/scanners.c [ In reply to ]
Rudolph Pereira wrote:

> Apologies for reply to myself, but I've now attached a second, slightly
> cleaned-up patch to this email. The only changes are that I've removed
> some commented-out code to make it slightly more readable.
>
>
Looks good to me on a first sight.

+1

Thomas
Re: patch for libclamav/scanners.c [ In reply to ]
On Wed, 5 Nov 2003 11:03:29 +1100
Rudolph Pereira <r.pereira@isu.usyd.edu.au> wrote:

Index: libclamav/scanners.c
===================================================================
RCS file: /cvsroot/clamav/clamav-devel/libclamav/scanners.c,v
retrieving revision 1.11
diff -u -b -w -B -d -r1.11 scanners.c
--- libclamav/scanners.c 29 Sep 2003 11:44:52 -0000 1.11
+++ libclamav/scanners.c 4 Nov 2003 23:51:29 -0000
@@ -241,7 +241,7 @@
ZZIP_DIRENT zdirent;
ZZIP_FILE *zfp;
FILE *tmp;
- char buff[BUFFSIZE];
+ char *buff;
int fd, bytes, files = 0, ret = CL_CLEAN, err;


@@ -253,7 +253,14 @@
return CL_EZIP;
}

- while(zzip_dir_read(zdir, &zdirent)) {
+ /* malloc a buffer for the read */
+ if(!(buff=(char *)cli_calloc(BUFFSIZE,sizeof(char))))
+ {
+ ret=CL_EMEM;
+ }
+
+ while((ret==CL_CLEAN) && zzip_dir_read(zdir, &zdirent))
+ {
cli_dbgmsg("Zip -> %s, compressed: %d, normal: %d.\n",
zdirent.d_name, zdirent.d_csize, zdirent.st_size);

if(!zdirent.st_size) { /* omit directories and null files */
@@ -274,7 +281,8 @@
cli_dbgmsg("Zip -> %s: Size exceeded (%d, max: %d)\n",
zdirent.d_name, zdirent.st_size, limits->maxfilesize);
files++;
ret = CL_EMAXSIZE;
- continue;
+ /*continue;*/
+ break; /*this seems more correct*/
}

if(limits->maxfiles && (files > limits->maxfiles)) {
@@ -287,39 +295,48 @@
/* generate temporary file and get its descriptor */
if((tmp = tmpfile()) == NULL) {
cli_dbgmsg("Zip -> Can't generate tmpfile().\n");
- zzip_dir_close(zdir);
- return CL_ETMPFILE;
+ ret = CL_ETMPFILE;
+ break;
}

if((zfp = zzip_file_open(zdir, zdirent.d_name, 0)) == NULL) {
cli_dbgmsg("Zip -> %s: Can't open file.\n", zdirent.d_name);
ret = CL_EZIP;
- continue;
+ /*continue;*/
+ break; /* this seems more correct */
}

- while((bytes = zzip_file_read(zfp, buff, BUFFSIZE)) > 0) {
+ while((ret==CL_CLEAN) && ((bytes = zzip_file_read(zfp, buff,
BUFFSIZE)) > 0)) {
if(fwrite(buff, bytes, 1, tmp)*bytes != bytes) {
cli_dbgmsg("Zip -> Can't fwrite() file: %s\n",
strerror(errno));
- zzip_file_close(zfp);
- zzip_dir_close(zdir);
+ /*zzip_file_close(zfp);
+ zzip_dir_close(zdir);*/
files++;
- fclose(tmp);
- return CL_EZIP;
+ /*fclose(tmp);*/
+ ret = CL_EZIP;
+ break; /* we want to get out of the outer loop as well,
additional check is performed below */
}
}

zzip_file_close(zfp);

+ if(ret != CL_CLEAN)
+ break; /* allow exit from outer loop - see while loop above */
+
if(fflush(tmp) != 0) {
cli_errmsg("fflush() failed: %s\n", strerror(errno));
- zzip_dir_close(zdir);
- fclose(tmp);
- return CL_EFSYNC;
+ /*zzip_dir_close(zdir);
+ fclose(tmp);*/
+ ret = CL_EFSYNC;
+ break;
}

fd = fileno(tmp);

+ if(ret == CL_CLEAN)
+ {
lseek(fd, 0, SEEK_SET);
+
if((ret = cli_magic_scandesc(fd, virname, scanned, root, limits,
options, reclev)) == CL_VIRUS ) {
cli_dbgmsg("Zip -> Found %s virus.\n", *virname);
fclose(tmp);
@@ -324,7 +341,6 @@
cli_dbgmsg("Zip -> Found %s virus.\n", *virname);
fclose(tmp);
tmp = NULL;
- ret = CL_VIRUS;
break;
} else if(ret == CL_EMALFZIP) {
/*
@@ -338,6 +354,7 @@
ret = CL_VIRUS;
break;
}
+ }

if (tmp) {
fclose(tmp);
@@ -346,6 +363,7 @@
files++;
}

+ free(buff);
zzip_dir_close(zdir);
if (tmp) {
fclose(tmp);
@@ -358,17 +376,19 @@
{
int fd, bytes, ret = CL_CLEAN;
long int size = 0;
- char buff[BUFFSIZE];
+ char *buff;
FILE *tmp;
gzFile gd;


- if((gd = gzdopen(dup(desc), "rb")) == NULL) {
+ if((gd = gzdopen(dup(desc), "rb")) == NULL)
+ {
cli_dbgmsg("Can't gzdopen() descriptor %d.\n", desc);
return CL_EGZIP;
}

- if((tmp = tmpfile()) == NULL) {
+ if((tmp = tmpfile()) == NULL)
+ {
cli_dbgmsg("Can't generate tmpfile().\n");
gzclose(gd);
return CL_ETMPFILE;
@@ -373,41 +393,53 @@
gzclose(gd);
return CL_ETMPFILE;
}
+
fd = fileno(tmp);

- while((bytes = gzread(gd, buff, BUFFSIZE)) > 0) {
+ /* malloc a buffer for the read */
+ if(!(buff=(char *)cli_calloc(BUFFSIZE,sizeof(char))))
+ {
+ ret=CL_EMEM;
+ }
+
+ while((ret == CL_CLEAN) && ((bytes = gzread(gd, buff, BUFFSIZE)) >
0))
+ {
size += bytes;

- if(limits)
- if(limits->maxfilesize && (size + BUFFSIZE > limits->maxfilesize))
{
+ if(limits && limits->maxfilesize && (size + BUFFSIZE >
limits->maxfilesize))
+ {
cli_dbgmsg("Gzip->desc(%d): Size exceeded (stopped at %d, max:
%d)\n", desc, size, limits->maxfilesize);
ret = CL_EMAXSIZE;
break;
}

- if(write(fd, buff, bytes) != bytes) {
+ if(write(fd, buff, bytes) != bytes)
+ {
cli_dbgmsg("Gzip -> Can't write() file.\n");
- fclose(tmp);
- gzclose(gd);
- return CL_EGZIP;
+ ret=CL_EGZIP;
+ break;
}
}

gzclose(gd);
- if(fsync(fd) == -1) {
+ free(buff);
+
+ if((ret == CL_CLEAN) && (fsync(fd) == -1))
+ {
cli_dbgmsg("fsync() failed for descriptor %d\n", fd);
- fclose(tmp);
- return CL_EFSYNC;
+ ret=CL_EFSYNC;
}

+ if(ret==CL_CLEAN)
+ {
lseek(fd, 0, SEEK_SET);
- if((ret = cli_magic_scandesc(fd, virname, scanned, root, limits,
options, reclev)) == CL_VIRUS ) {
+ if((ret = cli_magic_scandesc(fd, virname, scanned, root,
limits, options, reclev)) == CL_VIRUS )
+ {
cli_dbgmsg("Gzip -> Found %s virus.\n", *virname);
- fclose(tmp);
- return CL_VIRUS;
}
- fclose(tmp);
+ }

+ fclose(tmp);
return ret;
}
#endif
@@ -425,7 +457,7 @@
int fd, bytes, ret = CL_CLEAN, bzerror = 0;
short memlim = 0;
long int size = 0;
- char buff[BUFFSIZE];
+ char *buff;
FILE *fs, *tmp;
BZFILE *bfd;

@@ -451,37 +483,51 @@
}
fd = fileno(tmp);

- while((bytes = BZ2_bzRead(&bzerror, bfd, buff, BUFFSIZE)) > 0) {
+ /* malloc a buffer for the read */
+ if(!(buff=(char *)cli_calloc(BUFFSIZE,sizeof(char))))
+ {
+ ret=CL_EMEM;
+ }
+
+
+ while((ret==CL_CLEAN) && ((bytes = BZ2_bzRead(&bzerror, bfd, buff,
BUFFSIZE)) > 0))
+ {
size += bytes;

- if(limits)
- if(limits->maxfilesize && (size + BUFFSIZE > limits->maxfilesize))
{
+ if(limits && limits->maxfilesize && (size + BUFFSIZE >
limits->maxfilesize))
+ {
cli_dbgmsg("Bzip2->desc(%d): Size exceeded (stopped at %d, max:
%d)\n", desc, size, limits->maxfilesize);
ret = CL_EMAXSIZE;
break;
}

- if(write(fd, buff, bytes) != bytes) {
+
+ if(write(fd, buff, bytes) != bytes)
+ {
cli_dbgmsg("Bzip2 -> Can't write() file.\n");
- BZ2_bzReadClose(&bzerror, bfd);
- fclose(tmp);
- return CL_EGZIP;
+ ret=CL_EGZIP;
+ break;
}
}

BZ2_bzReadClose(&bzerror, bfd);
- if(fsync(fd) == -1) {
+ free(buff);
+
+ if((ret == CL_CLEAN) && (fsync(fd) == -1))
+ {
cli_dbgmsg("fsync() failed for descriptor %d\n", fd);
- fclose(tmp);
- return CL_EFSYNC;
+ ret=CL_EFSYNC;
}

+ if(ret==CL_CLEAN)
+ {
lseek(fd, 0, SEEK_SET);
- if((ret = cli_magic_scandesc(fd, virname, scanned, root, limits,
options, reclev)) == CL_VIRUS ) {
+ if((ret = cli_magic_scandesc(fd, virname, scanned, root,
limits, options, reclev)) == CL_VIRUS )
+ {
cli_dbgmsg("Bzip2 -> Found %s virus.\n", *virname);
- fclose(tmp);
- return CL_VIRUS;
}
+ }
+
fclose(tmp);

return ret;
@@ -494,6 +540,7 @@
struct dirent *dent;
struct stat statbuf;
char *fname;
+ int ret=CL_CLEAN;


if((dd = opendir(dirname)) != NULL) {
@@ -507,13 +554,13 @@
/* stat the file */
if(lstat(fname, &statbuf) != -1) {
if(S_ISDIR(statbuf.st_mode) &&
!S_ISLNK(statbuf.st_mode))
- cli_scandir(dirname, virname, scanned, root, limits,
options, reclev);
+ ret=cli_scandir(dirname, virname, scanned, root, limits,
options, reclev);
else
if(S_ISREG(statbuf.st_mode))
- if(cl_scanfile(fname, virname, scanned, root, limits,
options) == CL_VIRUS) {
+ if((ret=cl_scanfile(fname, virname, scanned, root,
limits, options)) != CL_CLEAN) {
free(fname);
closedir(dd);
- return CL_VIRUS;
+ return ret;
}

}
@@ -527,7 +574,7 @@
}

closedir(dd);
- return 0;
+ return ret;
}

int cli_scanmail(int desc, char **virname, long int *scanned, const
struct cl_node *root, const struct cl_limits *limits, int options, int
*reclev)
@@ -607,7 +654,9 @@

if(limits && limits->maxreclevel)
if(*reclev > limits->maxreclevel)
+ {
return CL_EMAXREC;
+ }

(*reclev)++;





Best regards,
Tomasz Kojm
--
oo ..... http://www.clamav.net/gpg/tkojm.gpg
(\/)\......... 0DCA5A08407D5288279DB43454822DC8985A444B
\..........._ Wed Nov 5 02:00:56 CET 2003
//\ /\
Re: patch for libclamav/scanners.c [ In reply to ]
On Wed, 5 Nov 2003 11:03:29 +1100
Rudolph Pereira <r.pereira@isu.usyd.edu.au> wrote:

> Hello,
> I have attached a patch to libclamav/scanners.c (against the latest
> clamav-devel checkout), without which I have
> varying levels of failure with clamd/clam(d)scan

Hi Rudolph,

thanks for the patch. A few small things may require a change but
generally it's OK. However it's too late to include it into the main
tree and it must wait for some time.

Best regards,
Tomasz Kojm
--
oo ..... http://www.clamav.net/gpg/tkojm.gpg
(\/)\......... 0DCA5A08407D5288279DB43454822DC8985A444B
\..........._ Wed Nov 5 02:09:14 CET 2003
//\ /\
Re: patch for libclamav/scanners.c [ In reply to ]
On Wed, 5 Nov 2003 02:03:31 +0100
Tomasz Kojm <tk@lodz.tpnet.pl> wrote:

> On Wed, 5 Nov 2003 11:03:29 +1100
> Rudolph Pereira <r.pereira@isu.usyd.edu.au> wrote:
>
> Index: libclamav/scanners.c
> ===================================================================
> RCS file: /cvsroot/clamav/clamav-devel/libclamav/scanners.c,v

Sorry for that.

Best regards,
Tomasz Kojm
--
oo ..... http://www.clamav.net/gpg/tkojm.gpg
(\/)\......... 0DCA5A08407D5288279DB43454822DC8985A444B
\..........._ Wed Nov 5 11:57:11 CET 2003
//\ /\