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

[Soeren Gebbert]

On Friday 11 August 2006 00:02, Glynn Clements wrote:
> 
> 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).

Indeed. I just found some memory allocation erros by simply
searching for G_calloc and watching the pointer casts.

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

Done.

Soeren

>