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

[Glynn Clements]

Ivan Shmakov wrote:

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

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

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.

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

>  > 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] = "";

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

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

Yes.

>  > #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?

Not really. It's only a problem if a path longer than that actually
occurs. 4096 characters is over 50 lines of 80 columns each. Who
actually has pathnames that long?

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

Yes.

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

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

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.

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