[GRASS-dev] d.legend uses a static buffer for `map='

[Ivan Shmakov]

>>>>> Glynn Clements <glynn at gclements.plus.com> writes:

[...]

 >>>> PS.  I've seen a number of other uses of static buffers scattered
 >>>> across the code, leading to both potential limits and crashes,
 >>>> which I'm on the way to investigate.

 >>> There isn't necessarily a problem with using fixed-size buffers for
 >>> map names, but the size should be GNAME_MAX.

 >> Well, yes.  It's actually strcpy () use which bothers me the most,
 >> and not the static buffer per se.  Obviously, `d.legend' may
 >> accidentally get called with a `map' value larger than any
 >> predefined static buffer size, leading to hardly predicable
 >> behaviour.

 > If you're worried about buffer overflows, there are probably
 > somewhere between a few hundred and a few thousand others to worry
 > about.

	Perhaps.

 > To date, the general policy has been to use buffers which are
 > sufficently large for "normal" use and rely upon the user not doing
 > anything abnormal (like using 1000-character map names).

 > If you're writing a "web application" (CGI script) on top of GRASS,
 > validating any user-supplied data is essential.

	I'm not considering web applications as yet.  Besides, there may
	be other sources of ``potentially dangerous'' data.

	My primary concern is that the software shouldn't surprise the
	user by breaking gratuitously.  In particular, I was quite
	surprised when `d.legend' has failed for a particularly long
	raster name.  I had to make a copy for each of the rasters to
	make it work, and it was a hassle.

 >> In my opinion, strncpy () should generally be preferred over strcpy
 >> ().

 > Using strncpy() isn't much of an improvement, as the copy won't be
 > NUL-terminated if it's truncated. And simplyy NUL-terminating the
 > string risks reading/writing the wrong file. The only safe solutions
 > are to either dynamically allocate all buffers, or signal a fatal
 > error if the source is too long.

	Or, if a library function is considered, it may return a kind of
	error to the caller.

	For example, I'd suggest changing D_get_cell_name () as follows:

diff --git a/lib/display/list.c b/lib/display/list.c
index 48180cf..2353092 100644
--- a/lib/display/list.c
+++ b/lib/display/list.c
@@ -99,6 +99,9 @@ int D_get_cell_name(char *name )
 	if ((stat = R_pad_get_item ("cell", &list, &count)))
 		return(-1) ;
 
+	if (strlen (list[0]) >= GNAME_MAX)
+		return -1;
+
 	strcpy(name, list[0]) ;
 
 	R_pad_freelist (list,count) ;

	(Add a warning to your taste.)

	Along with changing all its users to use static buffers of size
	GNAME_MAX.  (I'd try to find them all and propose a patch soon.)

[...]

 >> $ nl -ba display/d.colors/main.c 
 >> ...
 >>     25	int 
 >>     26	main (int argc, char **argv)
 >>     27	{
 >>     28	    char name[128] = "";

 > Right. We encourage developers to replace these with the defined
 > constants as they encounter them.

	ACK.

[...]

 >>> As this never enlarges the string, it's okay to re-use the original
 >>> buffer.

 >> I see, Martin has turned G_find_cell () into G_find_cell2 ()
 >> instead.

	... Though `const' was stripped off the `map_name' declaration
	as well?

 > G_find_cell2() is preferable, but you need to ensure that there isn't
 > any code which relies upon the @mapset having been stripped off (e.g.
 > code which generates additional map names by appending a suffix).

	ACK.

 > A while back, I changed all of the libgis functions which I could
 > find to accept qualified, so leaving the name qualified shouldn't be
 > a problem so far as libgis is concerned, but there may still be
 > problems with other libraries and with the module itself.

	The only uses of `map_name' in d.legend are:

G_read_colors (map_name, mapset, ...)
G_raster_map_is_fp (map_name, mapset)
G_read_cats (map_name, mapset, ...)
G_read_range (map_name, mapset, ...)
G_read_fp_range (map_name, mapset, ...)

	I guess, they're safe?

PS.  Shouldn't G__open () use GPATH_MAX as well?