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

[Ivan Shmakov]

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

 >> As of a recent SVN HEAD, `d.legend' uses a static buffer to store
 >> the value of the `map' option.  It's therefore impossible to pass
 >> raster names more than 63 bytes long to `d.legend'.  Since I don't
 >> see why a static buffer may be necessary here, I suggest the
 >> following (yet untested) patch.

[...]

 >> 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.

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

 > Similar constants exist for mapset names and pathnames:

 > /* File/directory name lengths */
 > #define GNAME_MAX 256
 > #define GMAPSET_MAX 256

	ACK, thanks for the info.

	What I'm talking about actually is the use of strcpy () in
	lib/display/list.c.  Apparently, the static buffers passed to,
	e. g., D_get_cell_name () have the sizes that don't match any of
	the constants above.  Consider, e. g.:

$ nl -ba display/d.colors/main.c 
...
    25	int 
    26	main (int argc, char **argv)
    27	{
    28	    char name[128] = "";
...
    43	    if (R_open_driver() == 0)
    44	    {
    45	        if(D_get_cell_name (name) < 0)
    46		    *name = 0;
    47	        R_close_driver();
    48	    }
...
$ 

	Doesn't the above mean that `d.colors' cannot handle rasters
	with names more than 127 bytes long?

 > #define GPATH_MAX 4096

	... Though any such limits give me a hard feeling in general.
	Consider, e. g., GNU/Hurd system, which has no limits on the
	path length.  Wouldn't the above involve extra effort to the
	porters?

 > In the specific case of d.legend, your fix is the correct one,
 > except that map_name needs to be "char *" not "const char *", as
 > G_find_cell() may modify the map name (it removes any @mapset
 > suffix).

	ACK.  I've suspected the issues like this one.  I guess, would I
	make a test build of GRASS, the compiler would issue a warning.

 > 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.