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

GRASS GIS GRASS GIS developer
1

this bug's URL: http://intevation.de/rt/webrt?serial_num=4987
-------------------------------------------------------------------------

Subject: malloc errors/declaration - 64 bit problems

grass obtained from: CVS
grass binary for platform: Compiled from Sources

Copied to bugtracker by MN.
-------

On Tue, Aug 08, 2006 at 04:10:04PM +0200, Roberto Flor wrote:
Here my findings on 64 bit problems:

Simple, potential error on 64 bit machines, really only for float case:

./raster/simwe/r.sim.sediment/main.c: si = (double **)G_malloc (sizeof(double)*(my));
./raster/simwe/r.sim.sediment/main.c: sigma = (double **)G_malloc (sizeof(double)*(my));
./raster/simwe/r.sim.sediment/main.c: dif = (float **)G_malloc (sizeof(float)*(my));
./raster/simwe/r.sim.sediment/main.c: er = (float **)G_malloc (sizeof(float)*(my));
./vector/v.surf.idw/main.c: npoints_currcell = (long **)G_malloc(window.rows * sizeof(long));

Overzealous but should'nt be a problem:

./raster/r.param.scale/nrutil.c: t = (float ***) G_malloc (((nrow+NR_END) * sizeof(float**)));
./imagery/i.smap/bouman/multialloc.c: r[0]=(char *)G_malloc(max*sizeof(char **));
./lib/imagery/alloc.c: x = (double ***)G_malloc((a+1) * sizeof(double **));

Hyper allocation, requires more memory than necessary:

./vector/v.surf.idw/main.c: search_list = (struct cell_list **)G_malloc(max_radius * sizeof(struct cell_list));
./vector/v.surf.idw/main.c: search_list_start = (struct cell_list **)G_malloc(max_radius * sizeof(struct cell_list));
./vector/v.surf.idw/main.c: points = (struct Point ***)G_malloc(window.rows * sizeof(struct Point));
./vector/v.surf.idw/main.c: points[row] = (struct Point **)G_malloc(window.cols * sizeof(struct Point));

In all the case the sizeof shoul to be of a pointer.

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

2

On Tue, 8 Aug 2006, Request Tracker wrote:

On Tue, Aug 08, 2006 at 04:10:04PM +0200, Roberto Flor wrote:
Here my findings on 64 bit problems:

Simple, potential error on 64 bit machines, really only for float case:

[...]

./vector/v.surf.idw/main.c: npoints_currcell = (long **)G_malloc(window.rows * sizeof(long));

[...]

Hyper allocation, requires more memory than necessary:

./vector/v.surf.idw/main.c: search_list = (struct cell_list **)G_malloc(max_radius * sizeof(struct cell_list));
./vector/v.surf.idw/main.c: search_list_start = (struct cell_list **)G_malloc(max_radius * sizeof(struct cell_list));
./vector/v.surf.idw/main.c: points = (struct Point ***)G_malloc(window.rows * sizeof(struct Point));
./vector/v.surf.idw/main.c: points[row] = (struct Point **)G_malloc(window.cols * sizeof(struct Point));

In all the case the sizeof shoul to be of a pointer.

The v.surf.idw are all my mistakes; well spotted and thanks for pointing them out. To fix them, I guess the size of a pointer is always the same no matter what type it points to, so is there a standard way of representing it always, e.g. like sizeof(char *)? Or should we always write sizeof(long *),
sizeof(struct Point *) etc. as appropriate? Is it just a question of
programming style?

Paul

3

Request Tracker wrote:

this bug's URL: http://intevation.de/rt/webrt?serial_num=4987

Subject: malloc errors/declaration - 64 bit problems

grass obtained from: CVS
grass binary for platform: Compiled from Sources

Copied to bugtracker by MN.
-------

On Tue, Aug 08, 2006 at 04:10:04PM +0200, Roberto Flor wrote:
Here my findings on 64 bit problems:

Simple, potential error on 64 bit machines, really only for float case:

./raster/simwe/r.sim.sediment/main.c: si = (double **)G_malloc (sizeof(double)*(my));
./raster/simwe/r.sim.sediment/main.c: sigma = (double **)G_malloc (sizeof(double)*(my));
./raster/simwe/r.sim.sediment/main.c: dif = (float **)G_malloc (sizeof(float)*(my));
./raster/simwe/r.sim.sediment/main.c: er = (float **)G_malloc (sizeof(float)*(my));
./vector/v.surf.idw/main.c: npoints_currcell = (long **)G_malloc(window.rows * sizeof(long));

./vector/v.surf.idw/main.c: search_list = (struct cell_list **)G_malloc(max_radius * sizeof(struct cell_list));
./vector/v.surf.idw/main.c: search_list_start = (struct cell_list **)G_malloc(max_radius * sizeof(struct cell_list));
./vector/v.surf.idw/main.c: points = (struct Point ***)G_malloc(window.rows * sizeof(struct Point));
./vector/v.surf.idw/main.c: points[row] = (struct Point **)G_malloc(window.cols * sizeof(struct Point));

In all the case the sizeof shoul to be of a pointer.

I've fixed these in CVS.

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

4

On Tue, Aug 08, 2006 at 06:43:12PM +0100, Glynn Clements wrote:

Request Tracker wrote:
> this bug's URL: http://intevation.de/rt/webrt?serial_num=4987
> Subject: malloc errors/declaration - 64 bit problems

The modified regex from Roberto shows numerous further
candidates:

egrep -R 'sizeof[ \t]*\([ \t]*[a-Z]+[ \t]*\*' . |grep [ch]: | grep -v void

It would be good to fix at least the dangerous ones.

Markus

5

Hi,

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?

Maris.

On Thursday 10 August 2006 16:54, Markus Neteler wrote:

On Tue, Aug 08, 2006 at 06:43:12PM +0100, Glynn Clements wrote:
> Request Tracker wrote:
> > this bug's URL: http://intevation.de/rt/webrt?serial_num=4987
> > Subject: malloc errors/declaration - 64 bit problems

The modified regex from Roberto shows numerous further
candidates:

egrep -R 'sizeof[ \t]*\([ \t]*[a-Z]+[ \t]*\*' . |grep [ch]: | grep -v void

It would be good to fix at least the dangerous ones.

Markus

_______________________________________________
grass-dev mailing list
grass-dev@grass.itc.it
http://grass.itc.it/mailman/listinfo/grass-dev

6

Hi,

On Thursday 10 August 2006 21:52, Māris Nartišs wrote:

Hi,

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 *));
     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));

         /* In newly created space load data from file.*/
         if (G_get_f_raster_row(in_terran_fd, in_terran[row], row)!=1)

Best regards
Soeren

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

Maris.

On Thursday 10 August 2006 16:54, Markus Neteler wrote:
> On Tue, Aug 08, 2006 at 06:43:12PM +0100, Glynn Clements wrote:
> > Request Tracker wrote:
> > > this bug's URL: http://intevation.de/rt/webrt?serial_num=4987
> > > Subject: malloc errors/declaration - 64 bit problems
>
> The modified regex from Roberto shows numerous further
> candidates:
>
> egrep -R 'sizeof[ \t]*\([ \t]*[a-Z]+[ \t]*\*' . |grep [ch]: | grep -v void
>
> It would be good to fix at least the dangerous ones.
>
> Markus
>
> _______________________________________________
> grass-dev mailing list
> grass-dev@grass.itc.it
> http://grass.itc.it/mailman/listinfo/grass-dev

_______________________________________________
grass-dev mailing list
grass-dev@grass.itc.it
http://grass.itc.it/mailman/listinfo/grass-dev

7

Me-Dàris Nartie-B¹s wrote:e-A

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?

That one is correct. The type passed to sizeof() should have one less
"*" than the type of the data being allocated. E.g. in_terran has type
FCELL** so the allocation multiple should be FCELL*, which is what is
used.

However, further down in that file, we have:

        in_terran[row] = G_malloc(cols * sizeof(FCELL *));

in_terran is FCELL**, so in_terran[row] is FCELL*, so the allocation
multiple should be just FCELL. Ditto for out_water.

There's no way that this kind of error can reliably be detected by a
regex. The correct type depends upon the type of the lvalue to which
the result is assigned, and that can't be determined by a regex.

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

8

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@gclements.plus.com>

9

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