Timo Lilja wrote: > Procmeter3 segfaults immediately on startup if /proc/mounts has long > lines. > > The function Initialise() in modules/df.c has few buffer overflows > when there are long lines in /proc/mounts: sscanf() is not called with > a proper size arguments on one occasion. > > The line[] buffer size is currently 128 which is a bit too small. The > attached patch fixes both the sscanf() format string and the size of > the line[] buffer by extending it to 256 characters.
Sizing the agruments to sscanf and losing the segfault is definitly a
good thing. Raising the static buffer size seems to just be papering
over the problem of there being a static buffer in the first place, as
far as I know there's nothing special about 256 bytes that makes it big
enough in all cases.
I wonder if perhaps there's a potential for this to be a security hole,
although at the moment I can't think of any setups that let an untrusted
user put arbitrary data in /proc/mounts ..
> It might be a good idea to add some functionality to the GUI rendering
> of the disk information as well. At the moment, only the initial part
> of the mount name is displayed to the user.
Maybe Andrew can do that..
> $ uname -a
> Linux dog 2.6.17-2-686 #1 SMP Wed Sep 13 16:34:10 UTC 2006 i686 GNU/Linux
> $ dpkg -s libc6 | grep ^Version
> Version: 2.3.6.ds1-8
>
>
> diff -ru procmeter3-3.4e.orig/modules/df.c procmeter3-3.4e/modules/df.c
> --- procmeter3-3.4e.orig/modules/df.c 2005-06-14 20:38:33.000000000 +0300
> +++ procmeter3-3.4e/modules/df.c 2006-12-12 10:40:17.000000000 +0200
> @@ -95,7 +95,7 @@
> ProcMeterOutput **Initialise(char *options)
> {
> FILE *f;
> - char line[128];
> + char line[256];
>
> outputs=(ProcMeterOutput**)malloc(sizeof(ProcMeterOutput*));
> outputs[0]=NULL;
> @@ -107,18 +107,18 @@
> fprintf(stderr,"ProcMeter(%s): Could not open
> '/proc/mounts'.\n",__FILE__);
> else
> {
> - if(!fgets(line,128,f))
> + if(!fgets(line,256,f))
> fprintf(stderr,"ProcMeter(%s): Could not read
> '/proc/mounts'.\n",__FILE__);
> else
> do
> {
> - char device[32],mount[32];
> + char device[65],mount[65];
>
> - if(sscanf(line,"%s %s",device,mount)==2)
> + if(sscanf(line,"%64s %64s",device,mount)==2)
> if(strcmp(mount,"none") && *mount=='/' && (*device=='/' ||
> strstr(device, ":/")))
> add_disk(device,mount);
> }
> - while(fgets(line,128,f));
> + while(fgets(line,256,f));
>
> fclose(f);
> }
> @@ -130,21 +130,21 @@
> fprintf(stderr,"ProcMeter(%s): Could not open '/etc/fstab'.\n",__FILE__);
> else
> {
> - if(!fgets(line,128,f))
> + if(!fgets(line,256,f))
> fprintf(stderr,"ProcMeter(%s): Could not read
> '/etc/fstab'.\n",__FILE__);
> else
> do
> {
> - char device[33],mount[33];
> + char device[65],mount[65];
>
> if(*line=='#')
> continue;
>
> - if(sscanf(line,"%32s %32s",device,mount)==2)
> + if(sscanf(line,"%64s %64s",device,mount)==2)
> if(strcmp(mount,"none") && *mount=='/' && (*device=='/' ||
> strstr(device, ":/")))
> add_disk(device,mount);
> }
> - while(fgets(line,128,f));
> + while(fgets(line,256,f));
>
> fclose(f);
> }
> @@ -245,7 +245,7 @@
> if(now!=last)
> {
> FILE *f;
> - char line[128];
> + char line[256];
>
> for(i=0;i<ndisks;i++)
> mounted[i]=0;
> @@ -255,20 +255,20 @@
> return(-1);
> else
> {
> - if(!fgets(line,128,f))
> + if(!fgets(line,256,f))
> return(-1);
> else
> do
> {
> - char device[32],mount[32];
> + char device[65],mount[65];
>
> - if(sscanf(line,"%s %s",device,mount)==2)
> + if(sscanf(line,"%64s %64s",device,mount)==2)
> if(strcmp(mount,"none") && *mount=='/' && (*device=='/' ||
> strstr(device, ":/")))
> for(i=0;i<ndisks;i++)
> if(!strcmp(disk[i],mount))
> mounted[i]=1;
> }
> - while(fgets(line,128,f));
> + while(fgets(line,256,f));
>
> fclose(f);
> }
>
> --
> Timo Lilja
>
> "It's a 106 miles to Chicago. We've got a full tank of gas,
> half a pack of cigarettes, it's dark, and we're wearing sunglasses."
>
--
see shy jo
signature.asc
Description: Digital signature
