[Ady0333]

Problem

r.lake validates the seed coordinate before using it as a cell index:

start_col = (int)Rast_easting_to_col(east, &window);
start_row = (int)Rast_northing_to_row(north, &window);

if (start_row < 0 || start_row > rows || start_col < 0 ||
    start_col > cols)
    G_fatal_error(_("Seed point outside the current region"));

The valid indices are 0 to rows - 1 and 0 to cols - 1, so the comparisons need to be >=. As written, start_row == rows and start_col == cols pass the check.

Those values are reachable from ordinary input. Rast_northing_to_row() returns exactly rows for a coordinate on the southern edge of the region, and Rast_easting_to_col() returns exactly cols on the eastern edge. Neither is clamped afterwards.

The arrays have no spare element. in_terran and out_water hold exactly rows row pointers, and each row holds exactly cols values, so both indices are one past the end:

in_terran = (FCELL **)G_malloc(rows * sizeof(FCELL *));
in_terran[row] = (FCELL *)G_malloc(cols * sizeof(FCELL));
out_water[row] = (FCELL *)G_calloc(cols, sizeof(FCELL));
...
if (in_terran[start_row][start_col] >= water_level)      /* out of bounds read */
    G_fatal_error(...);
out_water[start_row][start_col] = 1;                     /* out of bounds write */

Effect

A seed on the southern edge reads a pointer from past the end of the pointer array and dereferences it, which crashes. A seed on the eastern edge reads past the end of a row and then writes four bytes past the end of another, after which the tool completes with exit code 0 and writes a lake map containing no water at all, with no warning.

AddressSanitizer on the current main branch:

seed on the southern edge (start_row == rows)
  heap-buffer-overflow READ of size 8 at raster/r.lake/main.c:277
  0 bytes after 400-byte region          (50 rows * 8 bytes)

seed on the eastern edge (start_col == cols)
  heap-buffer-overflow READ of size 4 at raster/r.lake/main.c:277
  0 bytes after 200-byte region          (50 cols * 4 bytes)
  heap-buffer-overflow WRITE of size 4 at raster/r.lake/main.c:281

Reproduction

g.region rows=50 cols=50 w=0 e=50 s=0 n=50 res=1
r.mapcalc "terrain = 5"

r.lake elevation=terrain water_level=10 lake=lk coordinates=25.5,25.5   # 2500 cells
r.lake elevation=terrain water_level=10 lake=lk coordinates=25.5,0      # segmentation fault
r.lake elevation=terrain water_level=10 lake=lk coordinates=50,25.5     # exit 0, empty map

The coordinate does not have to be chosen deliberately. g.region vector=<points> sets the region bounds to the bounding box of the vector, so the southernmost and easternmost points sit exactly on the region edge:

g.region vector=pts
r.lake elevation=terrain water_level=10 lake=lk coordinates=<southernmost point>
# segmentation fault

Fix

Use >= so the check covers the valid index range. Seeds on the edge then take the existing error path, which is what the check was written to do.

Tests

Three cases covering the southern edge, the eastern edge and the corner, asserting the tool exits with the regular error rather than a signal, plus one case for a seed in the last row to confirm valid coordinates are still accepted. The three edge cases fail on main and pass with the fix.

r.cost and r.walk already use the >= form for the same check. r.path and r.drain do not, but they behave differently and are left for a separate change.

[Ady0333]

/cc @echoix

[echoix]

I’m not sure. Also, why would only one side need a change?

[echoix]

Are you able to see if the other bounds (north or west) would also have a problem?

Details

Review of PR 7877 (OSGeo/grass, r.lake: Reject seed coordinates on the region edge).

The core fix is correct: Rast_northing_to_row() returns exactly rows on the southern edge and Rast_easting_to_col() returns cols on the eastern edge (both are plain unclamped divisions in lib/raster/window.c:201), while in_terran/out_water are allocated with exactly rows pointers and cols values, so >= is the right comparison. Tests check out: ToolError is exported lazily from grass.tools, .errors/.returncode are real attributes (errors set in CalledModuleError.__init__), stderr is captured by default (capture_output=True), and r.univar --format=json emits a top-level object (not an array) when no zones are used, so stats["n"] is valid. The flat-terrain fixture and the n == 2500 expectation are consistent with the 50x50 region.

One finding:

• raster/r.lake/main.c:241 — The negative half of the same check is still ineffective: for a seed under one cell north or west of the region, Rast_northing_to_row() returns e.g. -0.5 and the (int) cast truncates to 0, so start_row < 0 never fires and the seed is silently snapped to row/col 0 instead of erroring (e.g. coordinates=25.5,50.5 with n=50 res=1). Pre-existing, but it is the very expression this PR modifies.

[Ady0333]

yeahhh, i checked. North and west are fine because a coordinate on those edges maps to row 0 / col 0, which is already inside the region. South and east map to rows and cols, which is one past the end, so that's why only that side broke.

But they do have a smaller issue. The (int) cast truncates towards zero, so anything between -1 and 0 becomes 0 and start_row < 0 never catches it. On a 50x50 region a seed half a cell north of the region gets silently accepted as row 0 instead of being rejected. Same on the west side.

using floor() on the two conversions fixes that, and the existing tests still pass.
want me to add it here, or keep this PR to the out of bounds fix and open a separate one???

[echoix]

Open another one, someone else will review only that part. But in that PR, can you add a test that fails without the fix?

[Ady0333]

yes i can