[GRASS-dev] [bug #4987] (grass) malloc errors/declaration - 64 bit problems

[Glynn Clements]

Soeren Gebbert wrote:

> > my C skills are too poor to understand it.
> > 
> > Modified regex poped up also
> > ./raster/r.lake/main.c:    in_terran = G_malloc(rows * sizeof(FCELL *));
> > 
> > So - in brief- how it should look to be correct and safe?
> 
> I think the memory allocation code is wrong.
> This is a diff how it should be:
> 
> Index: main.c
> ===================================================================
> RCS file: /home/grass/grassrepository/grass6/raster/r.lake/main.c,v
> retrieving revision 1.2
> diff -u -r1.2 main.c
> --- main.c      13 Jun 2006 22:42:27 -0000      1.2
> +++ main.c      10 Aug 2006 20:31:36 -0000
> @@ -275,8 +275,8 @@
>      }
> 
>      /* Pointers to rows. Row = ptr to 'col' size array. */
> -    in_terran = G_malloc(rows * sizeof(FCELL *));
> -    out_water = G_malloc(rows * sizeof(FCELL *));
> +    in_terran = (FCELL **)G_malloc(rows * sizeof(FCELL *));
> +    out_water = (FCELL **)G_malloc(rows * sizeof(FCELL *));

It isn't necessary to cast the result. G_malloc() returns void* which
can (legitimately) be implicitly cast to any pointer type.

Whether or not an explicit cast is desirable is open to debate. 
Ordinarily, I would consider it as unnecessary verbosity, and omit it. 

However, as this particular error appears to be part of a trend within
GRASS, an explicit cast would help to identify such errors, as any
violation of the "one less *" maxim should be visually obvious (and
explicitly casting to the wrong type would generate a compiler
warning).

>      if (in_terran == NULL || out_water == NULL)
>           G_fatal_error(_("Failure to allocate memory for row pointers"));
> 
> @@ -285,8 +285,8 @@
>      /* foo_rows[row] == array with data (2d array). */
>      for (row = 0; row < rows; row++)
>      {
> -        in_terran[row] = G_malloc(cols * sizeof(FCELL *));
> -        out_water[row] = G_calloc(sizeof(FCELL *), cols);
> +        in_terran[row] = (FCELL *)G_malloc(cols * sizeof(FCELL));
> +        out_water[row] = (FCELL *)G_calloc(cols, sizeof(FCELL));

Here, the change in the type passed to sizeof() fixes an actual bug
(although, in practice, the only consequence will be to allocate too
much memory on 64-bit systems).

The more serious error of allocating insufficient memory occurs when
code either:

a) uses DCELL* when it should use DCELL (bug on 32-bit systems), or
b) uses FCELL when it should use FCELL* (bug on 64-bit systems).

> btw,:
> I can submit the changes if you want.

Please do.

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