cockroachdb / cockroachdb/cockroach
geo/geos: GEOS-allocated memory leaked on three error paths
- 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
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