Skip to content

lib/raster3d/tilewrite: Use >= for tile index bounds check#7723

Open
echoix wants to merge 3 commits into
mainfrom
ai-findings-autofix/lib-raster3d-tilewrite.c
Open

lib/raster3d/tilewrite: Use >= for tile index bounds check#7723
echoix wants to merge 3 commits into
mainfrom
ai-findings-autofix/lib-raster3d-tilewrite.c

Conversation

@echoix

@echoix echoix commented Jul 15, 2026

Copy link
Copy Markdown
Member

The bounds check uses > instead of >= for the upper bound. A tileIndex equal to map->nTiles is out of range (valid indices are 0 to map->nTiles - 1), so this condition should use >= map->nTiles to correctly reject that value.

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@echoix
echoix marked this pull request as draft July 15, 2026 05:14
@github-actions github-actions Bot added C Related code is in C libraries raster3d labels Jul 15, 2026
Add a pytest test covering the tileIndex bounds check in
Rast3d_write_tile: an index equal to nTiles must be rejected as out of
range, and the last valid index must be accepted. The invalid call runs
in a subprocess using the grass.lib.raster3d ctypes bindings because the
rejection is a fatal error which exits the calling process.

The out-of-range test fails without the bounds check change in this
branch and passes with it.

The test was written by Claude Code using the Claude Fable 5 model.
@wenzeslaus

Copy link
Copy Markdown
Member

Here is the Claude Code with Fable 5 review you asked for (I'm also adding a test):

Review of PR #7723 — lib/raster3d/tilewrite: Use >= for tile index bounds check

Overview

A one-line fix in Rast3d_write_tile() (lib/raster3d/tilewrite.c:135) changing the upper-bound check on tileIndex from > map->nTiles to >= map->nTiles. Valid tile indices are 0 to nTiles - 1, so the old check let exactly one out-of-range value (tileIndex == nTiles) slip through.

Correctness — verified, the fix is right

  • The old check permits real out-of-bounds access. map->index and map->tileLength are allocated with exactly nTiles elements (lib/raster3d/index.c:165-166). With tileIndex == nTiles, the function would read map->index[nTiles] (tilewrite.c:139), then write it (tilewrite.c:143) and write map->tileLength[nTiles] (tilewrite.c:178) — a one-element heap overflow past both arrays.
  • This is a regression fix, not a new behavior. Git history shows the check was originally >= and was accidentally flipped to > in 2013 by commit 84a06c3, whose stated purpose was removing LRE compression — the change to this line was unrelated collateral in that large commit. The PR restores the original check.
  • It aligns with every sibling check in the library, all of which use the >= convention:
    • lib/raster3d/tileread.c:154(tileIndex >= map->nTiles) || (tileIndex < 0)
    • lib/raster3d/tileio.c:82 — same
    • lib/raster3d/tilemath.c:198Rast3d_tile_index_in_range() returns (tileIndex < map->nTiles) && (tileIndex >= 0)
  • No caller relies on the old behavior. Internal callers derive indices from tile coordinates via Rast3d_tile2tile_index(), which yields in-range values for valid coordinates, so core code doesn't currently hit the gap. The check guards the public API (Rast3d_write_tile is exported and documented), where an addon or external caller could pass any index — the fix makes the guard actually cover the one value that overflows by a single element.

Code quality and conventions

  • The diff is minimal and correctly scoped; the error path (Rast3d_fatal_error) is unchanged and consistent with the read-side counterpart.
  • PR title works as the commit message: the lib/raster3d/tilewrite: prefix matches the Libraries category regexp in utils/release.yml ((grass_|lib)[^ ]*: ), capitalization and imperative mood are correct.
  • The PR description accurately states the problem — no overclaiming.

Optional suggestion (not blocking)

  • The library already provides Rast3d_tile_index_in_range(map, tileIndex) for exactly this test, so the check could be if (!Rast3d_tile_index_in_range(map, tileIndex)). However, tileread.c and tileio.c hand-roll the same condition, so the hand-rolled form is the prevailing idiom here; keeping the diff minimal for a targeted regression fix is a defensible choice. If consolidating, doing all three call sites in a follow-up would be cleaner than doing one here.

Test coverage

  • No test added. That's acceptable here: triggering the guard calls Rast3d_fatal_error(), which exits the process, and the repository has no death-test harness for C library code. The invalid index is also unreachable through the tools' Python-facing surface without deliberately misusing the C API.

Risks

  • Essentially none. The only behavior change is that tileIndex == nTiles now fails fast with a clear fatal error instead of silently corrupting heap memory. No performance implications.

Verdict: approve. A correct, well-scoped fix that restores a bounds check accidentally weakened in 2013 and brings tilewrite.c in line with the rest of the raster3d library.

Added test

Commit 4693358 adds lib/raster3d/tests/lib_raster3d_tilewrite_test.py. Since Rast3d_fatal_error() exits the calling process, the test runs the call in a subprocess: a small script uses the grass.lib.raster3d ctypes bindings to open a new 3D raster with Rast3d_open_cell_new() in non-cache mode, reads nTiles and tileSize from the RASTER3D_Map struct, and calls Rast3d_write_tile() with a chosen tile index.

  • test_write_tile_rejects_index_past_end passes tileIndex == nTiles and asserts a non-zero exit with the "tileIndex out of range" message. Without the fix in this PR, the invalid write is silently accepted (exit code 0) and the test fails; with the fix it passes.
  • test_write_tile_accepts_last_index writes the last valid index (nTiles - 1) as a positive control, so the bounds test cannot pass for an unrelated reason.

Verified against a local Autotools build: without the change, the bounds test fails and the control passes; after applying the change and rebuilding lib/raster3d, both tests pass.

@echoix
echoix marked this pull request as ready for review July 17, 2026 17:13
Comment on lines +11 to +53
WRITE_TILE_SCRIPT = """\
import sys
from ctypes import POINTER, byref, c_float, cast

from grass.lib.gis import G_gisinit
from grass.lib.raster import FCELL_TYPE
from grass.lib.raster3d import (
RASTER3D_NO_CACHE,
RASTER3D_Map,
RASTER3D_Region,
Rast3d_adjust_region,
Rast3d_init_defaults,
Rast3d_open_cell_new,
Rast3d_write_tile,
)

index_offset = int(sys.argv[1])

G_gisinit("test")
Rast3d_init_defaults()

region = RASTER3D_Region()
region.north = 100
region.south = 0
region.east = 100
region.west = 0
region.top = 100
region.bottom = 0
region.rows = 10
region.cols = 10
region.depths = 10
Rast3d_adjust_region(byref(region))

handle = Rast3d_open_cell_new(
"bounds_check_3d", FCELL_TYPE, RASTER3D_NO_CACHE, byref(region)
)
if not handle:
sys.exit("Rast3d_open_cell_new failed")
map_ptr = cast(handle, POINTER(RASTER3D_Map))
tile = (c_float * map_ptr.contents.tileSize)()
Rast3d_write_tile(map_ptr, map_ptr.contents.nTiles + index_offset, tile, FCELL_TYPE)
print("tile write accepted")
"""

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a weird way to create a test, but it does run a subprocess for it

@github-actions github-actions Bot added Python Related code is in Python tests Related to Test Suite labels Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C Related code is in C libraries Python Related code is in Python raster3d tests Related to Test Suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants