Merge pull request #487 from thomcc/avoid-unnecessary-copies

Avoid unnecessary copies/allocations when passing strings to sqlite

Cargo.toml

@@ -59,6 +59,7 @@ csv = { version = "1.0", optional = true }
 lazy_static = { version = "1.0", optional = true }
 byteorder = { version = "1.2", features = ["i128"], optional = true }
 fallible-streaming-iterator = { version = "0.1", optional = true }
+memchr = "2.2.0"
 
 [dev-dependencies]
 tempdir = "0.3"

src/context.rs

@@ -7,7 +7,7 @@ use std::rc::Rc;
 use crate::ffi;
 use crate::ffi::sqlite3_context;
 
-use crate::str_to_cstring;
+use crate::str_for_sqlite;
 use crate::types::{ToSqlOutput, ValueRef};
 #[cfg(feature = "array")]
 use crate::vtab::array::{free_array, ARRAY_TYPE};
@@ -38,25 +38,20 @@ pub(crate) unsafe fn set_result(ctx: *mut sqlite3_context, result: &ToSqlOutput<
         ValueRef::Real(r) => ffi::sqlite3_result_double(ctx, r),
         ValueRef::Text(s) => {
             let length = s.len();
-            if length > ::std::i32::MAX as usize {
+            if length > c_int::max_value() as usize {
                 ffi::sqlite3_result_error_toobig(ctx);
             } else {
-                let c_str = match str_to_cstring(s) {
+                let (c_str, len, destructor) = match str_for_sqlite(s) {
                     Ok(c_str) => c_str,
                     // TODO sqlite3_result_error
                     Err(_) => return ffi::sqlite3_result_error_code(ctx, ffi::SQLITE_MISUSE),
                 };
-                let destructor = if length > 0 {
+                ffi::sqlite3_result_text(ctx, c_str, len, destructor);
-                    ffi::SQLITE_TRANSIENT()
-                } else {
-                    ffi::SQLITE_STATIC()
-                };
-                ffi::sqlite3_result_text(ctx, c_str.as_ptr(), length as c_int, destructor);
             }
         }
         ValueRef::Blob(b) => {
             let length = b.len();
-            if length > ::std::i32::MAX as usize {
+            if length > c_int::max_value() as usize {
                 ffi::sqlite3_result_error_toobig(ctx);
             } else if length == 0 {
                 ffi::sqlite3_result_zeroblob(ctx, 0)

src/inner_connection.rs

@@ -9,7 +9,7 @@ use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Arc, Mutex, Once, ONCE_INIT};
 
 use super::ffi;
-use super::str_to_cstring;
+use super::{str_to_cstring, str_for_sqlite};
 use super::{Connection, InterruptHandle, OpenFlags, Result};
 use crate::error::{error_from_handle, error_from_sqlite_code, Error};
 use crate::raw_statement::RawStatement;
@@ -207,20 +207,16 @@ impl InnerConnection {
     }
 
     pub fn prepare<'a>(&mut self, conn: &'a Connection, sql: &str) -> Result<Statement<'a>> {
-        if sql.len() >= ::std::i32::MAX as usize {
-            return Err(error_from_sqlite_code(ffi::SQLITE_TOOBIG, None));
-        }
         let mut c_stmt: *mut ffi::sqlite3_stmt = unsafe { mem::uninitialized() };
-        let c_sql = str_to_cstring(sql)?;
+        let (c_sql, len, _) = str_for_sqlite(sql)?;
-        let len_with_nul = (sql.len() + 1) as c_int;
         let r = unsafe {
             if cfg!(feature = "unlock_notify") {
                 let mut rc;
                 loop {
                     rc = ffi::sqlite3_prepare_v2(
                         self.db(),
-                        c_sql.as_ptr(),
+                        c_sql,
-                        len_with_nul,
+                        len,
                         &mut c_stmt,
                         ptr::null_mut(),
                     );
@@ -236,8 +232,8 @@ impl InnerConnection {
             } else {
                 ffi::sqlite3_prepare_v2(
                     self.db(),
-                    c_sql.as_ptr(),
+                    c_sql,
-                    len_with_nul,
+                    len,
                     &mut c_stmt,
                     ptr::null_mut(),
                 )

src/lib.rs

@@ -237,6 +237,39 @@ fn str_to_cstring(s: &str) -> Result<CString> {
     Ok(CString::new(s)?)
 }
 
+/// Returns `Ok((string ptr, len as c_int, SQLITE_STATIC | SQLITE_TRANSIENT))`
+/// normally.
+/// Returns errors if the string has embedded nuls or is too large for sqlite.
+/// The `sqlite3_destructor_type` item is always `SQLITE_TRANSIENT` unless
+/// the string was empty (in which case it's `SQLITE_STATIC`, and the ptr is
+/// static).
+fn str_for_sqlite(s: &str) -> Result<(*const c_char, c_int, ffi::sqlite3_destructor_type)> {
+    let len = len_as_c_int(s.len())?;
+    if memchr::memchr(0, s.as_bytes()).is_none() {
+        let (ptr, dtor_info) = if len != 0 {
+            (s.as_ptr() as *const c_char, ffi::SQLITE_TRANSIENT())
+        } else {
+            // Return a pointer guaranteed to live forever
+            ("".as_ptr() as *const c_char, ffi::SQLITE_STATIC())
+        };
+        Ok((ptr, len, dtor_info))
+    } else {
+        // There's an embedded nul, so we fabricate a NulError.
+        let e = CString::new(s);
+        Err(Error::NulError(e.unwrap_err()))
+    }
+}
+
+// Helper to cast to c_int safely, returning the correct error type if the cast
+// failed.
+fn len_as_c_int(len: usize) -> Result<c_int> {
+    if len >= (c_int::max_value() as usize) {
+        Err(Error::SqliteFailure(ffi::Error::new(ffi::SQLITE_TOOBIG), None))
+    } else {
+        Ok(len as c_int)
+    }
+}
+
 fn path_to_cstring(p: &Path) -> Result<CString> {
     let s = p.to_str().ok_or_else(|| Error::InvalidPath(p.to_owned()))?;
     str_to_cstring(s)

src/statement.rs

@@ -7,7 +7,7 @@ use std::slice::from_raw_parts;
 use std::{convert, fmt, mem, ptr, result, str};
 
 use super::ffi;
-use super::str_to_cstring;
+use super::{str_to_cstring, str_for_sqlite, len_as_c_int};
 use super::{
     AndThenRows, Connection, Error, MappedRows, RawStatement, Result, Row, Rows, ValueRef,
 };
@@ -506,37 +506,25 @@ impl Statement<'_> {
             ValueRef::Integer(i) => unsafe { ffi::sqlite3_bind_int64(ptr, col as c_int, i) },
             ValueRef::Real(r) => unsafe { ffi::sqlite3_bind_double(ptr, col as c_int, r) },
             ValueRef::Text(s) => unsafe {
-                let length = s.len();
+                let (c_str, len, destructor) = str_for_sqlite(s)?;
-                if length > ::std::i32::MAX as usize {
-                    ffi::SQLITE_TOOBIG
-                } else {
-                    let c_str = str_to_cstring(s)?;
-                    let destructor = if length > 0 {
-                        ffi::SQLITE_TRANSIENT()
-                    } else {
-                        ffi::SQLITE_STATIC()
-                    };
                 ffi::sqlite3_bind_text(
                     ptr,
                     col as c_int,
-                        c_str.as_ptr(),
+                    c_str,
-                        length as c_int,
+                    len,
                     destructor,
                 )
-                }
             },
             ValueRef::Blob(b) => unsafe {
-                let length = b.len();
+                let length = len_as_c_int(b.len())?;
-                if length > ::std::i32::MAX as usize {
+                if length == 0 {
-                    ffi::SQLITE_TOOBIG
-                } else if length == 0 {
                     ffi::sqlite3_bind_zeroblob(ptr, col as c_int, 0)
                 } else {
                     ffi::sqlite3_bind_blob(
                         ptr,
                         col as c_int,
                         b.as_ptr() as *const c_void,
-                        length as c_int,
+                        length,
                         ffi::SQLITE_TRANSIENT(),
                     )
                 }