Skip to content

Conversation

@Dwd-xenopz
Copy link

1. Pointer Safety and Type Alignment

The most critical fix addressed a direct memory corruption bug in the cursor fetching logic where a Python object was passed to a function expecting a C structure.

The Issue: Python Object vs. C Pointer Mismatch

In the original code for usqlite_cursor_fetchone and fetchmany, the stepExecute function was called with the raw mp_obj_t. On the RP2350, dereferencing this resulted in an invalid memory access (Hard Fault).

Original Code :

static mp_obj_t usqlite_cursor_fetchone(mp_obj_t self_in) {
    usqlite_cursor_t *self = MP_OBJ_TO_PTR(self_in);
    // ... logic ...
    if (self->rc == SQLITE_ROW) {
        stepExecute(self_in); // BUG: Passing mp_obj_t to C pointer argument
    }
    return result;
}

The Fix: Pointer Correction

The logic was updated to pass the extracted C structure pointer (self) instead of the Python object wrapper.

Fixed Code:

static mp_obj_t usqlite_cursor_fetchone(mp_obj_t self_in) {
    usqlite_cursor_t *self = MP_OBJ_TO_PTR(self_in);
    // ... logic ...
    if (self->rc == SQLITE_ROW) {
        stepExecute(self); // FIXED: Passing usqlite_cursor_t*
    }
    return result;
}

2. Transient Data Binding

I modified how Python strings and bytes are bound to SQL parameters to prevent "Use-After-Free" crashes caused by the MicroPython Garbage Collector (GC).

The Issue: Garbage Collection Race Conditions

The original code used NULL as the destructor for SQLite bindings, assuming the memory was static. However, MicroPython's GC can move or delete these objects during execution.

Original Code:

// Inside bindParameter function
return sqlite3_bind_text(stmt, index, (const char *)str, nstr, NULL); 
// ...
return sqlite3_bind_blob(stmt, index, bytes, nbytes, NULL);

The Fix: SQLITE_TRANSIENT ((void*)-1)

The destructor was changed to (void*)-1. This forces SQLite to make a private copy of the data immediately, insulating it from GC movements.

Fixed Code:

// Inside bindParameter function
return sqlite3_bind_text(stmt, index, (const char *)str, nstr, (void*)-1); 
// ...
return sqlite3_bind_blob(stmt, index, bytes, nbytes, (void*)-1);

3. Cursor Lifecycle and Memory Leaks

Resolved a major memory leak where cursors were never removed from the connection's internal tracking list.

The Issue: Zombie Cursor Tracking

The original usqlite_cursor_close function finalized the SQL statement but left the cursor entry in the connection's list, causing Out of Memory (OOM) errors over time.

The Fix: Deregistration Logic

Added a call to usqlite_connection_deregister to ensure the cursor is fully unlinked from the connection.

Fixed Code:

mp_obj_t usqlite_cursor_close(mp_obj_t self_in) {
    usqlite_cursor_t *self = (usqlite_cursor_t *)MP_OBJ_TO_PTR(self_in);
    if (self->connection) {
        usqlite_connection_deregister(self->connection, self_in); // FIXED
        self->connection = NULL;
    }
    if (self->stmt) {
        sqlite3_finalize(self->stmt);
        self->stmt = NULL;
    }
    return mp_const_none;
}

4. Connection Cleanup Synchronization

Overhauled the database closing sequence to prevent crashes during shutdown.

The Issue: Unsafe List Iteration

The original closing logic attempted to iterate through the cursor list using standard index increments. This is unsafe because closing a cursor now removes it from the list, shifting all subsequent indices.

The Fix: Safe While-Loop Iteration

The loop now always targets the first item (items[0]) while the list length is greater than zero.

Fixed Code:

static mp_obj_t usqlite_connection_close(mp_obj_t self_in) {
    // ...
    while (self->cursors.len > 0) {
        usqlite_cursor_close(self->cursors.items[0]); // FIXED
    }
    // ...
}

Detailed Analysis: Connection Memory Management

This section specifically addresses flaws found in usqlite_connection.c.

The Problem

  1. Array Index Bug: The loop intended to clear every cursor but hard-coded items[0] while iterating with i, leaving dangling pointers for all items except the first.

  2. Illegal GC Memory Free: The code used m_free on objects managed by the MicroPython GC. This corrupts heap metadata, leading to crashes during the next GC cycle.

Fixed Implementation

The corrected version uses the proper index, removes illegal m_free calls, and resets the list state properly.

static mp_obj_t usqlite_connection_close(mp_obj_t self_in) {
    usqlite_connection_t *self = (usqlite_connection_t *)MP_OBJ_TO_PTR(self_in);
    if (!self->db) {
        return mp_const_none;
    }

    // 1. Iterate through ALL cursors and close them
    for (size_t i = 0; i < self->cursors.len; i++) {
        mp_obj_t cursor = self->cursors.items[i];
        
        // 2. Set index to None to prevent dangling references
        self->cursors.items[i] = mp_const_none; 
        
        // 3. Close the SQLite statement inside the cursor
        usqlite_cursor_close(cursor);
        
        // We let the MicroPython GC handle the cleanup.
    }
    
    // 4. Reset the list length to 0
    self->cursors.len = 0;

    // 5. Safely close the SQLite database connection
    sqlite3_close(self->db);
    self->db = NULL;

    return mp_const_none;
}

All these changed have been performed with the thought of making it work on a raspberry pi pico 2W. Since the program was constantly freezing before this.

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.

1 participant