Skip to content

Fix issue #10: Add null check for pGraph in cypher_execute()#12

Open
dirtyfilthy wants to merge 1 commit intoagentflare-ai:mainfrom
dirtyfilthy:fix/issue-10-cypher-execute-null-check_v2
Open

Fix issue #10: Add null check for pGraph in cypher_execute()#12
dirtyfilthy wants to merge 1 commit intoagentflare-ai:mainfrom
dirtyfilthy:fix/issue-10-cypher-execute-null-check_v2

Conversation

@dirtyfilthy
Copy link
Copy Markdown
Contributor

Description

This PR fixes a critical NULL pointer dereference bug in cypher_execute() that caused segmentation faults when the function was called before creating a graph virtual table.

Problem: The cypher_execute() SQL function accessed the global pGraph pointer without null validation. When called before CREATE VIRTUAL TABLE graph USING graph(), pGraph is NULL, causing segfaults when passed to functions that dereference it.

Solution: Added a null check for pGraph in cypherExecuteSqlFunc() that returns a clear error message instead of crashing.

Related Issues

Closes #10

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update
  • 🔧 Build/CI configuration change
  • ♻️ Code refactoring (no functional changes)
  • ⚡ Performance improvement
  • ✅ Test improvement

Changes Made

  • Added null check for pGraph in src/cypher/cypher-executor-sql.c before using it in cypherPlannerCreate() and cypherExecutorCreate()
  • Returns clear error message: "Graph virtual table not initialized. Create a graph table first using: CREATE VIRTUAL TABLE graph USING graph();"
  • Added comprehensive test case tests/test_cypher_execute_before_table.c to verify:
    • Error is returned (not segfault) when called before table creation
    • Normal operation works after table creation
    • Invalid queries return parse errors (not segfaults)
  • Updated tests/Makefile to include new test in BASIC_TESTS

Testing

  • All existing tests pass (make test)
  • Added new tests for new functionality
  • Tested with Valgrind for memory leaks
  • Tested with AddressSanitizer
  • Manual testing performed
  • Performance testing performed (if applicable)

Test Evidence

Before fix:

$ python3 -c "import sqlite3; conn = sqlite3.connect(':memory:'); conn.enable_load_extension(True); conn.load_extension('./build/libgraph'); conn.execute('SELECT cypher_execute(\"MATCH (n) RETURN n\")')"
Segmentation fault (SIGSEGV)

After fix:

$ cd tests && ./test_cypher_execute_before_table
✅ Extension loaded

Test 1: cypher_execute() before graph table creation
  Expected: Error message indicating graph table not initialized
  Actual: Error returned: Graph virtual table not initialized. Create a graph table first using: CREATE VIRTUAL TABLE graph USING graph();
✅ Correct error message returned (mentions graph/virtual table)

Test 2: Create graph virtual table
✅ Virtual table created

Test 3: cypher_execute() after graph table creation
  Expected: Query executes successfully
✅ Query executed successfully
   Result: []

Test 4: cypher_execute() with invalid query (after table creation)
  Expected: Parse/execution error, not segfault
✅ Error returned (as expected): Expected MATCH, CREATE, MERGE, SET, or DELETE near 'INVALID' at line 1 column 1

✅ All tests completed successfully
   The fix prevents segfaults when cypher_execute() is called
   before graph table creation.

Documentation

  • Updated API documentation
  • Updated README.md (if needed)
  • Updated CHANGELOG.md
  • Added code comments where necessary
  • Updated examples (if applicable)

Checklist

  • My code follows the project's code style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have checked for memory leaks
  • I have updated the documentation accordingly
  • My changes are compatible with the supported platforms (Linux x86_64 / aarch64)

Breaking Changes

N/A - This is a bug fix that prevents crashes. The behavior change is:

  • Before: Segfault when cypher_execute() called before table creation
  • After: Clear error message when cypher_execute() called before table creation

Performance Impact

  • No performance impact
  • Performance improved
  • Performance slightly degraded (justified because...)
  • Performance significantly degraded (needs discussion)

The null check adds a single pointer comparison, which has negligible performance impact.

Code Changes

Modified: src/cypher/cypher-executor-sql.c

Added null check after AST parsing and before using pGraph:

/* Check if graph virtual table is initialized */
if( !pGraph ) {
  sqlite3_result_error(context, 
    "Graph virtual table not initialized. Create a graph table first using: "
    "CREATE VIRTUAL TABLE graph USING graph();", -1);
  cypherParserDestroy(pParser);
  return;
}

Added: tests/test_cypher_execute_before_table.c

Comprehensive test case covering:

  1. Error handling when called before table creation
  2. Normal operation after table creation
  3. Invalid query handling

Additional Context

Root Cause: The global pGraph variable (defined in src/graph.c:27) is initialized to NULL and only set when CREATE VIRTUAL TABLE graph USING graph() is executed. The cypher_execute() function was using pGraph without validation.

Impact:

  • Severity: High - caused application crashes
  • Frequency: Occurs whenever cypher_execute() is called before graph table creation
  • Affected: Any code path executing Cypher queries before ensuring graph table exists

Related Code:

  • src/graph.c:27 - Global pGraph declaration
  • src/graph.c:34-36 - setGlobalGraph() function
  • src/graph-vtab.c:171 - setGlobalGraph() called in graphCreate()
  • src/graph-vtab.c:287 - setGlobalGraph() called in graphConnect()
  • src/cypher/cypher-executor-sql.c:15 - External reference to pGraph
  • src/cypher/cypher-executor-sql.c:75,109 - Usage of pGraph (now protected by null check)

Reviewer Notes

Please verify:

  1. Null check is placed correctly (after parsing, before using pGraph)
  2. Error message is clear and helpful
  3. Test case covers all scenarios (before table, after table, invalid queries)
  4. No segfaults occur when cypher_execute() is called before table creation
  5. Normal operation still works after table creation

NOTE: includes linker flags from the fix test infrastructure pr because i needed them to run the tests


For Maintainers:

  • Code review completed
  • All CI checks passing
  • Documentation reviewed
  • Version number updated (if needed)
  • CHANGELOG.md updated

…te()

- Add null check before using pGraph in cypherExecuteSqlFunc()
- Return clear error message when graph virtual table not initialized
- Add test case to verify fix prevents segfaults
- Test verifies error message and normal operation after table creation
- Update Makefile to include new test and necessary LDFLAGS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: NULL Pointer Dereference in cypher_execute()

2 participants