Skip to content

Commit

Permalink
Fix a use-after-free bug in our Sqlite backend
Browse files Browse the repository at this point in the history
We've missused `sqlite3_column_name`. The
[SQLite](https://www.sqlite.org/c3ref/column_name.html) documentation
states that the following:

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

As part of our `query_by_name` infrastructure we've first received all
field names for the prepared statement and stored them as string slices
for later use. After that we called `sqlite3_step()` for the first time,
which invalids the pointer and therefore the stored string slice.
I've opted to fix this by just populating the field name map after the
first call to `sqlite3_step()` as a minimal fix for this issue for the
1.x release series. We need to investigate further if and how a similar
issue can occur with the current master branch. The corresponding code
changed quite a lot and at least this particular behaviour is not
possible anymore.
  • Loading branch information
weiznich committed Mar 5, 2021
1 parent 8e9e639 commit d413328
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 14 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ All user visible changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/), as described
for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md)

## [1.4.6] - 2021-03-05

### Fixed

* Fixed a use-after-free issue in the `QueryableByName` implementation
of our `Sqlite` backend

## [1.4.5] - 2020-06-09

### Fixed
Expand Down Expand Up @@ -1640,3 +1647,4 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
[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
2 changes: 1 addition & 1 deletion diesel/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "diesel"
version = "1.4.5"
version = "1.4.6"
authors = ["Sean Griffin <[email protected]>"]
license = "MIT OR Apache-2.0"
description = "A safe, extensible ORM and Query Builder for PostgreSQL, SQLite, and MySQL"
Expand Down
34 changes: 23 additions & 11 deletions diesel/src/sqlite/connection/statement_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,28 +42,35 @@ where

pub struct NamedStatementIterator<'a, T> {
stmt: StatementUse<'a>,
column_indices: HashMap<&'a str, usize>,
column_indices: Option<HashMap<&'a str, usize>>,
_marker: PhantomData<T>,
}

impl<'a, T> NamedStatementIterator<'a, T> {
#[allow(clippy::new_ret_no_self)]
pub fn new(stmt: StatementUse<'a>) -> QueryResult<Self> {
let column_indices = (0..stmt.num_fields())
Ok(NamedStatementIterator {
stmt,
column_indices: None,
_marker: PhantomData,
})
}

fn populate_column_indices(&mut self) -> QueryResult<()> {
let column_indices = (0..self.stmt.num_fields())
.filter_map(|i| {
stmt.field_name(i).map(|column| {
let column = column
.to_str()
dbg!(i);
dbg!(self.stmt.field_name(i)).map(|column| {
let column = dbg!(column
.to_str())
.map_err(|e| DeserializationError(e.into()))?;
Ok((column, i))
})
})
.collect::<QueryResult<_>>()?;
Ok(NamedStatementIterator {
stmt,
column_indices,
_marker: PhantomData,
})

self.column_indices = Some(column_indices);
Ok(())
}
}

Expand All @@ -78,8 +85,13 @@ where
Ok(row) => row,
Err(e) => return Some(Err(e)),
};
if self.column_indices.is_none() {
if let Err(e) = self.populate_column_indices() {
return Some(Err(e));
}
}
row.map(|row| {
let row = row.into_named(&self.column_indices);
let row = row.into_named(self.column_indices.as_ref().expect("it's there because we populated it above"));
T::build(&row).map_err(DeserializationError)
})
}
Expand Down
11 changes: 9 additions & 2 deletions diesel/src/sqlite/connection/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,15 @@ impl Statement {
unsafe { ffi::sqlite3_column_count(self.inner_statement.as_ptr()) as usize }
}

/// The lifetime of the returned CStr is shorter than self. This function
/// should be tied to a lifetime that ends before the next call to `reset`
/// The lifetime of the returned CStr is shorter than 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://www.sqlite.org/c3ref/column_name.html
unsafe fn field_name<'a>(&self, idx: usize) -> Option<&'a CStr> {
let ptr = ffi::sqlite3_column_name(self.inner_statement.as_ptr(), idx as libc::c_int);
if ptr.is_null() {
Expand Down

0 comments on commit d413328

Please sign in to comment.