r.in.pdal: fix integer overflow

[nilason]

Fixes #2840

The segmentation fault reported with #2840 is caused by a call to Rast_set_null_value() where the parameter numVals is defined as integer. Overflow occurs if the col * row value given to numVals exceeds INT_MAX.

I put this up initially as a draft as this brings up some questions:

Firstly:
The array is allocated as rows * (cols + 1):

grass/raster/r.in.pdal/point_binning.c

Lines 251 to 252 in b06c460

	point_binning->index_array =
	G_calloc((size_t)rows * (cols + 1), Rast_cell_size(CELL_TYPE));

but zeroed as rows * cols:

grass/raster/r.in.pdal/point_binning.c

Line 60 in b06c460

Rast_set_null_value(array, nrows * ncols, map_type);

Is this really by design?

Secondly:

The question is whether Rast_set_null_value() family of functions should all be changed accordingly and to what type. I have now changed numVals to unsigned int, but this could be size_t, unsigned long ...
Or do we re-implement this function locally within r.in.pdal and leave lib/raster as is?

[metzm]

size_t on 64 bit systems is larger than unsigned int, this will not work for larger regions.

I would rather loop over the rows and set each row to NULL. A pointer can be used to go over the rows, increased with ptr = G_incr_void_ptr(ptr, ncols * map_type_size):

        for (row = 0; row < nrows; row++) {
            Rast_set_null_value(ptr, ncols, map_type);
            ptr = G_incr_void_ptr(ptr, ncols * Rast_cell_size(map_type));
        }

[wenzeslaus]

While "set whole raster map to null" seems like a good function to have, I'm afraid that the correct way of using Rast_set_null_value is to set only up a row of value as @metzm says.

[nilason]

With the change @metzm suggested, I guess there is no need to change (at this time) Rast_set_null_value()?

[wenzeslaus]

Oh, yes, that's what I meant. "Set whole raster map to null" would be a separate function wrapping that code.

[metzm]

"Set whole raster map to null" would be a rather special function because typically raster data are accessed by rows and not as a whole. Standard procedure would be e.g. to set the next output row to NULL, do some calculations with input data, set data in the output row where applicable, write output row, continue with next row.

In order to save memory consumption, raster operations should wherever possible never load a whole raster to memory but instead proceed row by row or a small chunk of rows (e.g. r.neighbors). This is IMHO a big strength of GRASS, also for cloud and grid computing.

[wenzeslaus]

Right, perhaps too specific to the binning procedure here. This reminds me that "whole map" here is actually a chunk of rows.

[nilason]

Just pushed the suggested changes. Please review!

[wenzeslaus]

...The array is allocated as rows * (cols + 1)...but zeroed as rows * cols... Is this really by design?

The history of the code is that I took the point binning code from the original r.in.lidar which likely took it from r.in.xyz. While I was trying to improve the API when working on r.in.lidar, I think I assumed the code to be correct and extra allocated space would pass any automated checks. So, from this point of view, it is likely not intentional. By quick look into the code I don't see anything either, but perhaps a second look would be good.

Both "null whole raster map" and point/value binning would be good to have in the library, but that's a different story.

[nilason]

Thank you both for your input and in particular for the brilliant solution @metzm !

Shall this be backported?

[neteler]

Shall this be backported?

Yes please.

[petrasovaa]

There is identical code in r.in.xyz as Vashek points out:

https://github.com/OSGeo/grass/blob/main/raster/r.in.xyz/support.c#L48

[nilason]

There is identical code in r.in.xyz as Vashek points out:

https://github.com/OSGeo/grass/blob/main/raster/r.in.xyz/support.c#L48

Thanks! I'll get to it.

[nilason]

There is identical code in r.in.xyz as Vashek points out:
https://github.com/OSGeo/grass/blob/main/raster/r.in.xyz/support.c#L48

Thanks! I'll get to it.

Done with #2847

[metzm]

There are a number of modules with shared code base such as r.in.xyz and r.in.pdal. In order to ease maintenance, it would be great to add some comment in the code for developers at a prominent place to keep shared code in sync. I tried to do this e.g. for the GDAL/OGR import modules r.in.gdal, r.external, v.in.ogr, v.external.

[nilason]

There are a number of modules with shared code base such as r.in.xyz and r.in.pdal. In order to ease maintenance, it would be great to add some comment in the code for developers at a prominent place to keep shared code in sync. I tried to do this e.g. for the GDAL/OGR import modules r.in.gdal, r.external, v.in.ogr, v.external.

Good point!

[wenzeslaus]

In case of r.in.xyz and r.in.pdal, an actual library might be more appropriate. However, as a simple fix, we now have also enforcement of duplicate files to be the same: 8b6cd94

[nilason]

In case of r.in.xyz and r.in.pdal, an actual library might be more appropriate. However, as a simple fix, we now have also enforcement of duplicate files to be the same: 8b6cd94

They are not duplicate files, r.in.pdal is, as far as I can tell, heavily refactored too. Library functions for repetitive code sounds better, if practical.