Skip to content

lib/raster: guard compressed row memcpy size - #8015

Merged
echoix merged 2 commits into
OSGeo:mainfrom
nilason:fix_raster_get_row
Oct 11, 2026
Merged

echoix merged 2 commits into
OSGeo:mainfrom
nilason:fix_raster_get_row

Conversation

@nilason

@nilason nilason commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Addresses Coverity Scan CID 1687668 introduced by 6e39994

Addresses CID 1687668 introduced by 6e39994
@github-actions github-actions Bot added raster Related to raster data processing C Related code is in C libraries labels Oct 4, 2026
@nilason nilason changed the title grass/raster: guard compressed row memcpy size lib/raster: guard compressed row memcpy size Oct 4, 2026
@echoix

echoix commented Oct 4, 2026

Copy link
Copy Markdown
Member

My AI review found something, I asked it to create a real test to prove the overflow it speculated, I’ll come back later when it’s done

@echoix

echoix commented Oct 4, 2026

Copy link
Copy Markdown
Member

Reading a crafted compressed CELL raster still overflows the row buffer with this PR applied — the new readamount > bufsize check guards the row length but not the per-cell byte count n, which is read from the file and used as bufsize = n * cols. A map whose header says 1 byte/cell but whose row count byte is 255 makes the memcpy write 255 * cols bytes into a cols-byte buffer. Reproduced under AddressSanitizer (heap-buffer-overflow ... read_data_compressed ... get_row.c, 0 bytes after the buffer from open.c:343).

Patch (fix + a pytest that fails with a heap-buffer-overflow without the guard and passes with it):

diff --git a/lib/raster/get_row.c b/lib/raster/get_row.c
index 6540f64..c1bc65a 100644
--- a/lib/raster/get_row.c
+++ b/lib/raster/get_row.c
@@ -180,6 +180,17 @@ static void read_data_compressed(int fd, int row, unsigned char *data_buf,
         /* one byte is nbyte count */
         n = *nbytes = *cmp++;
         readamount -= 1;
+
+        /* The per-cell byte count is read from the file and must not exceed
+         * the bytes-per-cell the data buffer was allocated for (fcb->nbytes
+         * from the map header); a larger value indicates a corrupted file and
+         * would otherwise overflow data_buf below. */
+        if (n > fcb->nbytes) {
+            G_free(cmp2);
+            G_fatal_error(
+                _("Invalid byte count in raster data for row %d of <%s>"), row,
+                fcb->name);
+        }
     }
     else
         /* pre 3.0 compression */
diff --git a/lib/raster/tests/lib_raster_get_row_corrupt_test.py b/lib/raster/tests/lib_raster_get_row_corrupt_test.py
new file mode 100644
index 0000000..957e6a9
--- /dev/null
+++ b/lib/raster/tests/lib_raster_get_row_corrupt_test.py
@@ -0,0 +1,55 @@
+"""Tests that reading a corrupted compressed CELL raster fails gracefully
+
+(C) 2025 by the GRASS Development Team
+
+This program is free software under the GNU General Public License
+(>=v2). Read the file COPYING that comes with GRASS for details.
+"""
+
+import struct
+from pathlib import Path
+
+import pytest
+
+from grass.exceptions import CalledModuleError
+from grass.tools import Tools
+
+
+def test_compressed_cell_row_byte_count_overflow(session):
+    """A crafted per-cell byte count must not overflow the row buffer
+
+    read_data_compressed() reads the per-cell byte count from the first byte
+    of each compressed row and computes the copy size as count * cols. The
+    row buffer, however, is allocated for the bytes-per-cell recorded in the
+    map header. A corrupted file whose byte count exceeds that header value
+    would make the copy write past the end of the buffer (a heap overflow,
+    reported by Coverity). Reading such a file must fail with an error instead.
+    """
+    tools = Tools(session=session)
+    cols = 4
+    tools.g_region(rows=1, cols=cols, n=1, s=0, e=cols, w=0, env=session.env)
+    # Values 0..cols fit in one byte, so the map header records one byte per
+    # cell and the read buffer is cols bytes long.
+    tools.r_mapcalc(expression="corrupt = col()", env=session.env)
+
+    cell_file = Path(
+        tools.g_findfile(
+            element="cell", file="corrupt", format="json", env=session.env
+        )["file"]
+    )
+
+    # Rewrite the cell file as a single compressed row whose first byte claims
+    # 255 bytes per cell. The row pointer table is one leading byte giving the
+    # offset width (8) followed by one big-endian offset per row boundary.
+    offset_width = 8
+    table_len = 1 + (1 + 1) * offset_width
+    byte_count = 255
+    row_payload = bytes([byte_count]) + b"\x00" * (byte_count * cols)
+
+    table = bytearray([offset_width])
+    table += struct.pack(">q", table_len)
+    table += struct.pack(">q", table_len + len(row_payload))
+    cell_file.write_bytes(bytes(table) + row_payload)
+
+    with pytest.raises(CalledModuleError, match="byte count"):
+        tools.r_out_ascii(input="corrupt", env=session.env)

The per-cell byte count is read from the file and must not exceed
the bytes-per-cell the data buffer was allocated for (fcb->nbytes
from the map header); a larger value indicates a corrupted file and
would otherwise overflow data_buf below.
@nilason

nilason commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Reading a crafted compressed CELL raster still overflows the row buffer with this PR applied — the new readamount > bufsize check guards the row length but not the per-cell byte count n, which is read from the file and used as bufsize = n * cols. A map whose header says 1 byte/cell but whose row count byte is 255 makes the memcpy write 255 * cols bytes into a cols-byte buffer. Reproduced under AddressSanitizer (heap-buffer-overflow ... read_data_compressed ... get_row.c, 0 bytes after the buffer from open.c:343).

Thanks, makes sense, added the suggested check.

@echoix
echoix merged commit 30cdc69 into OSGeo:main Oct 11, 2026
26 checks passed
@github-actions github-actions Bot added this to the 8.6.0 milestone Oct 11, 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 raster Related to raster data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants