Glynn Clements <glynn@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?
