Merge pull request #2676 from weiznich/bug/fix_potential_use_after_free

Bug/fix potential use after free

Author: weiznich

CHANGELOG.md

@@ -245,6 +245,14 @@ Key points:
 
 [2-0-migration]: FIXME write a migration guide
 
+## [1.4.6] - 2021-03-05
+
+### Fixed
+
+* Fixed a use-after-free issue in the `QueryableByName` implementation
+  of our `Sqlite` backend
+* Updated several dependencies
+
 ## [1.4.5] - 2020-06-09
 
 ### Fixed
@@ -1882,3 +1890,5 @@ Key points:
 [1.4.2]: https://github.com/diesel-rs/diesel/compare/v1.4.1...v1.4.2
 [1.4.3]: https://github.com/diesel-rs/diesel/compare/v1.4.2...v1.4.3
 [1.4.4]: https://github.com/diesel-rs/diesel/compare/v1.4.3...v1.4.4
+[1.4.5]: https://github.com/diesel-rs/diesel/compare/v1.4.4...v1.4.5
+[1.4.6]: https://github.com/diesel-rs/diesel/compare/v1.4.5...v1.4.6

diesel/src/sqlite/connection/mod.rs

@@ -89,7 +89,7 @@ impl Connection for SqliteConnection {
         Self::Backend: QueryMetadata<T::SqlType>,
     {
         let mut statement = self.prepare_query(&source.as_query())?;
-        let statement_use = StatementUse::new(&mut statement);
+        let statement_use = StatementUse::new(&mut statement, true);
         let iter = StatementIterator::new(statement_use);
         iter.collect()
     }
@@ -100,7 +100,7 @@ impl Connection for SqliteConnection {
         T: QueryFragment<Self::Backend> + QueryId,
     {
         let mut statement = self.prepare_query(source)?;
-        let mut statement_use = StatementUse::new(&mut statement);
+        let mut statement_use = StatementUse::new(&mut statement, false);
         statement_use.run()?;
         Ok(self.raw_connection.rows_affected_by_last_query())
     }

diesel/src/sqlite/connection/sqlite_value.rs

@@ -1,13 +1,14 @@
 extern crate libsqlite3_sys as ffi;
 
 use std::marker::PhantomData;
-use std::os::raw as libc;
 use std::ptr::NonNull;
 use std::{slice, str};
 
 use crate::row::*;
 use crate::sqlite::{Sqlite, SqliteType};
 
+use super::stmt::StatementUse;
+
 /// Raw sqlite value as received from the database
 ///
 /// Use existing `FromSql` implementations to convert this into
@@ -18,11 +19,8 @@ pub struct SqliteValue<'a> {
     p: PhantomData<&'a ()>,
 }
 
-#[derive(Clone)]
-pub struct SqliteRow<'a> {
-    stmt: NonNull<ffi::sqlite3_stmt>,
-    next_col_index: libc::c_int,
-    p: PhantomData<&'a ()>,
+pub struct SqliteRow<'a: 'b, 'b: 'c, 'c> {
+    stmt: &'c StatementUse<'a, 'b>,
 }
 
 impl<'a> SqliteValue<'a> {
@@ -92,22 +90,20 @@ impl<'a> SqliteValue<'a> {
     }
 }
 
-impl<'a> SqliteRow<'a> {
-    pub(crate) unsafe fn new(inner_statement: NonNull<ffi::sqlite3_stmt>) -> Self {
+impl<'a: 'b, 'b: 'c, 'c> SqliteRow<'a, 'b, 'c> {
+    pub(crate) fn new(inner_statement: &'c StatementUse<'a, 'b>) -> Self {
         SqliteRow {
             stmt: inner_statement,
-            next_col_index: 0,
-            p: PhantomData,
         }
     }
 }
 
-impl<'a> Row<'a, Sqlite> for SqliteRow<'a> {
-    type Field = SqliteField<'a>;
+impl<'a: 'b, 'b: 'c, 'c> Row<'c, Sqlite> for SqliteRow<'a, 'b, 'c> {
+    type Field = SqliteField<'a, 'b, 'c>;
     type InnerPartialRow = Self;
 
     fn field_count(&self) -> usize {
-        column_count(self.stmt) as usize
+        self.stmt.column_count() as usize
     }
 
     fn get<I>(&self, idx: I) -> Option<Self::Field>
@@ -116,9 +112,8 @@ impl<'a> Row<'a, Sqlite> for SqliteRow<'a> {
     {
         let idx = self.idx(idx)?;
         Some(SqliteField {
-            stmt: self.stmt,
+            stmt: &self.stmt,
             col_idx: idx as i32,
-            p: PhantomData,
         })
     }
 
@@ -127,63 +122,37 @@ impl<'a> Row<'a, Sqlite> for SqliteRow<'a> {
     }
 }
 
-impl<'a> RowIndex<usize> for SqliteRow<'a> {
+impl<'a: 'b, 'b: 'c, 'c> RowIndex<usize> for SqliteRow<'a, 'b, 'c> {
     fn idx(&self, idx: usize) -> Option<usize> {
-        if idx < self.field_count() {
+        if idx < self.stmt.column_count() as usize {
             Some(idx)
         } else {
             None
         }
     }
 }
 
-impl<'a, 'b> RowIndex<&'a str> for SqliteRow<'b> {
-    fn idx(&self, field_name: &'a str) -> Option<usize> {
-        (0..column_count(self.stmt))
-            .find(|idx| column_name(self.stmt, *idx) == Some(field_name))
-            .map(|a| a as usize)
+impl<'a: 'b, 'b: 'c, 'c, 'd> RowIndex<&'d str> for SqliteRow<'a, 'b, 'c> {
+    fn idx(&self, field_name: &'d str) -> Option<usize> {
+        self.stmt.index_for_column_name(field_name)
     }
 }
 
-pub struct SqliteField<'a> {
-    stmt: NonNull<ffi::sqlite3_stmt>,
+pub struct SqliteField<'a: 'b, 'b: 'c, 'c> {
+    stmt: &'c StatementUse<'a, 'b>,
     col_idx: i32,
-    p: PhantomData<&'a ()>,
 }
 
-impl<'a> Field<'a, Sqlite> for SqliteField<'a> {
-    fn field_name(&self) -> Option<&'a str> {
-        column_name(self.stmt, self.col_idx)
+impl<'a: 'b, 'b: 'c, 'c> Field<'c, Sqlite> for SqliteField<'a, 'b, 'c> {
+    fn field_name(&self) -> Option<&'c str> {
+        self.stmt.field_name(self.col_idx)
     }
 
     fn is_null(&self) -> bool {
         self.value().is_none()
     }
 
-    fn value(&self) -> Option<crate::backend::RawValue<'a, Sqlite>> {
-        unsafe {
-            let ptr = ffi::sqlite3_column_value(self.stmt.as_ptr(), self.col_idx);
-            SqliteValue::new(ptr)
-        }
+    fn value(&self) -> Option<crate::backend::RawValue<'c, Sqlite>> {
+        self.stmt.value(self.col_idx)
     }
 }
-
-fn column_name<'a>(stmt: NonNull<ffi::sqlite3_stmt>, field_number: i32) -> Option<&'a str> {
-    unsafe {
-        let ptr = ffi::sqlite3_column_name(stmt.as_ptr(), field_number);
-        if ptr.is_null() {
-            None
-        } else {
-            Some(std::ffi::CStr::from_ptr(ptr).to_str().expect(
-                "The Sqlite documentation states that this is UTF8. \
-                 If you see this error message something has gone \
-                 horribliy wrong. Please open an issue at the \
-                 diesel repository.",
-            ))
-        }
-    }
-}
-
-fn column_count(stmt: NonNull<ffi::sqlite3_stmt>) -> i32 {
-    unsafe { ffi::sqlite3_column_count(stmt.as_ptr()) }
-}

diesel/src/sqlite/connection/statement_iterator.rs

@@ -6,21 +6,21 @@ use crate::result::Error::DeserializationError;
 use crate::result::QueryResult;
 use crate::sqlite::Sqlite;
 
-pub struct StatementIterator<'a, ST, T> {
-    stmt: StatementUse<'a>,
+pub struct StatementIterator<'a: 'b, 'b, ST, T> {
+    stmt: StatementUse<'a, 'b>,
     _marker: PhantomData<(ST, T)>,
 }
 
-impl<'a, ST, T> StatementIterator<'a, ST, T> {
-    pub fn new(stmt: StatementUse<'a>) -> Self {
+impl<'a: 'b, 'b, ST, T> StatementIterator<'a, 'b, ST, T> {
+    pub fn new(stmt: StatementUse<'a, 'b>) -> Self {
         StatementIterator {
-            stmt: stmt,
+            stmt,
             _marker: PhantomData,
         }
     }
 }
 
-impl<'a, ST, T> Iterator for StatementIterator<'a, ST, T>
+impl<'a: 'b, 'b, ST, T> Iterator for StatementIterator<'a, 'b, ST, T>
 where
     T: FromSqlRow<ST, Sqlite>,
 {

diesel/src/sqlite/connection/stmt.rs

@@ -8,6 +8,7 @@ use std::ptr::{self, NonNull};
 use super::raw::RawConnection;
 use super::serialized_value::SerializedValue;
 use super::sqlite_value::SqliteRow;
+use super::SqliteValue;
 use crate::result::Error::DatabaseError;
 use crate::result::*;
 use crate::sqlite::SqliteType;
@@ -39,10 +40,6 @@ impl Statement {
         })
     }
 
-    fn run(&mut self) -> QueryResult<()> {
-        self.step().map(|_| ())
-    }
-
     pub fn bind(&mut self, tpe: SqliteType, value: Option<Vec<u8>>) -> QueryResult<()> {
         self.bind_index += 1;
         let value = SerializedValue {
@@ -54,16 +51,6 @@ impl Statement {
         ensure_sqlite_ok(result, self.raw_connection())
     }
 
-    fn step(&mut self) -> QueryResult<Option<SqliteRow>> {
-        unsafe {
-            match ffi::sqlite3_step(self.inner_statement.as_ptr()) {
-                ffi::SQLITE_DONE => Ok(None),
-                ffi::SQLITE_ROW => Ok(Some(SqliteRow::new(self.inner_statement))),
-                _ => Err(last_error(self.raw_connection())),
-            }
-        }
-    }
-
     fn reset(&mut self) {
         self.bind_index = 0;
         unsafe { ffi::sqlite3_reset(self.inner_statement.as_ptr()) };
@@ -127,27 +114,118 @@ impl Drop for Statement {
     }
 }
 
-pub struct StatementUse<'a> {
+pub struct StatementUse<'a: 'b, 'b> {
     statement: &'a mut Statement,
+    column_names: Vec<&'b str>,
+    should_init_column_names: bool,
 }
 
-impl<'a> StatementUse<'a> {
-    pub fn new(statement: &'a mut Statement) -> Self {
+impl<'a, 'b> StatementUse<'a, 'b> {
+    pub(in crate::sqlite::connection) fn new(
+        statement: &'a mut Statement,
+        should_init_column_names: bool,
+    ) -> Self {
         StatementUse {
-            statement: statement,
+            statement,
+            // Init with empty vector because column names
+            // can change till the first call to `step()`
+            column_names: Vec::new(),
+            should_init_column_names,
+        }
+    }
+
+    pub(in crate::sqlite::connection) fn run(&mut self) -> QueryResult<()> {
+        self.step().map(|_| ())
+    }
+
+    pub(in crate::sqlite::connection) fn step<'c>(
+        &'c mut self,
+    ) -> QueryResult<Option<SqliteRow<'a, 'b, 'c>>>
+    where
+        'b: 'c,
+    {
+        let res = unsafe {
+            match ffi::sqlite3_step(self.statement.inner_statement.as_ptr()) {
+                ffi::SQLITE_DONE => Ok(None),
+                ffi::SQLITE_ROW => Ok(Some(())),
+                _ => Err(last_error(self.statement.raw_connection())),
+            }
+        }?;
+        if self.should_init_column_names {
+            self.column_names = (0..self.column_count())
+                .map(|idx| unsafe { self.column_name(idx) })
+                .collect();
+            self.should_init_column_names = false;
         }
+        Ok(res.map(move |()| SqliteRow::new(self)))
+    }
+
+    // The returned string pointer is valid until either the prepared statement is
+    // destroyed by sqlite3_finalize() or until the statement is automatically
+    // reprepared by the first call to sqlite3_step() for a particular run or
+    // until the next call to sqlite3_column_name() or sqlite3_column_name16()
+    // on the same column.
+    //
+    // https://sqlite.org/c3ref/column_name.html
+    //
+    // As result of this requirements: Never use that function outside of `ColumnInformation`
+    // and never use `ColumnInformation` outside of `StatementUse`
+    unsafe fn column_name(&mut self, idx: i32) -> &'b str {
+        let name = {
+            let column_name =
+                ffi::sqlite3_column_name(self.statement.inner_statement.as_ptr(), idx);
+            assert!(
+                !column_name.is_null(),
+                "The Sqlite documentation states that it only returns a \
+                 null pointer here if we are in a OOM condition."
+            );
+            CStr::from_ptr(column_name)
+        };
+        name.to_str().expect(
+            "The Sqlite documentation states that this is UTF8. \
+             If you see this error message something has gone \
+             horribliy wrong. Please open an issue at the \
+             diesel repository.",
+        )
     }
 
-    pub fn run(&mut self) -> QueryResult<()> {
-        self.statement.run()
+    pub(in crate::sqlite::connection) fn column_count(&self) -> i32 {
+        unsafe { ffi::sqlite3_column_count(self.statement.inner_statement.as_ptr()) }
     }
 
-    pub fn step(&mut self) -> QueryResult<Option<SqliteRow>> {
-        self.statement.step()
+    pub(in crate::sqlite::connection) fn index_for_column_name(
+        &self,
+        field_name: &str,
+    ) -> Option<usize> {
+        self.column_names
+            .iter()
+            .enumerate()
+            .find(|(_, name)| name == &&field_name)
+            .map(|(idx, _)| idx)
+    }
+
+    pub(in crate::sqlite::connection) fn field_name<'c>(&'c self, idx: i32) -> Option<&'c str>
+    where
+        'b: 'c,
+    {
+        self.column_names.get(idx as usize).copied()
+    }
+
+    pub(in crate::sqlite::connection) fn value<'c>(
+        &'c self,
+        idx: i32,
+    ) -> Option<super::SqliteValue<'c>>
+    where
+        'b: 'c,
+    {
+        unsafe {
+            let ptr = ffi::sqlite3_column_value(self.statement.inner_statement.as_ptr(), idx);
+            SqliteValue::new(ptr)
+        }
     }
 }
 
-impl<'a> Drop for StatementUse<'a> {
+impl<'a, 'b> Drop for StatementUse<'a, 'b> {
     fn drop(&mut self) {
         self.statement.reset();
     }