diff --git a/.agent/work-plans/issue-152/plan.md b/.agent/work-plans/issue-152/plan.md new file mode 100644 index 0000000..1038341 --- /dev/null +++ b/.agent/work-plans/issue-152/plan.md @@ -0,0 +1,76 @@ +# Plan: Fix GDAL/OGR handle leaks in Georeferenced / VectorDataset chart loading + +## Issue + +https://github.com/rolker/camp/issues/152 + +## Context + +Two classes leak GDAL/OGR handles on every chart load, confirmed by valgrind (~180 KB of OGR/PROJ state per `DepthRaster` / `VectorDataset` instance, growing monotonically across a session): + +1. **`Georeferenced`** (`georeferenced.{h,cpp}`) — `extractGeoreference()` allocates `m_projectTransformation` and `m_unprojectTransformation` via `OGRCreateCoordinateTransformation` (lines 48–49). There is no destructor to free them. This base class is inherited by both `DepthRaster` and `VectorDataset`, so every instance leaks both handles. + +2. **`VectorDataset::open`** (`vector/vectordataset.cpp`) — four leaks: + - `GDALDataset*` from `GDALOpenEx` (line 22) is never `GDALClose`d. + - Per-layer `OGRCoordinateTransformation* unprojectTransformation` (line 35) is never destroyed. + - `OGRPointIterator* pi` is never destroyed at three sites: linestring (line 72), polygon exterior ring (line 100), polygon interior rings (line 119). + - (`OGRFeature` is correctly freed via `DestroyFeature` — no change needed there.) + +`DepthRaster` already calls `GDALClose(dataset)` (line 30 of `depth_raster.cpp`) — no dataset handle leak there. The `gdal_closer` RAII pattern is already established in `raster_layer.cpp:176–178`. + +## Approach + +1. **Add `virtual ~Georeferenced()`** — destroy both OGR coordinate transformations. Also `= delete` the copy ctor/assignment (rule-of-three: the class now owns handles; both subclasses are heap-only so no live call site changes). *(review-plan suggestion folded)* +2. **Fix `VectorDataset::open`** — RAII-close the GDAL dataset; destroy the per-layer transformation **at end-of-layer** (else all but the last layer leak); destroy every `OGRPointIterator` **at its site**, including the interior-ring iterator **inside the ring loop** (`:119`, reassigned per ring). *(review-plan must-fix folded)* +3. **Extract a testable seam (operator decision, post-review).** The original `open()` interleaved the GDAL/OGR resource lifecycle with construction of the project item graph (`Point`/`LineString`/`Polygon` + `connect(autonomousVehicleProject(), …)`), so any test driving `open()` would have to link ~all 52 app TUs and stand up a `QApplication`-backed `AutonomousVehicleProject` — the "bounded source list" balloons. Instead split the pure parse into a new MissionItem-free TU: + - `src/camp/vector/vector_parse.{h,cpp}` — `camp::vector::parseVectorLayers(GDALDataset*)` returns plain WGS84 geometry (`ParsedLayer`/`ParsedGeometry`) and owns the per-layer transform + per-ring iterator lifecycle (the #152 leak sites). No Qt-item / project coupling. + - `VectorDataset::open` RAII-opens the dataset, calls `extractGeoreference`, then `buildItems(parseVectorLayers(...))`. + - `VectorDataset::buildItems(layers)` — the unchanged item-graph construction, now fed plain data. Coordinate handling preserved verbatim per geometry type. +4. **Extend `test_depth_raster.cpp`** — add a GDAL open-dataset-count baseline-delta test (`LoadAndDestroyLeavesNoOpenDataset`). NOTE: it guards the **pre-existing** `GDALClose`, not the new dtor — `GetOpenDatasets()` can't see the `OGRCoordinateTransformation` handles the dtor frees (valgrind-only). Stated in the test comment. *(review-plan suggestion folded)* +5. **Add `test_vector_dataset_cleanup.cpp`** — links the standalone `vector_parse` TU (no item graph). Synthetic 2-layer GeoPackage (point + linestring + polygon-with-hole per layer); calls `parseVectorLayers`; asserts every parse branch ran (non-vacuous) and the GDAL open-dataset count returns to baseline. The transform/iterator leaks are **valgrind-only** (run the binary with `--leak-check=full`; definitely-lost must be 0) — documented in the test + CMake comments. + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp/georeferenced.h` | Add `virtual ~Georeferenced();` + `= delete` copy ctor/assignment | +| `src/camp/georeferenced.cpp` | Implement destructor: `OGRCoordinateTransformation::DestroyCT` on both members | +| `src/camp/vector/vector_parse.h` | **New** — `ParsedGeometry`/`ParsedLayer` + `parseVectorLayers(GDALDataset*)` (MissionItem-free) | +| `src/camp/vector/vector_parse.cpp` | **New** — pure parse; per-layer transform + per-ring iterator created and destroyed; `OGRFeature::DestroyFeature` retained | +| `src/camp/vector/vectordataset.h` | Forward-declare `camp::vector::ParsedLayer`; declare private `buildItems` | +| `src/camp/vector/vectordataset.cpp` | `open()` → RAII dataset + `extractGeoreference` + `buildItems(parseVectorLayers(...))`; new `buildItems` holds the item-graph construction | +| `test/test_depth_raster.cpp` | Add GDAL open-dataset baseline-delta test (guards pre-existing `GDALClose`; dtor leak is valgrind-only) | +| `test/test_vector_dataset_cleanup.cpp` | **New** — links `vector_parse` TU; synthetic 2-layer GPKG; asserts parse branches + handle-count baseline; transform/iterator leaks valgrind-only | +| `CMakeLists.txt` | Add `vector_parse.cpp` to executable sources; register `test_vector_dataset_cleanup` | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| Only what's needed | Fix targets exactly the confirmed leak sites; no refactor beyond RAII at those sites | +| A change includes its consequences | Tests added for both fix paths; valgrind re-run required in PR | +| Test what breaks | New tests catch regressions in the GDAL dataset leak path (handle count delta); OGR transform leak covered by valgrind output in PR | +| Improve incrementally | Single PR; no structural refactor of `Georeferenced` beyond the dtor | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| ADR-0002 — Worktree isolation | Yes | Worktree `issue-camp-152` / branch `feature/issue-152` already in use | +| ADR-0008 — ROS 2 conventions | Marginal | C++ source only; follow existing RAII and naming patterns in this package | +| ADR-0013 — progress.md vocabulary | Yes | progress.md maintained | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| `Georeferenced` gains `virtual ~Georeferenced()` | `DepthRaster` and `VectorDataset` inherit — they will call the base dtor automatically; no extra change needed | Yes — confirmed both subclasses hold no additional OGR handles beyond those `Georeferenced` owns | +| `VectorDataset::open` RAII-closes `GDALDataset*` | Existing behaviour (close on scope exit of `open()`) is unchanged — dataset was only needed for the duration of that call | Yes | + +## Open Questions + +- None — fix approach is unambiguous; valgrind harness is the implementer's responsibility (run before PR). + +## Estimated Scope + +Single PR. diff --git a/.agent/work-plans/issue-152/progress.md b/.agent/work-plans/issue-152/progress.md new file mode 100644 index 0000000..c7d6d82 --- /dev/null +++ b/.agent/work-plans/issue-152/progress.md @@ -0,0 +1,110 @@ +--- +issue: 152 +--- + +# Issue #152 — Fix GDAL/OGR handle leaks in Georeferenced / VectorDataset chart loading + +## Issue Review +**Status**: complete +**When**: 2026-06-30 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #152 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Summary + +Confirmed GDAL/OGR memory leaks in `camp` project repo, specifically: +1. `Georeferenced` — missing destructor; two `OGRCoordinateTransformation*` members allocated in `extractGeoreference()` (lines 48–49 of `georeferenced.cpp`) are never freed. Every `DepthRaster` and `VectorDataset` instance leaks ~180 KB of OGR/PROJ state. +2. `VectorDataset::open` — `GDALDataset*` from `GDALOpenEx` is never `GDALClose`d; per-layer `OGRCoordinateTransformation` is never destroyed; `OGRPointIterator*` leaked at all three geometry branches (linestring line 72, polygon exterior ring line 100, polygon interior ring line 119). + +The leaks are well-evidenced (valgrind confirmed). The fix is clearly scoped to two files (`georeferenced.{h,cpp}` and `vector/vectordataset.cpp`) and is completable in a single PR. + +### Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| Human control and transparency | OK | Issue is well-documented with valgrind stacks; proposed fix is unambiguous | +| Enforcement over documentation | Watch | No automated memory-leak CI check exists; valgrind re-run is manual. Acceptable for this codebase, but worth noting | +| Capture decisions, not just implementations | Watch | Adding a `virtual` destructor to `Georeferenced` is a minor design decision (vtable impact); the rationale should appear in the PR description | +| A change includes its consequences | Action needed | Re-run valgrind after the fix and include output in the PR to prove the leak is closed; explicitly confirm `DepthRaster` has no direct GDAL handle members beyond those `Georeferenced` holds | +| Only what's needed | OK | Fix is minimal and targeted; no scope creep | +| Improve incrementally | OK | Small, reviewable single-PR change | +| Test what breaks | Watch | No automated regression for this leak; valgrind harness is manual. Acceptable given the project's test maturity | +| Workspace vs. project separation | OK | Correctly scoped to the `camp` project repo | + +### ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| ADR-0002 — Worktree isolation | Yes | Worktree already exists (`issue-camp-152`) | +| ADR-0008 — ROS 2 conventions | Marginal | C++ source only; no package interface changes; follow existing RAII patterns in this package | +| ADR-0013 — progress.md vocabulary | Yes | This entry | + +### Consequences + +- `DepthRaster` inherits `Georeferenced` and will gain the virtual destructor. Confirmed it holds no direct GDAL handle members beyond those `Georeferenced` owns — no additional fix needed there. +- The `gdal_closer` lambda pattern (unique_ptr + custom deleter) is already established in `camp_map/raster/raster_layer.cpp:176–178`. The implementer should reuse or reference that pattern for `VectorDataset::open`'s `GDALDataset*` to keep the codebase consistent. + +### Actions +- [ ] Re-run valgrind after the fix and include "definitely lost" output in the PR (proves the leak is closed from `Georeferenced::extractGeoreference` frames) +- [ ] Add virtual destructor to `Georeferenced`; include "why virtual" rationale in PR description (base class with polymorphic subclasses — deleting through base pointer would be UB without it) +- [ ] In `VectorDataset::open`: use the `gdal_closer` unique_ptr pattern (already in `raster_layer.cpp:176`) for the `GDALDataset*`; destroy the per-layer `OGRCoordinateTransformation`; call `OGRPointIterator::destroy(pi)` at all three iterator sites +- [ ] Confirm `DepthRaster` has no direct GDAL handle members that also need cleanup (quick grep shows it does not, but verify during implementation) + +## Plan Authored +**Status**: complete +**When**: 2026-06-30 14:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-152/plan.md` at `40d4749` +**Branch**: feature/issue-152 at `40d4749` +**Phases**: single + +### Open questions +- [ ] No open questions — plan is review-plan-ready. + +## Plan Review +**Status**: complete +**When**: 2026-06-30 15:15 +00:00 +**By**: Claude Code Agent (Claude Opus) + + +**Plan**: `.agent/work-plans/issue-152/plan.md` at `40d4749` +**PR**: PR-less (`gh` unauthenticated in this environment; reviewed local plan file) +**Verdict**: approve-with-suggestions + +### Findings +- [ ] (must-fix) `test_vector_dataset_cleanup` has no library to link — `vectordataset.cpp`/`group.cpp`/`autonomousvehicleproject.cpp` compile only into the `CCOMAutonomousMissionPlanner` executable, and `open()` drags the full QGraphics item graph + `AutonomousVehicleProject` (mission_manager, platform_manager, map/raster). Settle the build strategy (bounded source list vs. valgrind-only descope) before implementing — `plan.md:26,36,37` +- [ ] (must-fix) In `VectorDataset::open`, destroy the per-layer `unprojectTransformation` at end-of-layer (else all but the last layer leak), and destroy the interior-ring `OGRPointIterator` inside the ring loop (`:119`, reassigned per ring) — `plan.md:34` +- [ ] (suggestion) Automated tests cover only the GDAL-dataset leak; `GetOpenDatasets()` does not see the `OGRCoordinateTransformation` handles that are the primary ~180 KB `Georeferenced` leak (valgrind-only). The `DepthRaster` test guards the pre-existing `GDALClose`, not this PR's dtor. State this in test comments so the names don't mislead — `plan.md:25,26,45` +- [ ] (suggestion) Rule of three: `= delete` copy ctor/assignment on `Georeferenced` when adding the owning dtor (latent double-free if ever copied; currently heap-only so no live bug) — `plan.md:23,32` + +### Notes +- Source fixes verified against actual code: all five leak sites confirmed real and correctly located (`georeferenced.cpp:48-49`; `vectordataset.cpp:22,35,72,100,119`). Approach correctly reuses the `gdal_closer` RAII pattern (`raster_layer.cpp:176`) and the `GetOpenDatasets()` baseline-delta test pattern (`test_raster_layer_gdal_cleanup.cpp`). +- `virtual ~Georeferenced()` is not strictly required (no call site deletes through `Georeferenced*`) but is a sound defensive choice; review-issue already tracks the PR rationale. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-30 16:39 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-152 at `550164a` +**Mode**: pre-push +**Depth**: Deep (reason: memory/resource-lifecycle fix; polymorphic base-class change affecting DepthRaster + VectorDataset) +**Must-fix**: 0 | **Suggestions**: 3 +**Round**: 1 | **Ship**: recommended — zero must-fix; refactor verified behavior-preserving and leak-free on every path by two adversarial passes + +### Findings +- [ ] (suggestion) Attach valgrind `--leak-check=full` output (`definitely lost: 0`) to the PR — the automated tests only see the GDAL dataset handle via `GetOpenDatasets()`, not the primary ~180 KB `OGRCoordinateTransformation` leak the new dtor frees — `georeferenced.cpp:14`, `test/test_vector_dataset_cleanup.cpp` +- [ ] (suggestion) Soften the `~Georeferenced` comment: `DepthRaster` is deleted as `DepthRaster*` and `VectorDataset` via the QObject chain — neither is delete-through-`Georeferenced*`, so the stated risk is overstated (the `virtual` is fine as defensive future-proofing) — `georeferenced.h:13` +- [ ] (suggestion) Optional RAII parity: the per-layer `unprojectTransformation` raw pointer is held across the feature loop and its end-of-layer `DestroyCT` is not exception-safe (OOM-only); wrap it in `unique_ptr` like the dataset closer — `src/camp/vector/vector_parse.cpp:49` + +### Notes +- Static analysis: cppcheck clean (one const-pointer style nit only); ament_cpplint findings (header-guard style, trailing whitespace) are all pre-existing package style confirmed via `git blame` (2017–2021), not introduced here. +- Two fresh-context Claude adversarial passes (Lens A logic / Lens B systemic-lifecycle) independently traced every GDAL/OGR handle and confirmed: all coordinate handling preserved verbatim (incl. the Point `(Y,X)` vs Line/Polygon `(X,Y)` axis quirk), every handle freed on every path, no double-free, no new leak, `=delete` copy ops break no call site (both subclasses heap-only via `new`), and the constructor zero-inits both transformation pointers so the dtor null-checks are safe. +- Full plan adherence: both review-plan must-fixes (end-of-layer transform destroy; per-ring iterator destroy) and both suggestions (rule-of-three `=delete`; honest test comments) are folded into the implementation. The `vector_parse` testable-seam extraction resolves the plan-review must-fix about the test having no linkable library. diff --git a/CMakeLists.txt b/CMakeLists.txt index 6ba4c52..8893d80 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -97,6 +97,7 @@ set(SOURCES src/camp/waypointdetails.cpp src/camp/vector/vectordataset.cpp + src/camp/vector/vector_parse.cpp src/camp/group.cpp src/camp/vector/point.cpp src/camp/vector/linestring.cpp @@ -497,6 +498,28 @@ if(BUILD_TESTING) ${GDAL_LIBRARY} ) + # [camp#152] VectorDataset GDAL/OGR handle-leak regression. parseVectorLayers + # creates and destroys each layer's OGRCoordinateTransformation and every + # OGRPointIterator and must leave the process-wide open-dataset count at + # baseline. NOTE: GetOpenDatasets() can see the GDAL *dataset* handle but NOT + # the OGRCoordinateTransformation pipelines that are the primary ~180 KB leak — + # those are proven only under valgrind (run this binary with + # `valgrind --leak-check=full`; "definitely lost" must be 0). Linked against + # the standalone vector_parse TU so the leak path is exercised without the + # MissionItem / AutonomousVehicleProject item graph. + ament_add_gtest(test_vector_dataset_cleanup + test/test_vector_dataset_cleanup.cpp + src/camp/vector/vector_parse.cpp + ) + target_include_directories(test_vector_dataset_cleanup PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/src/camp + ) + target_link_libraries(test_vector_dataset_cleanup + Qt5::Core + Qt5::Positioning + ${GDAL_LIBRARY} + ) + # A* shoal avoidance + unknown-depth-as-obstacle (#59 PR6 — depth/A* decoupled # from BackgroundRaster). Pure std C++; no Qt/ROS link needed. ament_add_gtest(test_astar diff --git a/src/camp/georeferenced.cpp b/src/camp/georeferenced.cpp index dae3453..9588685 100644 --- a/src/camp/georeferenced.cpp +++ b/src/camp/georeferenced.cpp @@ -11,6 +11,17 @@ Georeferenced::Georeferenced(): m_geoTransform{0.0,1.0,0.0,0.0,0.0,1.0}, m_inver } +Georeferenced::~Georeferenced() +{ + // [#152] Free the PROJ-backed transformation pipelines allocated by + // extractGeoreference. Each is ~tens of KB of PROJ state; without this they + // leaked once per chart load (DepthRaster + VectorDataset both derive here). + if(m_projectTransformation) + OGRCoordinateTransformation::DestroyCT(m_projectTransformation); + if(m_unprojectTransformation) + OGRCoordinateTransformation::DestroyCT(m_unprojectTransformation); +} + QPointF Georeferenced::pixelToProjectedPoint(const QPointF &point) const { return QPointF(m_geoTransform[0]+point.x()*m_geoTransform[1]+point.y()*m_geoTransform[2], diff --git a/src/camp/georeferenced.h b/src/camp/georeferenced.h index 387fa11..86c79a9 100644 --- a/src/camp/georeferenced.h +++ b/src/camp/georeferenced.h @@ -10,6 +10,18 @@ class Georeferenced { public: Georeferenced(); + // [#152] Owns m_project/unprojectTransformation (created in + // extractGeoreference) and frees them here. virtual is defensive + // future-proofing: today neither subclass is deleted through a + // Georeferenced* (DepthRaster is deleted as DepthRaster*, VectorDataset via + // the QObject parent chain), so the dtor runs regardless — but making it + // virtual keeps delete-through-base correct if that ever changes. The + // =delete rule-of-three guards against a copy silently double-freeing those + // owned handles; both subclasses are heap-only today, so this removes a + // latent foot-gun without affecting any live call site. + virtual ~Georeferenced(); + Georeferenced(const Georeferenced &) = delete; + Georeferenced &operator=(const Georeferenced &) = delete; QPointF pixelToProjectedPoint(QPointF const &point) const; QPointF projectedPointToPixel(QPointF const &point) const; QPointF project(QGeoCoordinate const &point) const; diff --git a/src/camp/vector/vector_parse.cpp b/src/camp/vector/vector_parse.cpp new file mode 100644 index 0000000..5e4195b --- /dev/null +++ b/src/camp/vector/vector_parse.cpp @@ -0,0 +1,155 @@ +#include "vector_parse.h" + +#include + +#include +#include + +namespace camp::vector +{ + +namespace +{ + +// RAII deleter for OGRCoordinateTransformation, so the per-layer transformation +// frees on every path (including an exception mid feature loop) — parity with +// the dataset's gdal_closer in VectorDataset::open. +struct OGRCTDeleter +{ + void operator()(OGRCoordinateTransformation *ct) const + { + if(ct) + OGRCoordinateTransformation::DestroyCT(ct); + } +}; + +// Read one ring's vertices, transforming to WGS84, and destroy the iterator. +// [#152] The OGRPointIterator is always destroyed here — at every call site, +// including the interior-ring loop where the original code reassigned `pi` per +// ring and so leaked all but (at most) one. +std::vector readRing(const OGRCurve *ring, OGRCoordinateTransformation *unprojectTransformation) +{ + std::vector points; + if(!ring) + return points; + OGRPointIterator *pi = ring->getPointIterator(); + OGRPoint p; + while(pi->getNextPoint(&p)) + { + if(unprojectTransformation) + { + double x = p.getX(); + double y = p.getY(); + unprojectTransformation->Transform(1, &x, &y); + p.setX(x); + p.setY(y); + } + points.emplace_back(p.getX(), p.getY()); + } + OGRPointIterator::destroy(pi); + return points; +} + +} // namespace + +std::vector parseVectorLayers(GDALDataset *dataset) +{ + std::vector result; + if(!dataset) + return result; + + for(int i = 0; i < dataset->GetLayerCount(); ++i) + { + OGRLayer *layer = dataset->GetLayer(i); + if(!layer) + continue; + + // [#152] One transformation per layer, freed by RAII at end-of-layer. + // The original destroyed none, leaking one PROJ pipeline per layer. + std::unique_ptr unprojectTransformation; + if(OGRSpatialReference *projected = layer->GetSpatialRef()) + { + OGRSpatialReference wgs84; + wgs84.SetWellKnownGeogCS("WGS84"); + unprojectTransformation.reset(OGRCreateCoordinateTransformation(projected, &wgs84)); + } + + ParsedLayer parsed; + parsed.name = layer->GetName(); + + layer->ResetReading(); + OGRFeature *feature = layer->GetNextFeature(); + while(feature) + { + if(OGRGeometry *geometry = feature->GetGeometryRef()) + { + switch(geometry->getGeometryType()) + { + case wkbPoint: + { + OGRPoint *op = dynamic_cast(geometry); + if(op) + { + ParsedGeometry g; + g.type = ParsedGeometry::Point; + // Preserve original coordinate handling exactly: the + // untransformed point is (lat=Y, lon=X); a transformed + // point uses the (x,y) output as (lat, lon). + QGeoCoordinate location(op->getY(), op->getX()); + if(unprojectTransformation) + { + double x = op->getX(); + double y = op->getY(); + unprojectTransformation->Transform(1, &x, &y); + location.setLatitude(x); + location.setLongitude(y); + } + g.exterior.push_back(location); + parsed.geometries.push_back(std::move(g)); + } + break; + } + case wkbLineString: + { + OGRLineString *ols = dynamic_cast(geometry); + if(ols) + { + ParsedGeometry g; + g.type = ParsedGeometry::LineString; + g.exterior = readRing(ols, unprojectTransformation.get()); + parsed.geometries.push_back(std::move(g)); + } + break; + } + case wkbPolygon: + { + OGRPolygon *op = dynamic_cast(geometry); + // Match the original: only emit a polygon when it has an + // exterior ring. + if(op && op->getExteriorRing()) + { + ParsedGeometry g; + g.type = ParsedGeometry::Polygon; + g.exterior = readRing(op->getExteriorRing(), unprojectTransformation.get()); + for(int ringNum = 0; ringNum < op->getNumInteriorRings(); ++ringNum) + g.interiorRings.push_back(readRing(op->getInteriorRing(ringNum), unprojectTransformation.get())); + parsed.geometries.push_back(std::move(g)); + } + break; + } + default: + break; + } + } + OGRFeature::DestroyFeature(feature); + feature = layer->GetNextFeature(); + } + + // unprojectTransformation's RAII deleter frees it here. + result.push_back(std::move(parsed)); + } + + return result; +} + +} // namespace camp::vector diff --git a/src/camp/vector/vector_parse.h b/src/camp/vector/vector_parse.h new file mode 100644 index 0000000..373f871 --- /dev/null +++ b/src/camp/vector/vector_parse.h @@ -0,0 +1,50 @@ +#ifndef CAMP_VECTOR_PARSE_H +#define CAMP_VECTOR_PARSE_H + +#include + +#include +#include + +class GDALDataset; + +namespace camp::vector +{ + +// Plain-data geometry parsed from an OGR vector source, coordinates already in +// WGS84. Deliberately free of MissionItem / AutonomousVehicleProject coupling: +// VectorDataset::open historically interleaved the GDAL/OGR resource lifecycle +// (per-layer coordinate transformations + point iterators — the issue #152 leak +// sites) with construction of the project item graph, which made the leak path +// impossible to exercise without standing up the whole application. Splitting +// the pure parse into this standalone TU lets a headless unit test reach it. +struct ParsedGeometry +{ + enum Type { Point, LineString, Polygon }; + Type type; + // Point: exactly one coordinate. LineString / Polygon exterior ring: the + // ordered vertices. + std::vector exterior; + // Polygon only: each interior ring's vertices (empty for Point/LineString). + std::vector> interiorRings; +}; + +struct ParsedLayer +{ + QString name; + std::vector geometries; +}; + +// Parse every layer of an already-open OGR dataset into WGS84 plain data. +// +// [#152] For each layer this creates an OGRCoordinateTransformation and, per +// geometry, OGRPointIterators — and DESTROYS every one of them before +// returning (the transformation at end-of-layer, each iterator at end-of-use). +// OGRFeatures are freed via DestroyFeature. The caller retains ownership of +// `dataset` (open/close is the caller's responsibility); this function opens no +// dataset of its own. +std::vector parseVectorLayers(GDALDataset *dataset); + +} // namespace camp::vector + +#endif // CAMP_VECTOR_PARSE_H diff --git a/src/camp/vector/vectordataset.cpp b/src/camp/vector/vectordataset.cpp index 2836528..e90fee6 100644 --- a/src/camp/vector/vectordataset.cpp +++ b/src/camp/vector/vectordataset.cpp @@ -1,9 +1,11 @@ #include "vectordataset.h" +#include #include #include "group.h" #include "point.h" #include "linestring.h" #include "polygon.h" +#include "vector_parse.h" #include "autonomousvehicleproject.h" #include #include @@ -19,128 +21,66 @@ void VectorDataset::open(const QString& fname) if(!fname.isEmpty()) m_filename = fname; - GDALDataset * dataset = reinterpret_cast(GDALOpenEx(m_filename.toStdString().c_str(),GDAL_OF_READONLY,nullptr,nullptr,nullptr)); - if (dataset) + // [#152] RAII-close the dataset on every return path. Previously the handle + // from GDALOpenEx was never GDALClose'd, leaking it on each chart load. + const auto gdal_closer = [](GDALDataset* d){ if(d) GDALClose(d); }; + std::unique_ptr dataset( + reinterpret_cast(GDALOpenEx(m_filename.toStdString().c_str(), GDAL_OF_READONLY, nullptr, nullptr, nullptr)), + gdal_closer); + if(!dataset) + return; + + extractGeoreference(dataset.get()); + buildItems(camp::vector::parseVectorLayers(dataset.get())); +} + +void VectorDataset::buildItems(const std::vector& layers) +{ + for(const auto& parsedLayer : layers) { - extractGeoreference(dataset); - for(int i = 0; i < dataset->GetLayerCount(); ++i) + Group *group = new Group(this); + group->setObjectName(parsedLayer.name); + for(const auto& geometry : parsedLayer.geometries) { - OGRLayer *layer = dataset->GetLayer(i); - OGRCoordinateTransformation *unprojectTransformation = nullptr; - OGRSpatialReference *projected = layer->GetSpatialRef(); - if(projected) + switch(geometry.type) + { + case camp::vector::ParsedGeometry::Point: { - OGRSpatialReference wgs84; - wgs84.SetWellKnownGeogCS("WGS84"); - unprojectTransformation = OGRCreateCoordinateTransformation(projected,&wgs84); + Point *p = new Point(group); + if(!geometry.exterior.empty()) + p->setLocation(geometry.exterior.front()); + p->setObjectName("point"); + p->lock(); + connect(autonomousVehicleProject(),&AutonomousVehicleProject::updatingBackground,p,&Point::updateBackground); + break; } - - Group *group = new Group(this); - group->setObjectName(layer->GetName()); - layer->ResetReading(); - OGRFeature * feature = layer->GetNextFeature(); - while(feature) + case camp::vector::ParsedGeometry::LineString: + { + LineString *ls = new LineString(group); + ls->setObjectName("lineString"); + for(const auto& location : geometry.exterior) + ls->addPoint(location); + ls->lock(); + connect(autonomousVehicleProject(),&AutonomousVehicleProject::updatingBackground, ls, &LineString::updateBackground); + break; + } + case camp::vector::ParsedGeometry::Polygon: { - OGRGeometry * geometry = feature->GetGeometryRef(); - if(geometry) + Polygon *p = new Polygon(group); + p->setObjectName("polygon"); + for(const auto& location : geometry.exterior) + p->addExteriorPoint(location); + for(const auto& ring : geometry.interiorRings) { - OGRwkbGeometryType gtype = geometry->getGeometryType(); - if(gtype == wkbPoint) - { - OGRPoint *op = dynamic_cast(geometry); - Point *p = new Point(group); - QGeoCoordinate location(op->getY(),op->getX()); - if(unprojectTransformation) - { - double x = op->getX(); - double y = op->getY(); - unprojectTransformation->Transform(1,&x,&y); - location.setLatitude(x); - location.setLongitude(y); - } - p->setLocation(location); - p->setObjectName("point"); - p->lock(); - connect(autonomousVehicleProject(),&AutonomousVehicleProject::updatingBackground,p,&Point::updateBackground); - - } - else if(gtype == wkbLineString) - { - OGRLineString *ols = dynamic_cast(geometry); - LineString *ls = new LineString(group); - ls->setObjectName("lineString"); - OGRPointIterator *pi = ols->getPointIterator(); - OGRPoint p; - while(pi->getNextPoint(&p)) - { - if(unprojectTransformation) - { - double x = p.getX(); - double y = p.getY(); - unprojectTransformation->Transform(1,&x,&y); - p.setX(x); - p.setY(y); - } - QGeoCoordinate location(p.getX(),p.getY()); - ls->addPoint(location); - } - ls->lock(); - connect(autonomousVehicleProject(),&AutonomousVehicleProject::updatingBackground, ls, &LineString::updateBackground); - - } - else if(gtype == wkbPolygon) - { - OGRPolygon *op = dynamic_cast(geometry); - OGRLinearRing *lr = op->getExteriorRing(); - if(lr) - { - Polygon *p = new Polygon(group); - p->setObjectName("polygon"); - qDebug() << "polygon exterior ring point count " << lr->getNumPoints(); - OGRPointIterator *pi = lr->getPointIterator(); - OGRPoint pt; - while(pi->getNextPoint(&pt)) - { - if(unprojectTransformation) - { - double x = pt.getX(); - double y = pt.getY(); - unprojectTransformation->Transform(1,&x,&y); - pt.setX(x); - pt.setY(y); - } - QGeoCoordinate location(pt.getX(),pt.getY()); - p->addExteriorPoint(location); - } - for(int ringNum = 0; ringNum < op->getNumInteriorRings(); ringNum++) - { - p->addInteriorRing(); - lr = op->getInteriorRing(ringNum); - pi = lr->getPointIterator(); - while(pi->getNextPoint(&pt)) - { - if(unprojectTransformation) - { - double x = pt.getX(); - double y = pt.getY(); - unprojectTransformation->Transform(1,&x,&y); - pt.setX(x); - pt.setY(y); - } - QGeoCoordinate location(pt.getX(),pt.getY()); - p->addInteriorPoint(location); - } - } - p->updateBBox(); - p->lock(); - connect(autonomousVehicleProject(),&AutonomousVehicleProject::updatingBackground,p,&Polygon::updateBackground); - } - } - else - qDebug() << "type: " << gtype; + p->addInteriorRing(); + for(const auto& location : ring) + p->addInteriorPoint(location); } - OGRFeature::DestroyFeature(feature); - feature = layer->GetNextFeature(); + p->updateBBox(); + p->lock(); + connect(autonomousVehicleProject(),&AutonomousVehicleProject::updatingBackground,p,&Polygon::updateBackground); + break; + } } } } diff --git a/src/camp/vector/vectordataset.h b/src/camp/vector/vectordataset.h index 737cc85..7ebd6f7 100644 --- a/src/camp/vector/vectordataset.h +++ b/src/camp/vector/vectordataset.h @@ -1,9 +1,13 @@ #ifndef VECTORDATASET_H #define VECTORDATASET_H +#include + #include "../georeferenced.h" #include "../group.h" +namespace camp::vector { struct ParsedLayer; } + class VectorDataset :public Group, public Georeferenced { Q_OBJECT @@ -21,6 +25,12 @@ public slots: void updateProjectedPoints() override; private: + // Build the project item graph (Group/Point/LineString/Polygon + the + // updatingBackground connections) from already-parsed WGS84 geometry. The + // GDAL/OGR resource lifecycle lives in camp::vector::parseVectorLayers so it + // can be unit-tested without this MissionItem coupling (issue #152). + void buildItems(const std::vector &layers); + QString m_filename; }; diff --git a/test/test_depth_raster.cpp b/test/test_depth_raster.cpp index 9ecbd6b..812bc55 100644 --- a/test/test_depth_raster.cpp +++ b/test/test_depth_raster.cpp @@ -8,11 +8,56 @@ #include #include +#include + +#include +#include #include +#include #include "depth_raster.h" +namespace +{ + +// Minimal valid WGS84 single-band Float32 GeoTIFF so DepthRaster's load runs its +// full GDAL open + georeference path (and so the open-dataset count below moves). +QString writeDepthRaster(const QTemporaryDir& dir) +{ + GDALAllRegister(); + GDALDriver* driver = GetGDALDriverManager()->GetDriverByName("GTiff"); + if(!driver) + return QString(); + const QString path = dir.filePath("depth.tif"); + const int w = 8, h = 8; + GDALDataset* ds = driver->Create(path.toUtf8().constData(), w, h, 1, GDT_Float32, nullptr); + if(!ds) + return QString(); + double geo[6] = {-70.80, 0.0001, 0.0, 43.20, 0.0, -0.0001}; + ds->SetGeoTransform(geo); + OGRSpatialReference srs; + srs.SetWellKnownGeogCS("WGS84"); + char* wkt = nullptr; + srs.exportToWkt(&wkt); + ds->SetProjection(wkt); + CPLFree(wkt); + std::vector samples(static_cast(w) * h, -5.0f); + GDALRasterBand* band = ds->GetRasterBand(1); + const CPLErr err = band->RasterIO(GF_Write, 0, 0, w, h, samples.data(), w, h, GDT_Float32, 0, 0); + GDALClose(ds); + return err == CE_None ? path : QString(); +} + +int openDatasetCount() +{ + int count = 0; + GDALDataset::GetOpenDatasets(&count); + return count; +} + +} // namespace + // A file that does not exist must not crash and must report no depth. TEST(DepthRasterTest, NonexistentFileHasNoDepth) { @@ -33,6 +78,31 @@ TEST(DepthRasterTest, InvalidRasterReturnsNanDepth) EXPECT_TRUE(std::isnan(dr.getDepth(QGeoCoordinate(43.07, -70.71)))); } +// [camp#152] DepthRaster's ctor opens a GDAL dataset and must GDALClose it +// (depth_raster.cpp). Loading then destroying the provider must leave the +// process-wide open-dataset count at its baseline. NOTE: this guards the +// pre-existing GDALClose, NOT this PR's new ~Georeferenced() — the dtor frees +// the OGRCoordinateTransformation pipelines, which GetOpenDatasets() cannot see; +// that ~180 KB leak is proven only under valgrind. A valid load is asserted so +// the check is not vacuous (a failed load opens nothing). +TEST(DepthRasterTest, LoadAndDestroyLeavesNoOpenDataset) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString path = writeDepthRaster(dir); + ASSERT_FALSE(path.isEmpty()); + + const int baseline = openDatasetCount(); + { + DepthRaster dr(path); + ASSERT_TRUE(dr.depthValid()); // non-vacuous: the load actually opened a dataset + // The ctor already opened AND closed the source dataset. + EXPECT_EQ(openDatasetCount(), baseline); + } + // After destruction the count is still at baseline. + EXPECT_EQ(openDatasetCount(), baseline); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); diff --git a/test/test_vector_dataset_cleanup.cpp b/test/test_vector_dataset_cleanup.cpp new file mode 100644 index 0000000..0b94bf1 --- /dev/null +++ b/test/test_vector_dataset_cleanup.cpp @@ -0,0 +1,192 @@ +// [camp#152] Regression test for the GDAL/OGR handle leaks in VectorDataset +// chart loading. The leak-prone resource lifecycle — opening the dataset, +// creating one OGRCoordinateTransformation per layer, and creating an +// OGRPointIterator per ring — was historically interleaved with construction of +// the project item graph (Group/Point/LineString/Polygon + AutonomousVehicle- +// Project signal connections), which made it impossible to exercise without +// standing up the whole application. The fix splits the pure parse into +// camp::vector::parseVectorLayers (a MissionItem-free TU), which this test links +// directly and drives headless. +// +// Coverage: +// - GDAL *dataset* handle: asserted via GDALDataset::GetOpenDatasets() — a +// baseline-delta (the count returns to what it was before the test opened its +// own handle), proving parseVectorLayers opens/leaks no dataset of its own. +// - OGRCoordinateTransformation + OGRPointIterator: these are NOT visible to +// GetOpenDatasets() and are the primary ~180 KB leak. They are proven closed +// only under valgrind — run this binary with `valgrind --leak-check=full` and +// require "definitely lost: 0 bytes". The asserts below also force every +// leak site to execute (multi-layer, points, linestrings, polygon interior +// rings) so a valgrind run is non-vacuous. + +#include + +#include +#include + +#include +#include +#include + +#include + +#include "vector/vector_parse.h" + +namespace +{ + +int openDatasetCount() +{ + int count = 0; + GDALDataset::GetOpenDatasets(&count); + return count; +} + +// Add a point, a linestring, and a polygon-with-one-hole to `layer` — one of +// each geometry type so every parse branch (and every OGRPointIterator site) +// runs. +void populateLayer(OGRLayer* layer) +{ + OGRFeatureDefn* defn = layer->GetLayerDefn(); + + { + OGRFeature* f = OGRFeature::CreateFeature(defn); + OGRPoint pt(-70.71, 43.07); + f->SetGeometry(&pt); + EXPECT_EQ(layer->CreateFeature(f), OGRERR_NONE); + OGRFeature::DestroyFeature(f); + } + { + OGRFeature* f = OGRFeature::CreateFeature(defn); + OGRLineString ls; + ls.addPoint(-70.70, 43.06); + ls.addPoint(-70.69, 43.05); + ls.addPoint(-70.68, 43.07); + f->SetGeometry(&ls); + EXPECT_EQ(layer->CreateFeature(f), OGRERR_NONE); + OGRFeature::DestroyFeature(f); + } + { + OGRFeature* f = OGRFeature::CreateFeature(defn); + OGRPolygon poly; + OGRLinearRing exterior; + exterior.addPoint(-70.80, 43.00); + exterior.addPoint(-70.60, 43.00); + exterior.addPoint(-70.60, 43.20); + exterior.addPoint(-70.80, 43.20); + exterior.closeRings(); + poly.addRing(&exterior); + OGRLinearRing hole; + hole.addPoint(-70.74, 43.06); + hole.addPoint(-70.66, 43.06); + hole.addPoint(-70.66, 43.14); + hole.addPoint(-70.74, 43.14); + hole.closeRings(); + poly.addRing(&hole); + f->SetGeometry(&poly); + EXPECT_EQ(layer->CreateFeature(f), OGRERR_NONE); + OGRFeature::DestroyFeature(f); + } +} + +// Write a 2-layer GeoPackage (each layer WGS84, each with the three geometry +// types) so the per-layer transform-destroy path runs more than once. +QString writeGeoPackage(const QTemporaryDir& dir) +{ + GDALAllRegister(); + GDALDriver* driver = GetGDALDriverManager()->GetDriverByName("GPKG"); + if(!driver) + return QString(); + + const QString path = dir.filePath("vectors.gpkg"); + GDALDataset* ds = driver->Create(path.toUtf8().constData(), 0, 0, 0, GDT_Unknown, nullptr); + if(!ds) + return QString(); + + OGRSpatialReference srs; + srs.SetWellKnownGeogCS("WGS84"); + srs.SetAxisMappingStrategy(OAMS_TRADITIONAL_GIS_ORDER); + + for(const char* name : {"alpha", "beta"}) + { + OGRLayer* layer = ds->CreateLayer(name, &srs, wkbUnknown, nullptr); + if(!layer) + { + GDALClose(ds); + return QString(); + } + populateLayer(layer); + } + + GDALClose(ds); + return path; +} + +} // namespace + +TEST(VectorDatasetCleanupTest, ParseLeavesNoOpenDataset) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString path = writeGeoPackage(dir); + ASSERT_FALSE(path.isEmpty()); + + // Baseline AFTER writeGeoPackage closed its writer handle and BEFORE this test + // opens its own — GDAL internals may hold unrelated handles. + const int baseline = openDatasetCount(); + + { + const auto gdal_closer = [](GDALDataset* d){ if(d) GDALClose(d); }; + std::unique_ptr dataset( + reinterpret_cast(GDALOpenEx(path.toUtf8().constData(), GDAL_OF_READONLY, nullptr, nullptr, nullptr)), + gdal_closer); + ASSERT_TRUE(dataset); + + // This test's own open handle is live now. + EXPECT_EQ(openDatasetCount(), baseline + 1); + + const std::vector layers = + camp::vector::parseVectorLayers(dataset.get()); + + // Non-vacuous: every parse branch produced what we wrote, so a valgrind run + // of this binary genuinely exercises all the transform/iterator leak sites. + ASSERT_EQ(layers.size(), 2u); + for(const auto& layer : layers) + { + ASSERT_EQ(layer.geometries.size(), 3u); + int points = 0, lines = 0, polygons = 0; + for(const auto& g : layer.geometries) + { + switch(g.type) + { + case camp::vector::ParsedGeometry::Point: + ++points; + EXPECT_EQ(g.exterior.size(), 1u); + break; + case camp::vector::ParsedGeometry::LineString: + ++lines; + EXPECT_EQ(g.exterior.size(), 3u); + break; + case camp::vector::ParsedGeometry::Polygon: + ++polygons; + EXPECT_GE(g.exterior.size(), 4u); + ASSERT_EQ(g.interiorRings.size(), 1u); // the hole — interior-ring iterator site + EXPECT_GE(g.interiorRings.front().size(), 4u); + break; + } + } + EXPECT_EQ(points, 1); + EXPECT_EQ(lines, 1); + EXPECT_EQ(polygons, 1); + } + } + + // The RAII dataset closed at scope exit; parseVectorLayers leaked no dataset. + EXPECT_EQ(openDatasetCount(), baseline); +} + +int main(int argc, char** argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}