From 04f900059dd59b567f68b812356c18cee432128b Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 26 Feb 2019 19:38:41 -0800 Subject: [PATCH] Avoid unnecessary copies/allocations when passing strings to sqlite --- Cargo.toml | 1 + src/context.rs | 15 +++++---------- src/inner_connection.rs | 16 ++++++---------- src/lib.rs | 33 +++++++++++++++++++++++++++++++++ src/statement.rs | 36 ++++++++++++------------------------ 5 files changed, 57 insertions(+), 44 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fb288ef..861c0f7 100644 --- a/Cargo.toml +++ b/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" diff --git a/src/context.rs b/src/context.rs index a7bb468..ad0a3ad 100644 --- a/src/context.rs +++ b/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::SQLITE_TRANSIENT() - } else { - ffi::SQLITE_STATIC() - }; - ffi::sqlite3_result_text(ctx, c_str.as_ptr(), length as c_int, destructor); + ffi::sqlite3_result_text(ctx, c_str, len, 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) diff --git a/src/inner_connection.rs b/src/inner_connection.rs index 499dcd6..421a131 100644 --- a/src/inner_connection.rs +++ b/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> { - 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 len_with_nul = (sql.len() + 1) as c_int; + let (c_sql, len, _) = str_for_sqlite(sql)?; let r = unsafe { if cfg!(feature = "unlock_notify") { let mut rc; loop { rc = ffi::sqlite3_prepare_v2( self.db(), - c_sql.as_ptr(), - len_with_nul, + c_sql, + len, &mut c_stmt, ptr::null_mut(), ); @@ -236,8 +232,8 @@ impl InnerConnection { } else { ffi::sqlite3_prepare_v2( self.db(), - c_sql.as_ptr(), - len_with_nul, + c_sql, + len, &mut c_stmt, ptr::null_mut(), ) diff --git a/src/lib.rs b/src/lib.rs index a06a25f..082a166 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -237,6 +237,39 @@ fn str_to_cstring(s: &str) -> Result { 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 { + 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 { let s = p.to_str().ok_or_else(|| Error::InvalidPath(p.to_owned()))?; str_to_cstring(s) diff --git a/src/statement.rs b/src/statement.rs index aa07ea7..241b21f 100644 --- a/src/statement.rs +++ b/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(); - 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(), - length as c_int, - destructor, - ) - } + let (c_str, len, destructor) = str_for_sqlite(s)?; + ffi::sqlite3_bind_text( + ptr, + col as c_int, + c_str, + len, + destructor, + ) }, ValueRef::Blob(b) => unsafe { - let length = b.len(); - if length > ::std::i32::MAX as usize { - ffi::SQLITE_TOOBIG - } else if length == 0 { + let length = len_as_c_int(b.len())?; + 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(), ) }