From d4133288261612d43853245aa886e1a3a0d2dbfe Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Thu, 4 Mar 2021 18:38:06 +0100 Subject: [PATCH 1/5] Fix a use-after-free bug in our Sqlite backend 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. --- CHANGELOG.md | 8 +++++ diesel/Cargo.toml | 2 +- .../sqlite/connection/statement_iterator.rs | 34 +++++++++++++------ diesel/src/sqlite/connection/stmt.rs | 11 ++++-- 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a04e1ead468..028758536239 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 @@ -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 diff --git a/diesel/Cargo.toml b/diesel/Cargo.toml index dfe063243a6a..4c129ec20806 100644 --- a/diesel/Cargo.toml +++ b/diesel/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "diesel" -version = "1.4.5" +version = "1.4.6" authors = ["Sean Griffin "] license = "MIT OR Apache-2.0" description = "A safe, extensible ORM and Query Builder for PostgreSQL, SQLite, and MySQL" diff --git a/diesel/src/sqlite/connection/statement_iterator.rs b/diesel/src/sqlite/connection/statement_iterator.rs index 4454dc4825fe..b851c3168c46 100644 --- a/diesel/src/sqlite/connection/statement_iterator.rs +++ b/diesel/src/sqlite/connection/statement_iterator.rs @@ -42,28 +42,35 @@ where pub struct NamedStatementIterator<'a, T> { stmt: StatementUse<'a>, - column_indices: HashMap<&'a str, usize>, + column_indices: Option>, _marker: PhantomData, } impl<'a, T> NamedStatementIterator<'a, T> { #[allow(clippy::new_ret_no_self)] pub fn new(stmt: StatementUse<'a>) -> QueryResult { - 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::>()?; - Ok(NamedStatementIterator { - stmt, - column_indices, - _marker: PhantomData, - }) + + self.column_indices = Some(column_indices); + Ok(()) } } @@ -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) }) } diff --git a/diesel/src/sqlite/connection/stmt.rs b/diesel/src/sqlite/connection/stmt.rs index e95106f0df40..6ef3cb564672 100644 --- a/diesel/src/sqlite/connection/stmt.rs +++ b/diesel/src/sqlite/connection/stmt.rs @@ -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() { From d2192118eba79ff08044455eabc784525de8d854 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Fri, 5 Mar 2021 10:33:08 +0100 Subject: [PATCH 2/5] Address review comments --- diesel/src/sqlite/connection/statement_iterator.rs | 12 ++++++++---- diesel/src/sqlite/connection/stmt.rs | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/diesel/src/sqlite/connection/statement_iterator.rs b/diesel/src/sqlite/connection/statement_iterator.rs index b851c3168c46..dfa91da99445 100644 --- a/diesel/src/sqlite/connection/statement_iterator.rs +++ b/diesel/src/sqlite/connection/statement_iterator.rs @@ -42,6 +42,11 @@ where pub struct NamedStatementIterator<'a, T> { stmt: StatementUse<'a>, + // 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>, _marker: PhantomData, } @@ -59,10 +64,9 @@ impl<'a, T> NamedStatementIterator<'a, T> { fn populate_column_indices(&mut self) -> QueryResult<()> { let column_indices = (0..self.stmt.num_fields()) .filter_map(|i| { - dbg!(i); - dbg!(self.stmt.field_name(i)).map(|column| { - let column = dbg!(column - .to_str()) + self.stmt.field_name(i).map(|column| { + let column = column + .to_str() .map_err(|e| DeserializationError(e.into()))?; Ok((column, i)) }) diff --git a/diesel/src/sqlite/connection/stmt.rs b/diesel/src/sqlite/connection/stmt.rs index 6ef3cb564672..b73ccb8a91f5 100644 --- a/diesel/src/sqlite/connection/stmt.rs +++ b/diesel/src/sqlite/connection/stmt.rs @@ -67,7 +67,7 @@ impl Statement { /// > 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> { + 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 From d699f1499f3cc1c6920cb6aebc72a3aff1880c8f Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Fri, 5 Mar 2021 10:47:22 +0100 Subject: [PATCH 3/5] Bump some dependencies --- CHANGELOG.md | 1 + diesel/Cargo.toml | 10 +++++----- diesel_cli/Cargo.toml | 2 +- diesel_tests/Cargo.toml | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 028758536239..c5b34dcd8385 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/ * Fixed a use-after-free issue in the `QueryableByName` implementation of our `Sqlite` backend +* Updated several dependencies ## [1.4.5] - 2020-06-09 diff --git a/diesel/Cargo.toml b/diesel/Cargo.toml index 4c129ec20806..24ad2afe8fc5 100644 --- a/diesel/Cargo.toml +++ b/diesel/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "diesel" -version = "1.4.6" +version = "1.4.5" authors = ["Sean Griffin "] license = "MIT OR Apache-2.0" description = "A safe, extensible ORM and Query Builder for PostgreSQL, SQLite, and MySQL" @@ -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 } @@ -25,11 +25,11 @@ 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 } -num-bigint = { version = ">=0.1.41, <0.3", optional = true } +ipnetwork = { version = ">=0.12.2, <0.18.0", optional = true } +num-bigint = { version = ">=0.1.41, <0.4", optional = true } num-traits = { version = ">=0.1.37, <0.3", optional = true } num-integer = { version = ">=0.1.33, <0.3", optional = true } -bigdecimal = { version = ">= 0.0.10, < 0.2.0", optional = true } +bigdecimal = { version = ">= 0.0.10, < 0.3.0", optional = true } bitflags = { version = "1.0", optional = true } r2d2 = { version = ">= 0.8, < 0.9", optional = true } diff --git a/diesel_cli/Cargo.toml b/diesel_cli/Cargo.toml index f12bfbaa3507..fafa4a160cdb 100644 --- a/diesel_cli/Cargo.toml +++ b/diesel_cli/Cargo.toml @@ -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" diff --git a/diesel_tests/Cargo.toml b/diesel_tests/Cargo.toml index 533bad308b14..dd3b7b77847a 100644 --- a/diesel_tests/Cargo.toml +++ b/diesel_tests/Cargo.toml @@ -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.3.0" [features] default = [] From 2a612f078c625fa4c63012ec1f56e68141720229 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Fri, 5 Mar 2021 11:06:03 +0100 Subject: [PATCH 4/5] Run cargo fmt --- diesel/src/sqlite/connection/statement_iterator.rs | 6 +++++- diesel/src/sqlite/connection/stmt.rs | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/diesel/src/sqlite/connection/statement_iterator.rs b/diesel/src/sqlite/connection/statement_iterator.rs index dfa91da99445..0073a679c151 100644 --- a/diesel/src/sqlite/connection/statement_iterator.rs +++ b/diesel/src/sqlite/connection/statement_iterator.rs @@ -95,7 +95,11 @@ where } } row.map(|row| { - let row = row.into_named(self.column_indices.as_ref().expect("it's there because we populated it above")); + 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) }) } diff --git a/diesel/src/sqlite/connection/stmt.rs b/diesel/src/sqlite/connection/stmt.rs index b73ccb8a91f5..206bad766b9a 100644 --- a/diesel/src/sqlite/connection/stmt.rs +++ b/diesel/src/sqlite/connection/stmt.rs @@ -65,7 +65,7 @@ impl Statement { /// > 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, '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); From cead1a3b5d14ec90d356306b168ba0940167d36b Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Fri, 5 Mar 2021 11:12:08 +0100 Subject: [PATCH 5/5] Downgrade max bigdecimal version as the new version is incompatible --- diesel/Cargo.toml | 4 ++-- diesel_tests/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/diesel/Cargo.toml b/diesel/Cargo.toml index 24ad2afe8fc5..b9da265a9b72 100644 --- a/diesel/Cargo.toml +++ b/diesel/Cargo.toml @@ -26,10 +26,10 @@ 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.18.0", optional = true } -num-bigint = { version = ">=0.1.41, <0.4", 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 } -bigdecimal = { version = ">= 0.0.10, < 0.3.0", optional = true } +bigdecimal = { version = ">= 0.0.10, < 0.2.0", optional = true } bitflags = { version = "1.0", optional = true } r2d2 = { version = ">= 0.8, < 0.9", optional = true } diff --git a/diesel_tests/Cargo.toml b/diesel_tests/Cargo.toml index dd3b7b77847a..806e67d463be 100644 --- a/diesel_tests/Cargo.toml +++ b/diesel_tests/Cargo.toml @@ -22,7 +22,7 @@ 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.18.0" -bigdecimal = ">= 0.0.10, < 0.3.0" +bigdecimal = ">= 0.0.10, < 0.2.0" [features] default = []