Fix memory-safety bugs in the C helper layer

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).")

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;
 }
 

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"))))

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")]