diff --git a/sqlite3.carp b/sqlite3.carp index b271275..7d62f4c 100644 --- a/sqlite3.carp +++ b/sqlite3.carp @@ -118,6 +118,11 @@ primitive Carp types can be casted to appropriate SQLite types by using the (Fn [SQLiteColumn] (Array Byte)) "SQLiteColumn_from_blob") + (register delete (Fn [SQLiteColumn] ())) + (implements delete SQLite3.SQLiteColumn.delete) + (register copy (Fn [&SQLiteColumn] SQLiteColumn)) + (implements copy SQLite3.SQLiteColumn.copy) + (defn to-carp [c] (case (tag &c) sql_int @@ -151,7 +156,9 @@ primitive Carp types can be casted to appropriate SQLite types by using the (register ok? (Fn [&SQLiteRes] Bool) "SQLiteRes_is_ok") (register length (Fn [&SQLiteRes] Int) "SQLiteRes_length") (register nth (Fn [&SQLiteRes Int] SQLiteRow) "SQLiteRes_nth") - (register error (Fn [SQLiteRes] (Ptr CChar)) "SQLiteRes_error") + (register error (Fn [SQLiteRes] String) "SQLiteRes_error") + (register delete (Fn [SQLiteRes] ())) + (implements delete SQLite3.SQLiteRes.delete) (defn to-array [r] (let-do [l (length &r) @@ -193,7 +200,7 @@ If it fails, we return an error message using `Result.Error`.") &(Array.copy-map &(fn [x] (Type.to-sqlite3-internal @x)) p))] (if (SQLiteRes.ok? &r) (Result.Success (SQLiteRes.to-array r)) - (Result.Error (from-cstr (SQLiteRes.error r)))))) + (Result.Error (SQLiteRes.error r))))) (doc close "closes a database.") (register close (Fn [SQLite] ()) "SQLite3_close_c") @@ -235,7 +242,7 @@ Automatically resets the statement afterward for reuse.") p))] (if (SQLiteRes.ok? &r) (Result.Success (SQLiteRes.to-array r)) - (Result.Error (from-cstr (SQLiteRes.error r)))))) + (Result.Error (SQLiteRes.error r))))) (doc reset-stmt "manually resets a prepared statement and clears its bindings. Called automatically by [exec-prepared](#exec-prepared).") diff --git a/sqlite3_helper.h b/sqlite3_helper.h index bf0bb46..c97a373 100644 --- a/sqlite3_helper.h +++ b/sqlite3_helper.h @@ -73,6 +73,25 @@ Array SQLiteColumn_from_blob(SQLiteColumn col) { return res; } +/* delete/copy implement the Carp interfaces of the same name, so they carry the + * mangled path of the Carp binding (SQLite3.SQLiteColumn.delete) rather than the + * SQLiteColumn_* convention used by the directly-called bindings above. */ +void SQLite3_SQLiteColumn_delete(SQLiteColumn col) { + if ((col.tag == SQLITE_TEXT || col.tag == SQLITE_BLOB) && col.s) { + CARP_FREE(col.s); + } +} + +SQLiteColumn SQLite3_SQLiteColumn_copy(SQLiteColumn* col) { + SQLiteColumn res = *col; + if (col->s && (col->tag == SQLITE_TEXT || col->tag == SQLITE_BLOB)) { + int len = col->tag == SQLITE_TEXT ? (int)strlen(col->s) + 1 : col->blob_len; + res.s = CARP_MALLOC(len); + memcpy(res.s, col->s, len); + } + return res; +} + typedef struct { int columns; SQLiteColumn* data; @@ -117,6 +136,30 @@ SQLiteRows SQLiteRows_new_rows() { return res; } +/* Frees only the container arrays: the per-row column array and the row array + * itself. The per-column text/blob buffers are left alone — when a result is + * handed back to Carp those buffers are moved into Carp values. The row array + * is grown with realloc, so it is released with the matching free. */ +static void SQLiteRows_free_containers(SQLiteRows* rows) { + for (int i = 0; i < rows->len; i++) CARP_FREE(rows->rows[i].data); + free(rows->rows); +} + +/* Frees a result set that never reaches Carp (the error paths): the column + * buffers as well as the containers. */ +static void SQLiteRows_free_all(SQLiteRows* rows) { + for (int i = 0; i < rows->len; i++) { + SQLiteRow* row = rows->rows + i; + for (int j = 0; j < row->columns; j++) { + int tag = row->data[j].tag; + if ((tag == SQLITE_TEXT || tag == SQLITE_BLOB) && row->data[j].s) { + CARP_FREE(row->data[j].s); + } + } + } + SQLiteRows_free_containers(rows); +} + typedef struct { int is; union { @@ -143,6 +186,24 @@ char* SQLiteRes_error(SQLiteRes r) { return (char*)r.err; } +/* Called by Carp once a successful result has been turned into Carp values by + * to-array; the column buffers are owned by Carp at that point, so only the + * container arrays are freed here. */ +void SQLite3_SQLiteRes_delete(SQLiteRes r) { + if (r.is == OK) SQLiteRows_free_containers(&r.rows); +} + +/* sqlite3_errmsg returns memory owned by SQLite that is invalidated by the next + * call into the library (finalize, reset, …). Copy it into a Carp-owned string + * so it stays valid after we tear the statement down. */ +static char* SQLite3_copy_errmsg(const char* msg) { + if (!msg) msg = ""; + size_t len = strlen(msg); + char* copy = CARP_MALLOC(len + 1); + memcpy(copy, msg, len + 1); + return copy; +} + SQLite SQLite3_init() { SQLite res; res.handle = NULL; @@ -288,10 +349,11 @@ SQLiteRes SQLite3_exec_c(SQLite* db, const char* stmt, Array* p) { return res; err: + SQLiteRows_free_all(&res.rows); + res.is = ERR; + res.err = SQLite3_copy_errmsg(err); if (s) sqlite3_finalize(s); if (n) sqlite3_finalize(n); - res.is = ERR; - res.err = err; return res; } @@ -308,10 +370,7 @@ int SQLite3_changes(SQLite* db) { } char* SQLite3_error_and_close(SQLite db) { - const char* msg = sqlite3_errmsg(db.handle); - size_t len = strlen(msg); - char* copy = CARP_MALLOC(len + 1); - memcpy(copy, msg, len + 1); + char* copy = SQLite3_copy_errmsg(sqlite3_errmsg(db.handle)); sqlite3_close_v2(db.handle); return copy; } @@ -331,11 +390,7 @@ int SQLite3_prepare_c(SQLite* db, const char* sql, Stmt* stmt) { } char* SQLite3_errmsg_c(SQLite* db) { - const char* msg = sqlite3_errmsg(db->handle); - size_t len = strlen(msg); - char* copy = CARP_MALLOC(len + 1); - memcpy(copy, msg, len + 1); - return copy; + return SQLite3_copy_errmsg(sqlite3_errmsg(db->handle)); } SQLiteRes SQLite3_exec_prepared_c(Stmt* stmt, Array* p) { @@ -354,10 +409,11 @@ SQLiteRes SQLite3_exec_prepared_c(Stmt* stmt, Array* p) { return res; fail: + SQLiteRows_free_all(&res.rows); + res.is = ERR; + res.err = SQLite3_copy_errmsg(err); sqlite3_reset(stmt->handle); sqlite3_clear_bindings(stmt->handle); - res.is = ERR; - res.err = err; return res; } diff --git a/test/memory.carp b/test/memory.carp new file mode 100644 index 0000000..b1eba47 --- /dev/null +++ b/test/memory.carp @@ -0,0 +1,60 @@ +(load "../sqlite3.carp") +(load "Test.carp") +(use Test) +(use SQLite3) + +; Exercises the text/blob parameter path, the result path, and the error paths +; that historically leaked or read freed memory. Run with --log-memory so the +; allocation counter is live. +(defn workload [] + (match (open ":memory:") + (Result.Error _) () + (Result.Success db) + (do + (ignore (query &db "CREATE TABLE t (i INT, s TEXT, b BLOB);" &[])) + (for [i 0 20] + (ignore + (query &db + "INSERT INTO t VALUES (?1, ?2, ?3);" + &[(to-sqlite3 i) + (to-sqlite3 @"a text parameter") + (SQLite3.Type.Blob [1b 2b 0b 3b 4b])]))) + (ignore (query &db "SELECT * FROM t;" &[])) + (ignore (query &db "NOT VALID SQL" &[])) + (match (prepare &db "INSERT INTO t VALUES (?1, ?2, ?3);") + (Result.Success stmt) + (do + (for [i 0 20] + (ignore + (exec-prepared &stmt + &[(to-sqlite3 i) + (to-sqlite3 @"prepared text") + (SQLite3.Type.Blob [9b 8b 7b])]))) + (finalize-stmt stmt)) + (Result.Error _) ()) + (match (prepare &db "SELECT * FROM t WHERE i = ?1;") + (Result.Success stmt) + (do + (ignore (exec-prepared &stmt &[(to-sqlite3 3)])) + (finalize-stmt stmt)) + (Result.Error _) ()) + (ignore (query &db "CREATE TABLE u (x INT UNIQUE);" &[])) + (ignore (query &db "INSERT INTO u VALUES (1);" &[])) + (match (prepare &db "INSERT INTO u VALUES (?1);") + (Result.Success stmt) + (do + (ignore (exec-prepared &stmt &[(to-sqlite3 1)])) + (finalize-stmt stmt)) + (Result.Error _) ()) + (close db)))) + +(deftest test + (do + ; warm up so one-time allocations are settled before we sample the counter + (workload) + (let-do [before (Debug.memory-balance)] + (for [i 0 50] (workload)) + (assert-equal test + 0l + (- (Debug.memory-balance) before) + "no net allocations leak across 50 query/prepared workloads")))) diff --git a/test/sqlite3.carp b/test/sqlite3.carp index 0b54adc..105e129 100644 --- a/test/sqlite3.carp +++ b/test/sqlite3.carp @@ -29,6 +29,12 @@ (Result.error? &r))) "invalid SQL returns error") + (assert-equal test + &(Result.Error @"near \"NOT\": syntax error") + &(let [db (open-memory)] + (let-do [r (SQLite3.query &db "NOT VALID SQL" &[])] (SQLite3.close db) r)) + "error message survives statement teardown") + (assert-equal test &(Result.Success [[(SQLite3.Type.Integer 42l) @@ -333,6 +339,20 @@ (Result.Error e) (do (SQLite3.close db) (Result.Error e)))) "exec-prepared returns results") + (assert-equal test + &(Result.Error @"UNIQUE constraint failed: t.x") + &(let-do [db (open-memory)] + (ignore (SQLite3.query &db "CREATE TABLE t (x INT UNIQUE);" &[])) + (ignore (SQLite3.query &db "INSERT INTO t VALUES (1);" &[])) + (match (SQLite3.prepare &db "INSERT INTO t VALUES (?1);") + (Result.Success stmt) + (let-do [r (SQLite3.exec-prepared &stmt &[(to-sqlite3 1)])] + (SQLite3.finalize-stmt stmt) + (SQLite3.close db) + r) + (Result.Error e) (do (SQLite3.close db) (Result.Error e)))) + "exec-prepared error message survives statement reset") + (assert-equal test &(Result.Success [[(SQLite3.Type.Integer 1l) (SQLite3.Type.Text @"a")]