cockroachdb / cockroachdb/cockroach

geo/geos: GEOS-allocated memory leaked on three error paths

Open
#174,958 1 comment 0 reactions 0 assignees View on GitHub
C-bug O-agent T-spatial
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Summary:**

Three error paths in `pkg/geo/geos` leak GEOS-allocated memory. There is no `runtime.SetFinalizer` anywhere in the package, so every one of these is permanent for the life of the process, and the leaked bytes are invisible to Go memory accounting. All three fire on ordinary error returns, independent of any panic. Part of #174944.

**Findings:**

- `geos.Version`: uses `cStringToUnsafeGoBytes` with the comment "Returns a `const char*`, so we don't have to free anything", but `CR_GEOS_Version` on the C side `malloc`s a copy via `toGEOSString`. Every call leaks a few dozen bytes of C heap. Caller-driven and unbounded in aggregate.
- `geos.IsValidDetail`: `CR_GEOS_IsValidDetail` populates both `retReason` (malloc'd) and `retLocationEWKB` *even when it sets the error status*, but the Go side returns before reaching the two `cStringToSafeGoBytes` calls that would free them. Two leaked buffers per `ST_IsValidDetail` on a malformed geometry. This is the only C function in the package that writes output buffers on its error path.
- `geos.PrepareGeometry`: `CR_GEOS_Prepare` `new`s a `CR_GEOS_PreparedGeometry` and stores the source geometry whenever the WKB read succeeded, even if `GEOSPrepare_r` returned nullptr and set the error; the Go side returns `nil, err` and never receives the pointer, leaking the C++ wrapper plus the source `GEOSGeometry`.

**Note on scope:** `geos.cc` has no `try`/`catch`, so a genuine memory-safety bug or uncaught C++ exception in GEOS/PROJ produces a SIGSEGV or `std::terminate` rather than a Go panic. That is not recoverable by any Go-side mechanism, and it bounds what panic recovery can promise for geo-heavy work. These three items are the part that *is* fixable.

**Next Steps:**

- [ ] Use `cStringToSafeGoBytes` in `Version`
- [ ] Free both buffers on the `IsValidDetail` error path, or zero them in C before use
- [ ] Destroy the prepared-geometry wrapper on the `CR_GEOS_Prepare` error path
- [ ] Consider a `runtime.SetFinalizer` on `PreparedGeometry` as defence in depth

Epic: none

Jira issue: CRDB-68129

Contributor guide

Open the contributing guide

Research direction

Start in `pkg/geo/geos` by reading the Go implementations of `Version`, `IsValidDetail`, and `PrepareGeometry`, then trace their corresponding C functions `CR_GEOS_Version`, `CR_GEOS_IsValidDetail`, and `CR_GEOS_Prepare` in `geos.cc`. Check how the existing `cStringToSafeGoBytes` and cleanup paths work. Done means all three error paths release GEOS-allocated memory; the issue does not name tests to run.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, go
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
57/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.