[GRASS-dev] Re: [bug #1140] (grass) Non-portable snprintf() used in several programs

GRASS GIS GRASS GIS developer
1

On Tue, 4 Jul 2006, Markus Neteler via RT wrote:

the snprintf() is used as of today in:

./raster3d/r3.in.ascii/main.c
./db/drivers/dbf/dbfexe.c
./raster/r.support/front/front.c
./raster/r.support/front/check.c
./raster/r.support/front/run.c
./raster/r.support/modhead/check_un.c
./raster/r.support/modhead/modhead.c
./raster/r.support/modhead/ask_format.c
./lib/init/clean_temp.c
./lib/db/dbmi_client/select.c
./lib/gis/user_config.c
./lib/vector/dglib/examples/components.c

Please see attached patch to replace use of snprintf in all those modules. I am posting it to the list first, hopefully for some comments, before committing. Have to admit what I was doing felt a bit pedantic towards the end. But some comments:

Mostly what I have done is replaced snprintf writing into fixed size buffers by dynamically allocating space for the buffers based on the length of the strings that were being copied in. Where a formatted number was going in I allowed 32 characters to be on the safe side.

In all but one example (./lib/init/clean_temp.c) snprintf() was being used without a check on the return value. So while a buffer overflow may have been avoided, if the string was truncated it would lead to unpredictable results later in the program. This won't happen now.

Also in another case (./lib/gis/user_config.c) snprintf() was used with a buffer that had already been correctly dynamically sized, so it wasn't even needed there.

Paul

(attachments)

diff.txt (16 KB)

2

Paul Kelly wrote:

> the snprintf() is used as of today in:
>
> ./raster3d/r3.in.ascii/main.c
> ./db/drivers/dbf/dbfexe.c
> ./raster/r.support/front/front.c
> ./raster/r.support/front/check.c
> ./raster/r.support/front/run.c
> ./raster/r.support/modhead/check_un.c
> ./raster/r.support/modhead/modhead.c
> ./raster/r.support/modhead/ask_format.c
> ./lib/init/clean_temp.c
> ./lib/db/dbmi_client/select.c
> ./lib/gis/user_config.c
> ./lib/vector/dglib/examples/components.c

Please see attached patch to replace use of snprintf in all those
modules. I am posting it to the list first, hopefully for some comments,
before committing.

In cases where the strings being inserted are themselves supposed to
have a maximum length (e.g. map names), I would just check that the
strings don't exceed their maximum length and use a fixed-size buffer.

Index: lib/init/clean_temp.c

RCS file: /grassrepository/grass6/lib/init/clean_temp.c,v
retrieving revision 2.5
diff -u -r2.5 clean_temp.c
--- lib/init/clean_temp.c 24 Jun 2006 19:33:03 -0000 2.5
+++ lib/init/clean_temp.c 5 Jul 2006 11:02:49 -0000
@@ -19,7 +19,6 @@
  *
  * 2006: Rewritten for GRASS 6 by roberto Flor, ITC-irst
  *
- * TODO (?): Implement snprintf() as G_snprintf() for portability
  **************************************************************/

#include <limits.h>
@@ -39,7 +38,6 @@

void clean_dir(const char *pathname,uid_t uid,pid_t pid,time_t now,int max_age)
{
- char buf[BUF_MAX];
     DIR *curdir;
     struct dirent *cur_entry;
     struct stat info;
@@ -53,11 +51,16 @@
     /* loop over current dir */
     while ((cur_entry = readdir(curdir)))
     {
- if ((G_strcasecmp(cur_entry->d_name,".") == 0 )|| (G_strcasecmp(cur_entry->d_name,"..")==0))
+ static char *buf = NULL;
+
+ if ((G_strcasecmp(cur_entry->d_name,".") == 0 )|| (G_strcasecmp(cur_entry->d_name,"..")==0))
     continue; /* Skip dir and parent dir entries */
-
- if ( (pathlen=snprintf(buf,BUF_MAX,"%s/%s",pathname,cur_entry->d_name)) >= BUF_MAX)
- G_fatal_error("clean_temp: exceeded maximum pathname length %d, got %d, should'nt happen",BUF_MAX,pathlen);
+
+ if(buf)
+ G_free(buf);
+
+ buf = G_malloc(strlen(pathname) + strlen(cur_entry->d_name) + 2);
+ sprintf(buf, "%s/%s", pathname, cur_entry->d_name);

In this situation, I would start with a reasonably-sized buffer (e.g.
4Kb), then use G_realloc() to enlarge it if necessary.

Both malloc() and free() can be quite computationally expensive, and
repeated use can cause heap fragmentation, particularly if the size
tends to increase with time (as the previously free()d block is too
small to be used by the subsequent malloc()).

The same applies to any kind of loop: re-using a buffer is more
efficient (in terms of both CPU and memory usage) than allocating a
new buffer every time.

--
Glynn Clements <glynn@gclements.plus.com>