Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a use-after-free bug in our Sqlite backend #2663

Merged
merged 5 commits into from
Mar 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@ 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
* Updated several dependencies

## [1.4.5] - 2020-06-09

### Fixed
Expand Down Expand Up @@ -1640,3 +1648,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
4 changes: 2 additions & 2 deletions diesel/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ byteorder = "1.0"
diesel_derives = "~1.4.0"
chrono = { version = "0.4", optional = true }
libc = { version = "0.2.0", optional = true }
libsqlite3-sys = { version = ">=0.8.0, <0.19.0", optional = true, features = ["min_sqlite_version_3_7_16"] }
libsqlite3-sys = { version = ">=0.8.0, <0.21.0", optional = true, features = ["min_sqlite_version_3_7_16"] }
mysqlclient-sys = { version = ">=0.1.0, <0.3.0", optional = true }
pq-sys = { version = ">=0.3.0, <0.5.0", optional = true }
quickcheck = { version = "0.4", optional = true }
Expand All @@ -25,7 +25,7 @@ time = { version = "0.1", optional = true }
url = { version = "1.4.0", optional = true }
uuid = { version = ">=0.2.0, <0.7.0", optional = true, features = ["use_std"] }
uuidv07 = { version = ">=0.7.0, <0.9.0", optional = true, package = "uuid"}
ipnetwork = { version = ">=0.12.2, <0.17.0", optional = true }
ipnetwork = { version = ">=0.12.2, <0.18.0", optional = true }
num-bigint = { version = ">=0.1.41, <0.3", optional = true }
num-traits = { version = ">=0.1.37, <0.3", optional = true }
num-integer = { version = ">=0.1.33, <0.3", optional = true }
Expand Down
38 changes: 29 additions & 9 deletions diesel/src/sqlite/connection/statement_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,28 +42,39 @@ where

pub struct NamedStatementIterator<'a, T> {
stmt: StatementUse<'a>,
column_indices: HashMap<&'a str, usize>,
// The actual lifetime of the stored column name is
// not really `'a`, but it's impossible to have a better
// fitting lifetime here.
// See the `Statement::field_name` method for details
// how long the underlying livetime is valid
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| {
self.stmt.field_name(i).map(|column| {
let column = 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 +89,17 @@ 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
13 changes: 10 additions & 3 deletions diesel/src/sqlite/connection/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,16 @@ 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`
unsafe fn field_name<'a>(&self, idx: usize) -> Option<&'a CStr> {
/// 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()
Comment on lines +65 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what "for a particular run" means in this context. I'd be able to review more usefully if I understood precisely what this means :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the exact wording of the SQLite documentation for this function. I interpret this in such a way that the pointer for a given column name can change if you execute a prepared statement and after the executing call sqlite3_step() the first time for this newly executed statement. (sqlite3_step() is basically Iterator::next() to get a new row from the result set). So we should fine as long as we call sqlite3_column_name() after the first call to sqlite3_step() for a particular run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm so if I understand correctly "for a particular run" means for us "for an instance of StatementUse".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats correct 👍

/// > or sqlite3_column_name16() on the same column.
///
/// https://www.sqlite.org/c3ref/column_name.html
unsafe fn field_name<'a, 'b: 'a>(&'a self, idx: usize) -> Option<&'b CStr> {
let ptr = ffi::sqlite3_column_name(self.inner_statement.as_ptr(), idx as libc::c_int);
if ptr.is_null() {
None
Expand Down
2 changes: 1 addition & 1 deletion diesel_cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ tempfile = "3.0.0"
toml = "0.4.6"
url = { version = "1.4.0", optional = true }
barrel = { version = ">= 0.5.0", optional = true, features = ["diesel"] }
libsqlite3-sys = { version = ">=0.8.0, <0.19.0", optional = true, features = ["min_sqlite_version_3_7_16"] }
libsqlite3-sys = { version = ">=0.8.0, <0.21.0", optional = true, features = ["min_sqlite_version_3_7_16"] }

[dev-dependencies]
difference = "1.0"
Expand Down
4 changes: 2 additions & 2 deletions diesel_tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ dotenv = ">=0.8, <0.11"
quickcheck = { version = "0.4", features = ["unstable"] }
uuid = { version = ">=0.7.0, <0.9.0" }
serde_json = { version=">=0.9, <2.0" }
ipnetwork = ">=0.12.2, <0.17.0"
bigdecimal = ">= 0.0.10, <= 0.1.0"
ipnetwork = ">=0.12.2, <0.18.0"
bigdecimal = ">= 0.0.10, < 0.2.0"

[features]
default = []
Expand Down