On Thu, Jul 30, 2009 at 10:33:03AM -0400, Jason A. Smith wrote: > On Thu, 2009-07-30 at 10:29 +0000, Carlo Marcelo Arenas Belon wrote: > > On Wed, Jul 29, 2009 at 03:42:05PM -0400, Jason A. Smith wrote: > > > > > > In gmond, the monitor-core/libmetrics/linux/metrics.c:find_disk_space() > > > function, was not only using small character arrays, but the arrays for > > > the sscanf after the fgets were smaller than the array for the line it > > > just read in, which can lead to buffer overflows and the "stack > > > smashing" problem that we were having. > > > > using fixed size arrays in the stack is never a good idea. > > in this case it could be theoretically possible to exploit this overflow > > with the help of a malicious NFS server (very unlikely though). > > > > > To fix out problem and prevent the overflows, I made a patch to increase > > > the size of the arrays and also make each of the arrays used in the > > > sscanf the same size as the line buffer used in fgets, so there is no > > > chance of another overflow. > > > > committed for trunk in r2007, but the new implementation might also > > generate segfaults on its own due to stack overflows when running with > > very small stacks as it requires a bigger stack. > > > > IMHO it would be better to migrate this function to use getmntent and > > friends as it was done already for Cygwin, Solaris and the BSD and that > > way avoid the use of local buffers and parsing of the /proc/mounts file > > directly. > > This is a good idea, I didn't think to check the libmetric code from the > other OSes. Besides a few minor differences for the remote_mounts & > valid_mount_type functions and the seen_before part, the solaris & linux > code look almost identical.
if I recall correctly, when I added disk metrics for Solaris it was indeed modeled after the linux code except for the following differences (there was a thread started at a time that got nowhere) : * originally using GiB instead of GB (changed since) * whitelist of filesystems instead of a blacklist * using getmntent and friends * avoid use of hashes, pointers or static buffers. > Should the linux code be updated to look > more like the solaris code? It would be probably easier to change the implementation to use getmntent instead of parsing the /proc/mounts file by hand and which might as well help to simplify the code further. > Too bad it isn't possible to merge similar > code instead copying it. there is no reason why we can't (if needed) write the code in 1 single place and link it against the OS specific metric code later (as shown by libmetrics/interface.c), but it is usually just easier to copy the code as it will need to be adjusted to work properly for each supported OS (and different versions of it as well, with different ABI/API) and that way avoid having to obscure the logic with portability constructs which are required otherwise. considering though that getmntent is available in all the OS that are implementing disk metrics AFAIK, this should be probably (as suggested originally) a secure, portable alternative IMHO. Carlo ------------------------------------------------------------------------------ Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july _______________________________________________ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
