[GRASS5] [bug #2767] (grass) r.stats bug (due to recent G_store() fix?)

GRASS GRASS developer
1

I'm not sure this line:
if (title == NULL) title="";
looks right.
Perhaps try changing to
if (title == NULL) *title="";
Makes more logical sense but still might not work.

If it doesn't something like this might:
int
G_set_raster_cats_title (char *intitle, struct Categories *pcats)
{
    char *title;
    if (intitle == NULL)
      *title="";
    else
      title=intitle;
    pcats->title = G_store (title);
    G_newlines_to_spaces (pcats->title);
    G_strip (pcats->title);
      return 0;
}

-------------------------------------------- Managed by Request Tracker

2

Hello Paul,

On Tue, Dec 07, 2004 at 10:21:08PM +0100, Paul Kelly via RT wrote:

I'm not sure this line:
if (title == NULL) title="";
looks right.
Perhaps try changing to
if (title == NULL) *title="";
Makes more logical sense but still might not work.

I tried, but still segfault.

If it doesn't something like this might:
int
G_set_raster_cats_title (char *intitle, struct Categories *pcats)
{
    char *title;
    if (intitle == NULL)
      *title="";
    else
      title=intitle;
    pcats->title = G_store (title);
    G_newlines_to_spaces (pcats->title);
    G_strip (pcats->title);
      return 0;
}

gcc reports
cats.c:1539: warning: assignment makes integer from pointer without a cast
here (l 1539 is: *title="";).
Also tried and segfault.

Thanks and sorry for no better news,

Markus

3

Hello,

I haven't look at the code you are trying to fix (I have some other
stuff ;-), but here are some tips :

On Tue, Dec 07, 2004 at 10:27:14PM +0100, Markus Neteler wrote:

> If it doesn't something like this might:
> int
> G_set_raster_cats_title (char *intitle, struct Categories *pcats)
> {
> char *title;
> if (intitle == NULL)
> *title="";
> else
> title=intitle;
> pcats->title = G_store (title);
> G_newlines_to_spaces (pcats->title);
> G_strip (pcats->title);
> return 0;
> }

title is declared as a char*
title is supposed to hold a value char*, but is current value after the
declaration in the local scope of the function is random (writing to
this destination will cause a mess).

The assignement :

title = "";

is syntactically correct since one affects to a char * a char * ("" is
indeed a pointer to a string reduced to one char '\0').
But here title is initialized
with the memory address of the "" string, which is whether put by the
compiler
in read-only memory (since "" is not a variable and doesn't change) or
may be allocated on the stack by some instructions generated by the
compiler (depending on optimization), thus making title points to a
read-only address or an evanescent address (on the stack).
Trying to change the value of the char pointed to by the address of
"", hence the address of title (the char '\0') afterwards is likely to
cause problem.

The assignement :
*title = "";

is syntactically and logically wrong : one is assigning a pointer ("" is
a pointer to the empty string) to an integer value (a char, since *title
is a char because title is a pointer to char). Furthermore, since title
is not initialized, one is trying to write something in a random address
(that may be initialized to NULL, but this is not guaranteed for
automatic variables at all).

So a correct sequence is :

title = (char *) malloc(expected_size_of_string_plus_trailing_null);
*title = '\0'; /* default to empty if intitle not set */

Cheers,
--
Thierry Laronde (Alceste) <tlaronde +AT+ polynum +dot+ com>
http://www.kergis.org/ | http://www.kergis.com/
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

4

Hello Thierry

On Tue, 7 Dec 2004 tlaronde@polynum.com wrote:

Hello,

I haven't look at the code you are trying to fix (I have some other
stuff ;-), but here are some tips :

[...]
Snip explanation of why my suggested fix was completely wrong
[...]

So a correct sequence is :

title = (char *) malloc(expected_size_of_string_plus_trailing_null);
*title = '\0'; /* default to empty if intitle not set */

The first line here is more or less what G_store() does. But the recent change added
if ( s == NULL ) return NULL ;
to G_store.

Perhaps (going by what you suggest above) it should look more like:
char *G_store (char *s)
{
   char *buf;

   if ( s == NULL )
     *buf = '\0';
   else
   {
     buf = G_malloc (strlen(s) + 1);
     strcpy (buf, s);
   }
   return buf;
}

so you are returning a pointer to an empty string rather than returning a NULL pointer. But I am still not very hopeful.

The funny thing is I am unable to reproduce this bug so cannot test. Running 'r.stats -anC fields' in Spearfish with yesterday's 5.7 CVS on IRIX produces normal-looking output.

Paul

5

Hello Paul,

On Wed, Dec 08, 2004 at 08:51:04PM +0000, Paul Kelly wrote:

Hello Thierry

[snipped]

The first line here is more or less what G_store() does. But the recent
change added
if ( s == NULL ) return NULL ;
to G_store.

Perhaps (going by what you suggest above) it should look more like:
char *G_store (char *s)
{
  char *buf;

  if ( s == NULL )
    *buf = '\0';

You have to allocate place for buf to point to even if you only want to
store '\0'. Here buf has some random value. If, on some systems, even
the automatic variables are initialized to zero, or if it happens that
the value of buf is 0, the program will core dump (SEGFAULT). If the
(random since not initialized) value of buf happens to be a valid
address pointing to some free space or not used values, you will be able
to store the '\0' without segfaulting, but for a long run program, it
will probably happen at some moment that another piece of code use
willingly this place (that has not be reserved by *alloc() ) and stores
something else (no more '\0' and you will generate a random output).

so here too :

if (s == NULL)
  buf = (char *) calloc(1,1); /* alloc 1 byte and zeroes */

but may I suggest that the purpose of G_store() is not to transform a
NULL in a zero length string and that probably, depending on the bug you
are trying to fix, it will be better to keep G_store in its present
state and use for example a test in the caller (if G_store returns NULL,
print an empty string or whatever---I haven't follow the history of the
problem so I don't know the context).

[..]
The funny thing is I am unable to reproduce this bug so cannot test.
Running 'r.stats -anC fields' in Spearfish with yesterday's 5.7 CVS on IRIX
produces normal-looking output.

Precisely due to the random value of buf, which depends on the
environment and the compiler.

Cheers,
--
Thierry Laronde (Alceste) <tlaronde +AT+ polynum +dot+ com>
http://www.kergis.org/ | http://www.kergis.com/
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

6

Paul Kelly via RT wrote:

I'm not sure this line:
if (title == NULL) title="";
looks right.

Looks fine to me. The variable "title" will point to read-only data,
so it can only be read, not written, but that isn't an issue here.

Perhaps try changing to
if (title == NULL) *title="";

That's a type error, but it's also guaranteed to segfault (writing to
address zero).

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