From 6af9a4b375242d04806e1b89565db80845295458 Mon Sep 17 00:00:00 2001 From: Ajit Pratap Singh Date: Wed, 21 Oct 2026 23:35:16 +0530 Subject: [PATCH] fix(cbinding): return caller-owned string from gosqlx_version; add Python CI gosqlx_version returned a sync.Once-cached C string while its doc comment, gosqlx_free's doc, and pygosqlx.version() all treat it as caller-owned. The second version() call in a process therefore freed the same pointer twice and macOS aborted with "pointer being freed was not allocated". - gosqlx_version now returns a fresh C.CString on every call, matching the documented contract of every other export; the cache is removed. - Go tests: getVersion frees its result; new TestCGosqlxVersion_CallerOwned calls gosqlx_version twice and frees both; TestCGosqlxFree_Nil. - pkg/cbinding/tests/test_ctypes.py declared results as c_char_p, so gosqlx_free received pointers to Python-owned bytes copies; use c_void_p and free the version string. - python: regression test calling version() repeatedly, __version__ / setup.py vs pyproject consistency tests; align __version__ with the declared 0.2.0; declare readme in pyproject so editable installs work with current setuptools. - .github/workflows/python.yml: build libgosqlx and run pytest plus the raw ctypes ABI tests on ubuntu/macos x Python 3.10/3.13. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RkzVu3JY9Y6BRWfjesWts9 --- .github/workflows/python.yml | 82 ++++++++++++++++++++++++++ pkg/cbinding/cbinding.go | 20 ++----- pkg/cbinding/cbinding_coverage_test.go | 22 +++++++ pkg/cbinding/cbinding_stress_test.go | 6 +- pkg/cbinding/cbinding_testhelpers.go | 27 ++++++++- pkg/cbinding/tests/test_ctypes.py | 38 ++++++++---- python/pygosqlx/__init__.py | 2 +- python/pyproject.toml | 1 + python/tests/test_integration.py | 12 ++++ python/tests/test_package_version.py | 47 +++++++++++++++ 10 files changed, 227 insertions(+), 30 deletions(-) create mode 100644 .github/workflows/python.yml create mode 100644 python/tests/test_package_version.py diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml new file mode 100644 index 000000000..536101731 --- /dev/null +++ b/.github/workflows/python.yml @@ -0,0 +1,82 @@ +name: Python Bindings + +on: + push: + branches: [ main ] + paths: + - 'pkg/**' + - 'python/**' + - 'go.mod' + - 'go.sum' + - '.github/workflows/python.yml' + pull_request: + branches: [ main ] + paths: + - 'pkg/**' + - 'python/**' + - 'go.mod' + - 'go.sum' + - '.github/workflows/python.yml' + +permissions: + contents: read + +concurrency: + group: python-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + pygosqlx: + name: pygosqlx (${{ matrix.os }}, Python ${{ matrix.python-version }}) + runs-on: ${{ matrix.os }} + timeout-minutes: 20 + strategy: + fail-fast: false + matrix: + os: [ ubuntu-latest, macos-latest ] + python-version: [ '3.10', '3.13' ] + + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + + - name: Set up Go + uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 + with: + go-version: '1.26' + check-latest: true + + - name: Set up Python + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: ${{ matrix.python-version }} + + - name: Build libgosqlx shared library + working-directory: pkg/cbinding + run: bash build.sh + + - name: Locate shared library + shell: bash + run: | + case "$(uname -s)" in + Darwin) ext=dylib ;; + *) ext=so ;; + esac + lib="$GITHUB_WORKSPACE/python/pygosqlx/lib/libgosqlx.$ext" + test -f "$lib" || { echo "::error::shared library not built at $lib"; exit 1; } + echo "GOSQLX_LIB_PATH=$lib" >> "$GITHUB_ENV" + # The raw ctypes ABI harness looks for the library next to pkg/cbinding. + cp "$lib" "pkg/cbinding/libgosqlx.$ext" + + - name: Install pygosqlx + run: | + python -m pip install --upgrade pip + python -m pip install -e "python[dev]" + + - name: Smoke test (version() twice must not double-free) + run: python -c "import pygosqlx as p; a = p.version(); b = p.version(); assert a == b, (a, b); print('libgosqlx', a, 'pygosqlx', p.__version__)" + + - name: Run pytest + run: python -m pytest python/tests -v -rs + + - name: Run raw ctypes ABI tests + run: python pkg/cbinding/tests/test_ctypes.py -v diff --git a/pkg/cbinding/cbinding.go b/pkg/cbinding/cbinding.go index 22be24872..ce7c3825d 100644 --- a/pkg/cbinding/cbinding.go +++ b/pkg/cbinding/cbinding.go @@ -45,7 +45,6 @@ import ( "encoding/json" "regexp" "strconv" - "sync" "unsafe" "github.com/ajitpratap0/GoSQLX/pkg/gosqlx" @@ -59,13 +58,6 @@ var VERSION = gosqlx.Version // errorPositionRegex matches "line X, column Y" patterns in error messages. var errorPositionRegex = regexp.MustCompile(`line\s+(\d+),\s*column\s+(\d+)`) -// cachedVersion is a process-lifetime C string for gosqlx_version(). -// Allocated once via sync.Once to avoid leaking memory on every call. -var ( - cachedVersion *C.char - cachedVersionOnce sync.Once -) - // ParseResult represents the result of parsing SQL. type ParseResult struct { Success bool `json:"success"` @@ -413,6 +405,7 @@ func gosqlx_extract_metadata(sql *C.char) *C.char { } // gosqlx_free releases memory allocated by the C binding functions. +// Passing NULL is a no-op (free(NULL) is defined by the C standard). // Every C string returned by gosqlx_parse, gosqlx_validate, gosqlx_format, // gosqlx_extract_tables, gosqlx_extract_columns, gosqlx_extract_functions, // gosqlx_extract_metadata, and gosqlx_version must be freed with this function @@ -429,15 +422,14 @@ func gosqlx_free(ptr *C.char) { // gosqlx_version returns the current GoSQLX library version as a C string. // The version follows semantic versioning (e.g., "1.8.0"). // -// Returns a C string with the version. The caller must free the returned -// pointer with gosqlx_free. +// Returns a newly allocated C string with the version on every call. Like every +// other gosqlx_* result, the caller owns it and must free it exactly once with +// gosqlx_free. (Earlier releases returned a process-wide cached pointer here, +// which made the documented free a double free on the second call.) // //export gosqlx_version func gosqlx_version() *C.char { - cachedVersionOnce.Do(func() { - cachedVersion = C.CString(VERSION) - }) - return cachedVersion + return C.CString(VERSION) } func main() {} diff --git a/pkg/cbinding/cbinding_coverage_test.go b/pkg/cbinding/cbinding_coverage_test.go index 369a48df2..1a33c3a34 100644 --- a/pkg/cbinding/cbinding_coverage_test.go +++ b/pkg/cbinding/cbinding_coverage_test.go @@ -364,6 +364,28 @@ func TestCGosqlxVersion_Idempotent(t *testing.T) { } } +// TestCGosqlxVersion_CallerOwned is the regression test for the double free +// hit by pygosqlx: gosqlx_version is documented as returning a caller-owned +// string that must be released with gosqlx_free, so two calls must yield two +// distinct allocations that can both be freed. +func TestCGosqlxVersion_CallerOwned(t *testing.T) { + v1, v2, distinct := versionTwiceFreed() + if !distinct { + t.Fatal("gosqlx_version returned the same pointer twice; freeing both would double free") + } + if v1 != v2 { + t.Errorf("gosqlx_version strings differ: %q vs %q", v1, v2) + } + if v1 != VERSION { + t.Errorf("gosqlx_version = %q, want %q", v1, VERSION) + } +} + +// TestCGosqlxFree_Nil verifies gosqlx_free(NULL) is a safe no-op. +func TestCGosqlxFree_Nil(t *testing.T) { + freeNil() +} + // --------------------------------------------------------------------------- // toJSON helper — tested directly (same package) // --------------------------------------------------------------------------- diff --git a/pkg/cbinding/cbinding_stress_test.go b/pkg/cbinding/cbinding_stress_test.go index af34b0eda..d35d18e01 100644 --- a/pkg/cbinding/cbinding_stress_test.go +++ b/pkg/cbinding/cbinding_stress_test.go @@ -157,7 +157,7 @@ func TestStressConcurrentAllFunctions(t *testing.T) { extractColumns(sql) extractFunctions(sql) extractMetadata(sql) - // gosqlx_version is a cached singleton — just read it. + // gosqlx_version returns a caller-owned string; getVersion frees it. getVersion() } }() @@ -259,10 +259,10 @@ func TestStressMixedValidInvalid(t *testing.T) { } // --------------------------------------------------------------------------- -// Version singleton is stable under concurrency +// Version is stable under concurrency (each call allocates and is freed) // --------------------------------------------------------------------------- -func TestStressVersionSingleton(t *testing.T) { +func TestStressVersionConcurrent(t *testing.T) { const goroutines = 100 var wg sync.WaitGroup versions := make(chan string, goroutines) diff --git a/pkg/cbinding/cbinding_testhelpers.go b/pkg/cbinding/cbinding_testhelpers.go index 28ee8f6aa..dc7b77f72 100644 --- a/pkg/cbinding/cbinding_testhelpers.go +++ b/pkg/cbinding/cbinding_testhelpers.go @@ -114,13 +114,36 @@ func extractMetadataRaw(sql string) string { return C.GoString(cResult) } -// getVersion calls the real gosqlx_version C export and returns the version string. -// The returned C string is a cached singleton and must NOT be freed. +// getVersion calls the real gosqlx_version C export and returns the version string, +// freeing the returned C string as the documented ownership contract requires. func getVersion() string { cResult := gosqlx_version() + defer gosqlx_free(cResult) return C.GoString(cResult) } +// versionTwiceFreed calls gosqlx_version twice, checks that each call returned a +// distinct caller-owned allocation, and frees both results with gosqlx_free. It +// returns both decoded strings and whether the two pointers were distinct. A +// regression to a cached pointer makes the second gosqlx_free a double free. +func versionTwiceFreed() (string, string, bool) { + p1 := gosqlx_version() + p2 := gosqlx_version() + distinct := p1 != p2 + v1 := C.GoString(p1) + v2 := C.GoString(p2) + gosqlx_free(p1) + if distinct { + gosqlx_free(p2) + } + return v1, v2, distinct +} + +// freeNil calls gosqlx_free with a NULL pointer, which must be a no-op. +func freeNil() { + gosqlx_free(nil) +} + // toJSONString calls the real toJSON helper and returns the JSON as a Go string, // freeing the intermediate C string. func toJSONString(v interface{}) string { diff --git a/pkg/cbinding/tests/test_ctypes.py b/pkg/cbinding/tests/test_ctypes.py index 733572ab0..d18d42aa6 100644 --- a/pkg/cbinding/tests/test_ctypes.py +++ b/pkg/cbinding/tests/test_ctypes.py @@ -44,10 +44,18 @@ def _load_library(): for path in _LIB_PATHS: if os.path.exists(path): lib = ctypes.CDLL(path) + # Results are declared c_void_p (not c_char_p) so ctypes hands back + # the original malloc'd pointer. With c_char_p, ctypes converts the + # result to a Python bytes copy and gosqlx_free would then be given + # a pointer it never allocated. for fn_name in _RESULT_FUNCTIONS: fn = getattr(lib, fn_name, None) if fn is not None: - fn.restype = ctypes.c_char_p + fn.restype = ctypes.c_void_p + if fn_name != "gosqlx_version": + fn.argtypes = [ctypes.c_char_p] + lib.gosqlx_free.argtypes = [ctypes.c_void_p] + lib.gosqlx_free.restype = None return lib raise RuntimeError( "Could not find libgosqlx. Build it first with:\n" @@ -75,8 +83,10 @@ def _call(fn_name: str, sql: str) -> dict: result_ptr = fn(sql.encode("utf-8")) if result_ptr is None: raise RuntimeError(f"{fn_name} returned NULL") - result_json = ctypes.cast(result_ptr, ctypes.c_char_p).value.decode("utf-8") - _lib.gosqlx_free(result_ptr) + try: + result_json = ctypes.string_at(result_ptr).decode("utf-8") + finally: + _lib.gosqlx_free(result_ptr) return json.loads(result_json) @@ -230,12 +240,14 @@ def test_extract_functions_invalid_sql(self): @_skip_if_no_lib class TestGosqlxVersion(unittest.TestCase): - """Tests for gosqlx_version (cached singleton — must NOT be freed).""" + """Tests for gosqlx_version (caller-owned string — must be freed with gosqlx_free).""" def test_version_is_semver(self): - # Do NOT call gosqlx_free — the version is a cached singleton. result_ptr = _lib.gosqlx_version() - version = ctypes.cast(result_ptr, ctypes.c_char_p).value.decode("utf-8") + try: + version = ctypes.string_at(result_ptr).decode("utf-8") + finally: + _lib.gosqlx_free(result_ptr) self.assertGreater(len(version), 0) parts = version.split(".") self.assertEqual(len(parts), 3, f"Expected semver X.Y.Z, got: {version}") @@ -243,12 +255,18 @@ def test_version_is_semver(self): self.assertTrue(part.isdigit(), f"Non-numeric semver part: {part!r} in {version!r}") def test_version_is_stable(self): - """Calling gosqlx_version twice must return the same string.""" + """Two calls return equal strings in distinct allocations, both freeable.""" ptr1 = _lib.gosqlx_version() ptr2 = _lib.gosqlx_version() - v1 = ctypes.cast(ptr1, ctypes.c_char_p).value.decode("utf-8") - v2 = ctypes.cast(ptr2, ctypes.c_char_p).value.decode("utf-8") - self.assertEqual(v1, v2) + try: + self.assertNotEqual(ptr1, ptr2, "gosqlx_version must return a fresh allocation") + v1 = ctypes.string_at(ptr1).decode("utf-8") + v2 = ctypes.string_at(ptr2).decode("utf-8") + self.assertEqual(v1, v2) + finally: + _lib.gosqlx_free(ptr1) + if ptr2 != ptr1: + _lib.gosqlx_free(ptr2) if __name__ == "__main__": diff --git a/python/pygosqlx/__init__.py b/python/pygosqlx/__init__.py index 960872b17..a6836436d 100644 --- a/python/pygosqlx/__init__.py +++ b/python/pygosqlx/__init__.py @@ -63,7 +63,7 @@ ValidationError, ) -__version__ = "0.1.0" +__version__ = "0.2.0" __all__ = [ # Functions "parse", diff --git a/python/pyproject.toml b/python/pyproject.toml index 88651ad46..7e27bb890 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -6,6 +6,7 @@ build-backend = "setuptools.build_meta" name = "pygosqlx" version = "0.2.0" description = "Python bindings for GoSQLX - High-performance SQL parser" +readme = "README.md" requires-python = ">=3.8" license = {text = "Apache-2.0"} authors = [ diff --git a/python/tests/test_integration.py b/python/tests/test_integration.py index dc3ae759d..521f69d73 100644 --- a/python/tests/test_integration.py +++ b/python/tests/test_integration.py @@ -330,6 +330,18 @@ def test_version_format(self): parts = v.split(".") assert len(parts) >= 2, f"Expected semver-like version, got: {v}" + def test_version_called_twice(self): + """Regression: the 2nd version() call used to double-free and abort. + + gosqlx_version returned a process-wide cached pointer while version() + freed it, so the second call in a process crashed with "pointer being + freed was not allocated" on macOS (heap corruption elsewhere). + """ + first = version() + second = version() + third = version() + assert first == second == third + # --------------------------------------------------------------------------- # Error detail tests diff --git a/python/tests/test_package_version.py b/python/tests/test_package_version.py new file mode 100644 index 000000000..8f75364c7 --- /dev/null +++ b/python/tests/test_package_version.py @@ -0,0 +1,47 @@ +"""Package metadata consistency tests (no shared library required). + +pygosqlx declares its version in three places: pyproject.toml (the source of +truth used by pip/PyPI), setup.py, and ``pygosqlx.__version__``. These tests +keep them from drifting apart. +""" + +import os +import re +import sys + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +import pygosqlx + +_PYTHON_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..") + + +def _read(name: str) -> str: + with open(os.path.join(_PYTHON_DIR, name), encoding="utf-8") as f: + return f.read() + + +def _pyproject_version() -> str: + text = _read("pyproject.toml") + try: + import tomllib # Python 3.11+ + except ModuleNotFoundError: # pragma: no cover - Python < 3.11 + tomllib = None + if tomllib is not None: + return tomllib.loads(text)["project"]["version"] + # Fallback for Python < 3.11: first `version = "..."` inside [project]. + section = re.search(r"^\[project\]\s*$(.*?)(?=^\[|\Z)", text, re.M | re.S) + assert section, "pyproject.toml has no [project] table" + match = re.search(r'^version\s*=\s*"([^"]+)"', section.group(1), re.M) + assert match, "pyproject.toml [project] has no version" + return match.group(1) + + +def test_dunder_version_matches_pyproject(): + assert pygosqlx.__version__ == _pyproject_version() + + +def test_setup_py_version_matches_pyproject(): + match = re.search(r'version\s*=\s*"([^"]+)"', _read("setup.py")) + assert match, "setup.py has no version= argument" + assert match.group(1) == _pyproject_version()