From b192ffb2f581989190dc0535724272431ae2c87a Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 19 May 2026 07:23:23 -0700 Subject: [PATCH 1/3] geotiff: sidecar download honours max_cloud_bytes (#2121) `load_sidecar` downloaded the sibling `.tif.ovr` over HTTP and fsspec with no byte cap. The base-file `max_cloud_bytes` budget that `read_to_array` and `_CloudSource` enforce was bypassed entirely, so a hostile server could serve a tiny base TIFF that passes the cloud budget plus a multi-GB sidecar; opening with `overview_level >= 1` pulled the full sidecar body into memory and OOMed the process. Thread `max_cloud_bytes` through `load_sidecar` and apply it on both transports: - HTTP path forwards the cap to `_HTTPSource.read_all(max_bytes=...)`, reusing the existing streaming overshoot detector. - fsspec path stats the sidecar via `fs.size()` and raises `CloudSizeLimitError` when the declared size exceeds the budget, mirroring the `_CloudSource` guard at `_reader.py:3239-3260`. `read_to_array` now resolves the cloud budget once at the top of the function so both the base-file size guard and the sidecar fetch see the same effective cap. `max_cloud_bytes=None` preserves the legacy unbounded behaviour. The local-file mmap path is unchanged. The GPU and dask-metadata sidecar call sites only see local file paths (via `_FileSource` and the `_read_geo_info` local branch respectively), so they fall through to the mmap branch and inherit the default `max_cloud_bytes=None` for free. Tests cover the fsspec size guard, the HTTP streaming cap, the `max_cloud_bytes=None` unbounded path, the local mmap passthrough, and end-to-end propagation from `read_to_array` into `load_sidecar` with a sidecar inflated past the base file's size. Closes #2121. --- xrspatial/geotiff/_reader.py | 20 +- xrspatial/geotiff/_sidecar.py | 61 ++++- .../test_sidecar_max_cloud_bytes_2121.py | 229 ++++++++++++++++++ 3 files changed, 300 insertions(+), 10 deletions(-) create mode 100644 xrspatial/geotiff/tests/test_sidecar_max_cloud_bytes_2121.py diff --git a/xrspatial/geotiff/_reader.py b/xrspatial/geotiff/_reader.py index 7494b9d8..b17cd13e 100644 --- a/xrspatial/geotiff/_reader.py +++ b/xrspatial/geotiff/_reader.py @@ -3229,6 +3229,12 @@ def read_to_array(source, *, window=None, overview_level: int | None = None, allow_rotated=allow_rotated) # Local file, cloud storage, or file-like buffer: read all bytes then parse + # Resolve the cloud byte budget once so both the base-file ``_CloudSource`` + # size guard and the sidecar download below see the same effective cap. + # ``_resolve_max_cloud_bytes`` honours the kwarg, the env var, and the + # default in that order; the result is ``None`` only when the caller + # explicitly passed ``max_cloud_bytes=None``. + cloud_budget = _resolve_max_cloud_bytes(max_cloud_bytes) if _is_file_like(source): src = _BytesIOSource(source) elif _is_fsspec_uri(source): @@ -3236,7 +3242,6 @@ def read_to_array(source, *, window=None, overview_level: int | None = None, # Check the compressed object size before any bytes are # downloaded. ``_CloudSource.__init__`` already fetched the size # via ``fsspec.size()``, so this is free. See issue #1928. - cloud_budget = _resolve_max_cloud_bytes(max_cloud_bytes) if cloud_budget is not None: size = src.size if size is None: @@ -3273,10 +3278,12 @@ def read_to_array(source, *, window=None, overview_level: int | None = None, # External `.tif.ovr` sidecar (issue #2112). GDAL/rasterio write # overview pyramids to a sibling file when the source is not a # COG; the sidecar's IFDs are the continuation of the base - # file's pyramid. Discovery only fires for local file paths; - # cloud / HTTP / file-like sources skip the lookup and keep the - # base-file-only behaviour. The sidecar must be loaded before - # IFD selection so ``overview_level`` can index into a unified + # file's pyramid. Discovery fires for local files, HTTP, and + # fsspec sources; file-like buffers skip the lookup. + # ``max_cloud_bytes`` propagates to ``load_sidecar`` so the + # sidecar fetch inherits the same byte budget the base file + # enforces (#2121). The sidecar must be loaded before IFD + # selection so ``overview_level`` indexes into a unified # pyramid list. from ._sidecar import ( attach_sidecar_origin, find_sidecar, load_sidecar, @@ -3284,7 +3291,8 @@ def read_to_array(source, *, window=None, overview_level: int | None = None, sidecar_origin: dict[int, tuple] = {} sidecar_path = find_sidecar(source) if sidecar_path is not None: - sidecar = load_sidecar(sidecar_path) + sidecar = load_sidecar(sidecar_path, + max_cloud_bytes=cloud_budget) sidecar_origin = attach_sidecar_origin( sidecar.ifds, sidecar.data, sidecar.header) ifds = ifds + sidecar.ifds diff --git a/xrspatial/geotiff/_sidecar.py b/xrspatial/geotiff/_sidecar.py index 9736136c..0de01ec6 100644 --- a/xrspatial/geotiff/_sidecar.py +++ b/xrspatial/geotiff/_sidecar.py @@ -129,7 +129,10 @@ def _probe_fsspec(uri: str) -> str | None: return None -def load_sidecar(path: str) -> SidecarOverviews: +def load_sidecar(path: str, + *, + max_cloud_bytes: int | None = None, + ) -> SidecarOverviews: """Open and parse a sidecar ``.ovr`` file. Accepts local file paths, HTTP / HTTPS URLs, and fsspec URIs. @@ -141,9 +144,32 @@ def load_sidecar(path: str) -> SidecarOverviews: full-resolution IFD, level 2 when the base file already carries one internal overview, and so on). + Parameters + ---------- + path + Sidecar path or URL returned by :func:`find_sidecar`. + max_cloud_bytes + Byte ceiling applied to HTTP and fsspec downloads. Mirrors the + base-file ``max_cloud_bytes`` budget that ``read_to_array`` and + ``_CloudSource`` enforce so a hostile or malformed sidecar can + not bypass the cap a caller already set on the source. ``None`` + (the default) means unbounded -- matches the base-file semantics + when the caller passes ``max_cloud_bytes=None`` explicitly. + Ignored on the local-file path because mmap does not allocate + the file. Issue #2121. + The returned ``data`` is either an ``mmap`` (local) or ``bytes`` (remote). Callers should close the mmap variant via ``data.close()`` when present; the bytes case is no-op. + + Raises + ------ + CloudSizeLimitError + If the fsspec sidecar's declared size exceeds ``max_cloud_bytes``. + The HTTP transport raises ``OSError`` from its streaming + overshoot detector rather than ``CloudSizeLimitError`` because + the size is checked mid-stream against ``Content-Length`` (when + present) and the running byte count (always). """ if "://" not in path: f = open(path, "rb") @@ -156,12 +182,39 @@ def load_sidecar(path: str) -> SidecarOverviews: # inherits SSRF validation, IP pinning, the shared urllib3 # PoolManager, and manual redirect re-validation. See # ``_probe_http`` for the threat model the indirection closes. + # ``max_bytes`` here closes a separate gap: without it, the + # sidecar fetch ignores the ``max_cloud_bytes`` budget the + # caller set on the base file and a hostile server can serve a + # multi-GB ``.ovr`` to OOM the process. Issue #2121. from ._reader import _HTTPSource - data = _HTTPSource(path).read_all() + data = _HTTPSource(path).read_all(max_bytes=max_cloud_bytes) else: - # fsspec URI + # fsspec URI. Stat the sidecar first so an oversized object is + # rejected before any bytes hit memory, mirroring the + # ``_CloudSource`` size guard at ``_reader.py:3239-3260``. + # Issue #2121. import fsspec - with fsspec.open(path, "rb") as f: + fs, fs_path = fsspec.core.url_to_fs(path) + if max_cloud_bytes is not None: + size = fs.size(fs_path) + from ._reader import CloudSizeLimitError + if size is None: + raise CloudSizeLimitError( + f"Sidecar {path!r} reports unknown size; refusing " + f"to download to avoid an unbounded read. Pass " + f"max_cloud_bytes=None on the source to disable the " + f"check for this sidecar." + ) + if size > max_cloud_bytes: + raise CloudSizeLimitError( + f"Sidecar {path!r} is {size:,} bytes, which exceeds " + f"max_cloud_bytes={max_cloud_bytes:,}. Raise " + f"max_cloud_bytes (or set " + f"XRSPATIAL_GEOTIFF_MAX_CLOUD_BYTES) if the sidecar " + f"is legitimate, or pass max_cloud_bytes=None on the " + f"source to disable the check." + ) + with fs.open(fs_path, "rb") as f: data = f.read() header = parse_header(data) ifds = parse_all_ifds(data, header) diff --git a/xrspatial/geotiff/tests/test_sidecar_max_cloud_bytes_2121.py b/xrspatial/geotiff/tests/test_sidecar_max_cloud_bytes_2121.py new file mode 100644 index 00000000..cc292f54 --- /dev/null +++ b/xrspatial/geotiff/tests/test_sidecar_max_cloud_bytes_2121.py @@ -0,0 +1,229 @@ +"""Sidecar download honours ``max_cloud_bytes`` (issue #2121). + +Before the fix, :func:`xrspatial.geotiff._sidecar.load_sidecar` downloaded +the sibling ``.tif.ovr`` over HTTP via ``_HTTPSource(path).read_all()`` and +over fsspec via ``fsspec.open(path, "rb").read()`` with no byte cap. The +base-file ``max_cloud_bytes`` budget that ``read_to_array`` and +``_CloudSource`` enforce was bypassed entirely, so a hostile server +serving a tiny base TIFF (which passes the cloud-budget check) plus a +multi-GB sidecar could OOM the reader on any ``overview_level >= 1``. + +These tests pin the new contract: + +* fsspec sidecar: declared size is checked against ``max_cloud_bytes`` + before any bytes are read, raising :class:`CloudSizeLimitError`. +* HTTP sidecar: ``max_bytes`` is threaded into the streaming read and + the overshoot detector raises :class:`OSError` mid-download. +* ``max_cloud_bytes=None`` preserves the legacy unbounded behaviour. +* Local-file mmap path is unaffected (no byte budget needed -- mmap + does not allocate the whole file). +""" +from __future__ import annotations + +import pathlib +import shutil + +import pytest + +from xrspatial.geotiff._reader import CloudSizeLimitError +from xrspatial.geotiff._sidecar import load_sidecar + + +_FIXTURE = ( + pathlib.Path(__file__).resolve().parent + / "golden_corpus" + / "fixtures" + / "overview_external_ovr_uint16.tif" +) + + +def _fixture_or_skip(): + if not _FIXTURE.exists(): + pytest.skip("sidecar fixture not present") + sidecar = _FIXTURE.parent / "overview_external_ovr_uint16.tif.ovr" + if not sidecar.exists(): + pytest.skip("sidecar .ovr file not present") + return _FIXTURE + + +def _start_http_server(directory): + """Spin up a loopback HTTP server serving *directory*. Returns (httpd, port).""" + import http.server + import socketserver + import threading + + class _Handler(http.server.SimpleHTTPRequestHandler): + def log_message(self, *a, **kw): + return # silence + + def __init__(self, *a, **kw): + super().__init__(*a, directory=str(directory), **kw) + + httpd = socketserver.TCPServer(("127.0.0.1", 0), _Handler) + thread = threading.Thread(target=httpd.serve_forever, daemon=True) + thread.start() + return httpd, httpd.server_address[1] + + +# --------------------------------------------------------------------------- +# Local file: mmap path is not subject to the budget. The cap argument is +# accepted but ignored because no allocation happens. +# --------------------------------------------------------------------------- +def test_local_sidecar_ignores_max_cloud_bytes(): + src = _fixture_or_skip() + sidecar_path = str(src) + ".ovr" + # Pass a tiny budget: local mmap is not subject to it. + sidecar = load_sidecar(sidecar_path, max_cloud_bytes=1) + try: + assert len(sidecar.ifds) == 2 + finally: + closer = getattr(sidecar.data, "close", None) + if closer is not None: + closer() + + +# --------------------------------------------------------------------------- +# fsspec: declared size is checked against the budget before any read. +# --------------------------------------------------------------------------- +def test_fsspec_sidecar_rejects_when_exceeds_max_cloud_bytes(tmp_path): + pytest.importorskip("fsspec") + src = _fixture_or_skip() + sidecar_src = str(src) + ".ovr" + sidecar_size = pathlib.Path(sidecar_src).stat().st_size + sidecar_copy = tmp_path / "x_2121.tif.ovr" + shutil.copy(sidecar_src, sidecar_copy) + + uri = f"file://{sidecar_copy}" + # Set the budget below the actual sidecar size: the size guard fires. + with pytest.raises(CloudSizeLimitError) as exc: + load_sidecar(uri, max_cloud_bytes=sidecar_size - 1) + assert "exceeds max_cloud_bytes" in str(exc.value) + assert str(sidecar_size) in str(exc.value).replace(",", "") + + +def test_fsspec_sidecar_succeeds_when_under_max_cloud_bytes(tmp_path): + pytest.importorskip("fsspec") + src = _fixture_or_skip() + sidecar_src = str(src) + ".ovr" + sidecar_size = pathlib.Path(sidecar_src).stat().st_size + sidecar_copy = tmp_path / "y_2121.tif.ovr" + shutil.copy(sidecar_src, sidecar_copy) + + uri = f"file://{sidecar_copy}" + # Budget comfortably above the sidecar size: download proceeds. + sidecar = load_sidecar(uri, max_cloud_bytes=sidecar_size * 10) + assert len(sidecar.ifds) == 2 + + +def test_fsspec_sidecar_max_cloud_bytes_none_is_unbounded(tmp_path): + pytest.importorskip("fsspec") + src = _fixture_or_skip() + sidecar_src = str(src) + ".ovr" + sidecar_copy = tmp_path / "z_2121.tif.ovr" + shutil.copy(sidecar_src, sidecar_copy) + + uri = f"file://{sidecar_copy}" + # max_cloud_bytes=None preserves the legacy unbounded behaviour: no + # fsspec.size() call, no cap, sidecar reads through. + sidecar = load_sidecar(uri, max_cloud_bytes=None) + assert len(sidecar.ifds) == 2 + + +# --------------------------------------------------------------------------- +# HTTP: streaming overshoot detector raises OSError when bytes exceed cap. +# --------------------------------------------------------------------------- +def test_http_sidecar_rejects_when_exceeds_max_cloud_bytes( + tmp_path, monkeypatch): + """Streaming download aborts when the body exceeds ``max_cloud_bytes``.""" + monkeypatch.setenv("XRSPATIAL_GEOTIFF_ALLOW_PRIVATE_HOSTS", "1") + src = _fixture_or_skip() + sidecar_src = str(src) + ".ovr" + sidecar_size = pathlib.Path(sidecar_src).stat().st_size + shutil.copy(sidecar_src, tmp_path / "h_2121.tif.ovr") + httpd, port = _start_http_server(tmp_path) + try: + url = f"http://127.0.0.1:{port}/h_2121.tif.ovr" + # Streaming cap below the body size. The HTTP source surfaces the + # over-shoot as OSError (Content-Length pre-check on + # SimpleHTTPRequestHandler) or via the streaming probe -- either + # way, the download is rejected before the full payload lands. + with pytest.raises(OSError): + load_sidecar(url, max_cloud_bytes=sidecar_size - 1) + finally: + httpd.shutdown() + + +def test_http_sidecar_succeeds_when_under_max_cloud_bytes( + tmp_path, monkeypatch): + monkeypatch.setenv("XRSPATIAL_GEOTIFF_ALLOW_PRIVATE_HOSTS", "1") + src = _fixture_or_skip() + sidecar_src = str(src) + ".ovr" + sidecar_size = pathlib.Path(sidecar_src).stat().st_size + shutil.copy(sidecar_src, tmp_path / "ok_2121.tif.ovr") + httpd, port = _start_http_server(tmp_path) + try: + url = f"http://127.0.0.1:{port}/ok_2121.tif.ovr" + sidecar = load_sidecar(url, max_cloud_bytes=sidecar_size * 10) + assert len(sidecar.ifds) == 2 + finally: + httpd.shutdown() + + +def test_http_sidecar_max_cloud_bytes_none_is_unbounded( + tmp_path, monkeypatch): + """``max_cloud_bytes=None`` preserves the pre-#2121 unbounded read.""" + monkeypatch.setenv("XRSPATIAL_GEOTIFF_ALLOW_PRIVATE_HOSTS", "1") + src = _fixture_or_skip() + sidecar_src = str(src) + ".ovr" + shutil.copy(sidecar_src, tmp_path / "u_2121.tif.ovr") + httpd, port = _start_http_server(tmp_path) + try: + url = f"http://127.0.0.1:{port}/u_2121.tif.ovr" + sidecar = load_sidecar(url, max_cloud_bytes=None) + assert len(sidecar.ifds) == 2 + finally: + httpd.shutdown() + + +# --------------------------------------------------------------------------- +# End-to-end: read_to_array's cloud_budget propagates into load_sidecar. +# A user-provided ``max_cloud_bytes`` set on the base file flows down to +# the sidecar fetch instead of being silently dropped. +# --------------------------------------------------------------------------- +def test_read_to_array_propagates_max_cloud_bytes_to_sidecar( + tmp_path, monkeypatch): + """A fsspec ``file://`` source with a tight budget rejects the sidecar. + + Pre-#2121, ``read_to_array(..., max_cloud_bytes=N)`` enforced ``N`` only + on the base file and silently bypassed it for the sidecar fetch. This + test inflates the sidecar (a normal ``.ovr`` is small relative to its + base) so the cloud budget that admits the base file rejects the + sidecar; the fix routes ``cloud_budget`` into ``load_sidecar`` so the + sidecar fetch trips the same guard. + """ + pytest.importorskip("fsspec") + src = _fixture_or_skip() + base_copy = tmp_path / "rt_2121.tif" + sidecar_copy = tmp_path / "rt_2121.tif.ovr" + shutil.copy(src, base_copy) + shutil.copy(str(src) + ".ovr", sidecar_copy) + # Pad the sidecar with trailing zeros so its on-disk size exceeds the + # base. The TIFF parser ignores trailing bytes past the last IFD chain + # entry, so the inflated sidecar still opens cleanly when the cap + # admits it. The base file is left untouched. + base_size = base_copy.stat().st_size + target_sidecar_size = base_size + 4096 + with sidecar_copy.open("ab") as f: + f.write(b"\x00" * (target_sidecar_size - sidecar_copy.stat().st_size)) + sidecar_size = sidecar_copy.stat().st_size + assert sidecar_size > base_size + + # Budget that admits the base file (base_size <= budget) and rejects + # the sidecar (sidecar_size > budget). + budget = base_size + 1 + assert base_size <= budget < sidecar_size + + from xrspatial.geotiff._reader import read_to_array + uri = f"file://{base_copy}" + with pytest.raises(CloudSizeLimitError): + read_to_array(uri, overview_level=1, max_cloud_bytes=budget) From 7cf7a8811bf3add6efeb6670ff691b26771d8390 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 19 May 2026 07:24:25 -0700 Subject: [PATCH 2/3] sweep state: geotiff re-audit 2026-05-19 #2121 --- .claude/sweep-security-state.csv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/sweep-security-state.csv b/.claude/sweep-security-state.csv index a58dab2a..a7dcefba 100644 --- a/.claude/sweep-security-state.csv +++ b/.claude/sweep-security-state.csv @@ -18,7 +18,7 @@ fire,2026-04-25,,,,,"Clean. Despite the module's size hint, fire.py is purely pe flood,2026-05-03,1437,MEDIUM,3,,Re-audit 2026-05-03. MEDIUM Cat 3 fixed in PR #1438 (travel_time and flood_depth_vegetation now validate mannings_n DataArray values are finite and strictly positive via _validate_mannings_n_dataarray helper). No remaining unfixed findings. Other categories clean: every allocation is same-shape as input; no flat index math; NaN propagation explicit in every backend; tan_slope clamped by _TAN_MIN; no CUDA kernels; no file I/O; every public API calls _validate_raster on DataArray inputs. focal,2026-04-27,1284,HIGH,1,,"HIGH (fixed PR #1286): apply(), focal_stats(), and hotspots() accepted unbounded user-supplied kernels via custom_kernel(), which only checks shape parity. The kernel-size guard from #1241 (_check_kernel_memory) only ran inside circle_kernel/annulus_kernel, so a (50001, 50001) custom kernel on a 10x10 raster allocated ~10 GB on the kernel itself plus a much larger padded raster before any work -- same shape as the bilateral DoS in #1236. Fixed by adding _check_kernel_vs_raster_memory in focal.py and wiring it into apply(), focal_stats(), and hotspots() after custom_kernel() validation. All 134 focal tests + 19 bilateral tests pass. No other findings: 10 CUDA kernels all have proper bounds + stencil guards; _validate_raster called on every public entry point; hotspots already raises ZeroDivisionError on constant-value rasters; _focal_variety_cuda uses a fixed-size local buffer (silent truncation but bounded); _focal_std_cuda/_focal_var_cuda clamp the catastrophic-cancellation case via if var < 0.0: var = 0.0; no file I/O." geodesic,2026-04-27,1283,HIGH,1,,"HIGH (fixed PR #1285): slope(method='geodesic') and aspect(method='geodesic') stack a (3, H, W) float64 array (data, lat, lon) before dispatch with no memory check. A large lat/lon-tagged raster passed to either function would OOM. Fixed by adding _check_geodesic_memory(rows, cols) in xrspatial/geodesic.py (mirrors morphology._check_kernel_memory): budgets 56 bytes/cell (24 stacked float64 + 4 float32 output + 24 padded copy + slack) and raises MemoryError when > 50% of available RAM; called from slope.py and aspect.py inside the geodesic branch before dispatch. No other findings: 6 CUDA kernels all have bounds guards (e.g. _run_gpu_geodesic_aspect at geodesic.py:395), custom 16x16 thread blocks avoid register spill, no shared memory, _validate_raster runs upstream in slope/aspect, all backends cast to float32, slope_mag < 1e-7 flat threshold prevents arctan2 NaN propagation, curvature correction uses hardcoded WGS84 R." -geotiff,2026-05-18,,MEDIUM,1,,"Re-audit pass 18 2026-05-18 (deep-sweep p1). MEDIUM Cat 1 fixed in deep-sweep-security-geotiff-2026-05-18-p1: read_geotiff_gpu eager path (_backends/gpu.py) now applies the same _max_tile_bytes_from_env() per-tile cap that _read_tiles and _fetch_decode_cog_http_tiles enforce. The CPU and GPU readers now agree on the per-tile budget; a malformed local TIFF with TileByteCounts pointing into a large file region is rejected before GPU decode rather than relying on _check_gpu_memory's aggregate-sum guard. Test: tests/test_gpu_tile_byte_cap_2026_05_18.py. Other categories verified clean: JPEG bomb cap (#1792), HTTP read_all byte budget (#2057), VRT XML cap, DOCTYPE rejection, path containment, SSRF, validate_tile_layout, dimension caps, IFD entry caps, MAX_IFDS, MAX_PIXEL_ARRAY_COUNT, GPU bounds guards, atomic writes, realpath canonicalization, dtype validation." +geotiff,2026-05-19,2121,HIGH,1,,"Re-audit pass 19 2026-05-19 (deep-sweep p1). HIGH Cat 1 found in _sidecar.py load_sidecar: HTTP and fsspec sidecar downloads bypassed max_cloud_bytes set on the base file, so a hostile server could OOM the reader via a multi-GB .tif.ovr beside a tiny base TIFF (issue #2121). Fixed in deep-sweep-security-geotiff-2026-05-19-01 (PR #2123) by threading max_cloud_bytes through load_sidecar and applying it on both transports (HTTP via _HTTPSource.read_all max_bytes streaming cap, fsspec via fs.size() pre-check raising CloudSizeLimitError). Test: tests/test_sidecar_max_cloud_bytes_2121.py. All other categories verified clean against new commits 68574fe (.tif.ovr sidecar), 6b88cea (allow_rotated rotated MTT), f2e191d (multi-ModelTiepoint GCP rejection), 1e9c432 (GPU per-tile byte cap). Carries forward: JPEG bomb cap (#1792), HTTP read_all byte budget (#2057), VRT XML cap, DOCTYPE rejection, path containment, SSRF, validate_tile_layout, dimension caps, IFD entry caps, MAX_IFDS, MAX_PIXEL_ARRAY_COUNT, GPU bounds guards, atomic writes, realpath canonicalization, dtype validation." glcm,2026-04-24,1257,HIGH,1,,"HIGH (fixed #1257): glcm_texture() validated window_size only as >= 3 and distance only as >= 1, with no upper bound on either. _glcm_numba_kernel iterates range(r-half, r+half+1) for every pixel, so window_size=1_000_001 on a 10x10 raster ran ~10^14 loop iterations with all neighbors failing the interior bounds check (CPU DoS). On the dask backends depth = window_size // 2 + distance drove map_overlap padding, so a huge window also caused oversize per-chunk allocations (memory DoS). Fixed by adding max_val caps in the public entrypoint: window_size <= max(3, min(rows, cols)) and distance <= max(1, window_size // 2). One cap covers every backend because cupy and dask+cupy call through to the CPU kernel after cupy.asnumpy. No other HIGH findings: levels is already capped at 256 so the per-pixel np.zeros((levels, levels)) matrix in the kernel is bounded to 512 KB. No CUDA kernels. No file I/O. Quantization clips to [0, levels-1] before the kernel and NaN maps to -1 which the kernel filters with i_val >= 0. Entropy log(p) and correlation p / (std_i * std_j) are both guarded. All four backends use _validate_raster and cast to float64 before quantizing. MEDIUM (unfixed, Cat 1): the per-pixel np.zeros((levels, levels)) allocation inside the hot loop is a perf issue (levels=256 -> 512 KB alloc+free per pixel) but not a security issue because levels is bounded. Could be hoisted out of the loop or replaced with an in-place clear, but that is an efficiency concern, not security." gpu_rtx,2026-04-29,1308,HIGH,1,,"HIGH (fixed #1308 / PR #1310): hillshade_rtx (gpu_rtx/hillshade.py:184) and viewshed_gpu (gpu_rtx/viewshed.py:269) allocated cupy device buffers sized by raster shape with no memory check. create_triangulation (mesh_utils.py:23-24) adds verts (12 B/px) + triangles (24 B/px) = 36 B/px; hillshade_rtx adds d_rays(32) + d_hits(16) + d_aux(12) + d_output(4) = 64 B/px (100 B/px total); viewshed_gpu adds d_rays(32) + d_hits(16) + d_visgrid(4) + d_vsrays(32) = 84 B/px (120 B/px total). A 30000x30000 raster asked for 90-108 GB of VRAM before cupy surfaced an opaque allocator error. Fixed by adding gpu_rtx/_memory.py with _available_gpu_memory_bytes() and _check_gpu_memory(func_name, h, w) helpers (cost_distance #1262 / sky_view_factor #1299 pattern, 120 B/px budget covers worst case, raises MemoryError when required > 50% of free VRAM, skips silently when memGetInfo() unavailable). Wired into both entry points after the cupy.ndarray type check and before create_triangulation. 9 new tests in test_gpu_rtx_memory.py (5 helper-unit + 4 end-to-end gated on has_rtx). All 81 existing hillshade/viewshed tests still pass. Cat 4 clean: all CUDA kernels (hillshade.py:25/62/106, viewshed.py:32/74/116, mesh_utils.py:50) have bounds guards; no shared memory, no syncthreads needed. MEDIUM not fixed (Cat 6): hillshade_rtx and viewshed_gpu do not call _validate_raster directly but parent hillshade() (hillshade.py:252) and viewshed() (viewshed.py:1707) already validate, so input validation runs before the gpu_rtx entry point - defense-in-depth, not exploitable. MEDIUM not fixed (Cat 2): mesh_utils.py:64-68 cast mesh_map_index to int32 in the triangle index buffer; overflows at H*W > 2.1B vertices (~46341x46341+) but the new memory guard rejects rasters that large first - documentation/clarity item rather than exploitable. MEDIUM not fixed (Cat 3): mesh_utils.py:19 scale = maxDim / maxH divides by zero on an all-zero raster, propagating inf/NaN into mesh vertex z-coords; separate follow-up. LOW not fixed (Cat 5): mesh_utils.write() opens user-supplied path without canonicalization but its only call site (mesh_utils.py:38-39) sits behind if False: in create_triangulation, not reachable in production." hillshade,2026-04-27,,,,,"Clean. Cat 1: only allocation is the output np.empty(data.shape) at line 32 (cupy at line 165) and a _pad_array with hardcoded depth=1 (line 62) -- bounded by caller, no user-controlled amplifier. Azimuth/altitude are scalars and don't drive size. Cat 2: numba kernel uses range(1, rows-1) with simple (y, x) indexing; numba range loops promote to int64. Cat 3: math.sqrt(1.0 + xx_plus_yy) is always >= 1.0 (no neg sqrt, no div-by-zero); NaN elevation propagates correctly through dz_dx/dz_dy -> shaded -> output (the shaded < 0.0 / shaded > 1.0 clamps don't fire on NaN). Azimuth validated to [0, 360], altitude to [0, 90]. Cat 4: _gpu_calc_numba (line 107) guards both grid bounds and 3x3 stencil reads via i > 0 and i < shape[0]-1 and j > 0 and j < shape[1]-1; no shared memory. Cat 5: no file I/O. Cat 6: hillshade() calls _validate_raster (line 252) and _validate_scalar for both azimuth (253) and angle_altitude (254); all four backend paths cast to float32; tests parametrize int32/int64/float32/float64." From 77e5bb663339cb9cdbd8dc8b4a067c301ca44443 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 19 May 2026 07:59:11 -0700 Subject: [PATCH 3/3] geotiff: unify sidecar size-limit error type and cover env-var path Address review feedback on #2121: 1. ``load_sidecar`` now translates the ``OSError`` raised by ``_HTTPSource.read_all`` budget guards into ``CloudSizeLimitError`` so the HTTP and fsspec branches surface the same exception type for "sidecar exceeds the cap". The original ``OSError`` is preserved as ``__cause__``. Non-budget HTTP failures (connection reset, DNS, etc.) still propagate as ``OSError``. 2. Add ``test_env_var_propagates_to_sidecar`` to pin that ``XRSPATIAL_GEOTIFF_MAX_CLOUD_BYTES`` (no kwarg) caps the sidecar the same way an explicit ``max_cloud_bytes`` kwarg does. The existing HTTP overshoot test now asserts ``CloudSizeLimitError`` and that ``__cause__`` retains the underlying ``OSError`` detail. --- xrspatial/geotiff/_sidecar.py | 39 +++++++++--- .../test_sidecar_max_cloud_bytes_2121.py | 60 ++++++++++++++++--- 2 files changed, 85 insertions(+), 14 deletions(-) diff --git a/xrspatial/geotiff/_sidecar.py b/xrspatial/geotiff/_sidecar.py index 0de01ec6..603dd162 100644 --- a/xrspatial/geotiff/_sidecar.py +++ b/xrspatial/geotiff/_sidecar.py @@ -165,11 +165,15 @@ def load_sidecar(path: str, Raises ------ CloudSizeLimitError - If the fsspec sidecar's declared size exceeds ``max_cloud_bytes``. - The HTTP transport raises ``OSError`` from its streaming - overshoot detector rather than ``CloudSizeLimitError`` because - the size is checked mid-stream against ``Content-Length`` (when - present) and the running byte count (always). + If the sidecar's size exceeds ``max_cloud_bytes`` on either + transport. fsspec checks the declared size up front via + ``fs.size()``; HTTP catches the ``OSError`` that + :meth:`_HTTPSource.read_all` raises from its + ``Content-Length`` pre-check or streaming overshoot detector + and re-raises it as ``CloudSizeLimitError`` so callers see a + single exception type for "sidecar too big". Non-budget HTTP + failures (connection reset, DNS error, etc.) pass through as + ``OSError`` unchanged. """ if "://" not in path: f = open(path, "rb") @@ -186,8 +190,29 @@ def load_sidecar(path: str, # sidecar fetch ignores the ``max_cloud_bytes`` budget the # caller set on the base file and a hostile server can serve a # multi-GB ``.ovr`` to OOM the process. Issue #2121. - from ._reader import _HTTPSource - data = _HTTPSource(path).read_all(max_bytes=max_cloud_bytes) + from ._reader import CloudSizeLimitError, _HTTPSource + try: + data = _HTTPSource(path).read_all(max_bytes=max_cloud_bytes) + except OSError as e: + # ``_HTTPSource.read_all(max_bytes=...)`` raises ``OSError`` + # with "byte budget" in the message for both the + # Content-Length pre-check and the streaming overshoot probe + # (see ``_HTTPSource._check_content_length`` and + # ``_read_capped``). Translate to ``CloudSizeLimitError`` so + # the HTTP and fsspec branches surface the same exception + # type for the same failure mode. Other ``OSError`` + # subtypes (connection reset, DNS, etc.) pass through + # untouched. + if max_cloud_bytes is not None and "byte budget" in str(e): + raise CloudSizeLimitError( + f"Sidecar {path!r} exceeds " + f"max_cloud_bytes={max_cloud_bytes:,}. Raise " + f"max_cloud_bytes (or set " + f"XRSPATIAL_GEOTIFF_MAX_CLOUD_BYTES) if the sidecar " + f"is legitimate, or pass max_cloud_bytes=None on the " + f"source to disable the check." + ) from e + raise else: # fsspec URI. Stat the sidecar first so an oversized object is # rejected before any bytes hit memory, mirroring the diff --git a/xrspatial/geotiff/tests/test_sidecar_max_cloud_bytes_2121.py b/xrspatial/geotiff/tests/test_sidecar_max_cloud_bytes_2121.py index cc292f54..cd5ed91f 100644 --- a/xrspatial/geotiff/tests/test_sidecar_max_cloud_bytes_2121.py +++ b/xrspatial/geotiff/tests/test_sidecar_max_cloud_bytes_2121.py @@ -13,7 +13,9 @@ * fsspec sidecar: declared size is checked against ``max_cloud_bytes`` before any bytes are read, raising :class:`CloudSizeLimitError`. * HTTP sidecar: ``max_bytes`` is threaded into the streaming read and - the overshoot detector raises :class:`OSError` mid-download. + the overshoot is re-raised as :class:`CloudSizeLimitError` so both + transports surface a single exception type for "sidecar exceeds the + cap". Non-budget HTTP failures still raise ``OSError`` unchanged. * ``max_cloud_bytes=None`` preserves the legacy unbounded behaviour. * Local-file mmap path is unaffected (no byte budget needed -- mmap does not allocate the whole file). @@ -130,7 +132,8 @@ def test_fsspec_sidecar_max_cloud_bytes_none_is_unbounded(tmp_path): # --------------------------------------------------------------------------- -# HTTP: streaming overshoot detector raises OSError when bytes exceed cap. +# HTTP: load_sidecar translates the underlying budget OSError into +# CloudSizeLimitError so both cloud transports raise the same type. # --------------------------------------------------------------------------- def test_http_sidecar_rejects_when_exceeds_max_cloud_bytes( tmp_path, monkeypatch): @@ -143,12 +146,17 @@ def test_http_sidecar_rejects_when_exceeds_max_cloud_bytes( httpd, port = _start_http_server(tmp_path) try: url = f"http://127.0.0.1:{port}/h_2121.tif.ovr" - # Streaming cap below the body size. The HTTP source surfaces the - # over-shoot as OSError (Content-Length pre-check on - # SimpleHTTPRequestHandler) or via the streaming probe -- either - # way, the download is rejected before the full payload lands. - with pytest.raises(OSError): + # Cap below the body size. Either the Content-Length pre-check + # or the streaming overshoot probe inside ``_HTTPSource.read_all`` + # raises ``OSError``; ``load_sidecar`` re-raises it as + # ``CloudSizeLimitError`` so callers see a single exception + # type regardless of transport. + with pytest.raises(CloudSizeLimitError) as exc: load_sidecar(url, max_cloud_bytes=sidecar_size - 1) + assert "max_cloud_bytes" in str(exc.value) + # The original OSError is preserved as the cause so debuggers + # can still see the Content-Length / streaming detail. + assert isinstance(exc.value.__cause__, OSError) finally: httpd.shutdown() @@ -227,3 +235,41 @@ def test_read_to_array_propagates_max_cloud_bytes_to_sidecar( uri = f"file://{base_copy}" with pytest.raises(CloudSizeLimitError): read_to_array(uri, overview_level=1, max_cloud_bytes=budget) + + +# --------------------------------------------------------------------------- +# Env-var precedence: XRSPATIAL_GEOTIFF_MAX_CLOUD_BYTES caps the sidecar +# when the caller omits the kwarg, mirroring base-file semantics. +# --------------------------------------------------------------------------- +def test_env_var_propagates_to_sidecar(tmp_path, monkeypatch): + """``XRSPATIAL_GEOTIFF_MAX_CLOUD_BYTES`` bounds the sidecar fetch. + + ``read_to_array`` resolves the budget once via + ``_resolve_max_cloud_bytes`` and passes it into ``load_sidecar``. + When the caller does not pass the kwarg, the env var should still + cap the sidecar -- the kwarg path and the env-var path must agree. + """ + pytest.importorskip("fsspec") + src = _fixture_or_skip() + base_copy = tmp_path / "env_2121.tif" + sidecar_copy = tmp_path / "env_2121.tif.ovr" + shutil.copy(src, base_copy) + shutil.copy(str(src) + ".ovr", sidecar_copy) + base_size = base_copy.stat().st_size + target_sidecar_size = base_size + 4096 + with sidecar_copy.open("ab") as f: + f.write(b"\x00" * (target_sidecar_size - sidecar_copy.stat().st_size)) + sidecar_size = sidecar_copy.stat().st_size + assert sidecar_size > base_size + + # Env-var budget admits the base file but rejects the sidecar. The + # caller passes no ``max_cloud_bytes``, so the resolver picks up the + # env var and threads it through to ``load_sidecar``. + budget = base_size + 1 + assert base_size <= budget < sidecar_size + monkeypatch.setenv("XRSPATIAL_GEOTIFF_MAX_CLOUD_BYTES", str(budget)) + + from xrspatial.geotiff._reader import read_to_array + uri = f"file://{base_copy}" + with pytest.raises(CloudSizeLimitError): + read_to_array(uri, overview_level=1)