Repository: diesel-rs/diesel (language: rust)

Issue:
Undefined behavior when using `diesel::sql_query` with an empty string for Sqlite
## Setup

### Versions

- **Rust:** rustc 1.80.1
- **Diesel:** v2.2.3
- **Database:** sqlite 3.44.1
- **Operating System** Linux

### Feature Flags

- **diesel:** sqlite

## Problem Description

This code has UB:
```rust
use diesel::{Connection, RunQueryDsl, SqliteConnection};

fn main() {
    let mut conn = SqliteConnection::establish("tmp.sqlite").unwrap();
    diesel::sql_query("").execute(&mut conn).unwrap();
}
```

Output with `cargo run`:
```
thread 'main' panicked at library/core/src/panicking.rs:219:5:
unsafe precondition(s) violated: NonNull::new_unchecked requires that the pointer is non-null
stack backtrace:
   0: rust_begin_unwind
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_nounwind_fmt::runtime
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panicking.rs:110:18
   2: core::panicking::panic_nounwind_fmt
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panicking.rs:120:5
   3: core::panicking::panic_nounwind
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panicking.rs:219:5
   4: core::ptr::non_null::NonNull<T>::new_unchecked::precondition_check
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/ub_checks.rs:68:21
   5: core::ptr::non_null::NonNull<T>::new_unchecked
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/ub_checks.rs:75:17
   6: diesel::sqlite::connection::stmt::Statement::prepare::{{closure}}
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/sqlite/connection/stmt.rs:53:43
   7: core::result::Result<T,E>::map
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/result.rs:771:25
   8: diesel::sqlite::connection::stmt::Statement::prepare
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/sqlite/connection/stmt.rs:51:9
   9: diesel::sqlite::connection::SqliteConnection::prepared_query::{{closure}}
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/sqlite/connection/mod.rs:356:30
  10: diesel::connection::statement_cache::StatementCache<DB,Statement>::cached_statement_non_generic
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/connection/statement_cache.rs:222:20
  11: diesel::connection::statement_cache::StatementCache<DB,Statement>::cached_statement
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/connection/statement_cache.rs:196:9
  12: diesel::sqlite::connection::SqliteConnection::prepared_query
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/sqlite/connection/mod.rs:352:31
  13: <diesel::sqlite::connection::SqliteConnection as diesel::connection::Connection>::execute_returning_count
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/sqlite/connection/mod.rs:187:29
  14: <T as diesel::query_dsl::load_dsl::ExecuteDsl<Conn,DB>>::execute
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/query_dsl/load_dsl.rs:90:9
  15: diesel::query_dsl::RunQueryDsl::execute
             at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.3/src/query_dsl/mod.rs:1433:9
  16: poc_diesel::main
             at ./src/main.rs:5:5
  17: core::ops::function::FnOnce::call_once
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread caused non-unwinding panic. aborting.
[1]    6997 abort (core dumped)
```

It seems the problem is linked to the [`sqlite3_prepare_v3`](https://github.com/diesel-rs/diesel/blob/08911c8420f4e2ff732a3b263a5a4f3df0f745d2/diesel/src/sqlite/connection/stmt.rs#L37) call (see [sqlite doc](https://www.sqlite.org/c3ref/prepare.html)):
> If the input text contains no SQL (if the input is an empty string or a comment) then *ppStmt is set to NULL.

## Checklist

- [x] I have already looked over the [issue tracker](https://github.com/diesel-rs/diesel/issues) and the [discussion forum](https://github.com/diesel-rs/diesel/discussions) for similar possible closed issues.
<!--
If you are unsure if your issue is a duplicate of an existing issue please link the issue in question here
--> 
- [x] This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)
- [x] This issue can be reproduced without requiring a third party crate



Discussion / hints:
Thanks for reporting. It seems like we missed that sentence. I've pushed #4224 to address this. I likely will cut another patch release later this week.

Also I'm not sure if that warrants a rustsec advisory or not. On the one hand it's a potential null pointer dereference, on the other hand you shouldn't run into that by accident. 

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 364378555acc..910dbae2a10e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,12 @@ Increasing the minimal supported Rust version will always be coupled at least wi
 
 ## Unreleased
 
+## [2.2.4] 2024-09-3
+
+### Fixed
+
+* Fix an issue where empty queries could trigger undefined behaviour in the sqlite backend
+
 ## [2.2.3] 2024-08-23
 
 ### Fixed
@@ -2136,4 +2142,5 @@ queries or set `PIPES_AS_CONCAT` manually.
 [2.2.0]: https://github.com/diesel-rs/diesel/compare/v.2.1.0...v2.2.0
 [2.2.1]: https://github.com/diesel-rs/diesel/compare/v.2.2.0...v2.2.1
 [2.2.2]: https://github.com/diesel-rs/diesel/compare/v.2.2.1...v2.2.2
-[2.2.2]: https://github.com/diesel-rs/diesel/compare/v.2.2.2...v2.2.3
+[2.2.3]: https://github.com/diesel-rs/diesel/compare/v.2.2.2...v2.2.3
+[2.2.4]: https://github.com/diesel-rs/diesel/compare/v.2.2.3...v2.2.4
diff --git a/diesel/Cargo.toml b/diesel/Cargo.toml
index a62c32293b28..db9347e10cf7 100644
--- a/diesel/Cargo.toml
+++ b/diesel/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "diesel"
-version = "2.2.3"
+version = "2.2.4"
 license = "MIT OR Apache-2.0"
 description = "A safe, extensible ORM and Query Builder for PostgreSQL, SQLite, and MySQL"
 readme = "README.md"
diff --git a/diesel/src/result.rs b/diesel/src/result.rs
index acf12423b068..92c4937fa9ca 100644
--- a/diesel/src/result.rs
+++ b/diesel/src/result.rs
@@ -456,6 +456,21 @@ impl fmt::Display for EmptyChangeset {
 
 impl StdError for EmptyChangeset {}
 
+/// Expected when you try to execute an empty query
+#[derive(Debug, Clone, Copy)]
+pub struct EmptyQuery;
+
+impl fmt::Display for EmptyQuery {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "Detected an empty query. These are not supported by your database system"
+        )
+    }
+}
+
+impl StdError for EmptyQuery {}
+
 /// An error occurred while deserializing a field
 #[derive(Debug)]
 #[non_exhaustive]
diff --git a/diesel/src/sqlite/connection/mod.rs b/diesel/src/sqlite/connection/mod.rs
index 12c59ccc960d..4f6e2e871c32 100644
--- a/diesel/src/sqlite/connection/mod.rs
+++ b/diesel/src/sqlite/connection/mod.rs
@@ -993,4 +993,21 @@ mod tests {
         .unwrap();
         assert_eq!(res, Some(Vec::new()));
     }
+
+    #[test]
+    fn correctly_handle_empty_query() {
+        let check_empty_query_error = |r: crate::QueryResult<usize>| {
+            assert!(r.is_err());
+            let err = r.unwrap_err();
+            assert!(
+                matches!(err, crate::result::Error::QueryBuilderError(ref b) if b.is::<crate::result::EmptyQuery>()),
+                "Expected a query builder error, but got {err}"
+            );
+        };
+        let connection = &mut SqliteConnection::establish(":memory:").unwrap();
+        check_empty_query_error(crate::sql_query("").execute(connection));
+        check_empty_query_error(crate::sql_query("   ").execute(connection));
+        check_empty_query_error(crate::sql_query("\n\t").execute(connection));
+        check_empty_query_error(crate::sql_query("-- SELECT 1;").execute(connection));
+    }
 }
diff --git a/diesel/src/sqlite/connection/stmt.rs b/diesel/src/sqlite/connection/stmt.rs
index 99bb271918a3..62e370fb81a3 100644
--- a/diesel/src/sqlite/connection/stmt.rs
+++ b/diesel/src/sqlite/connection/stmt.rs
@@ -48,11 +48,14 @@ impl Statement {
             )
         };
 
-        ensure_sqlite_ok(prepare_result, raw_connection.internal_connection.as_ptr()).map(|_| {
-            Statement {
-                inner_statement: unsafe { NonNull::new_unchecked(stmt) },
-            }
-        })
+        ensure_sqlite_ok(prepare_result, raw_connection.internal_connection.as_ptr())?;
+
+        // sqlite3_prepare_v3 returns a null pointer for empty statements. This includes
+        // empty or only whitespace strings or any other non-op query string like a comment
+        let inner_statement = NonNull::new(stmt).ok_or_else(|| {
+            crate::result::Error::QueryBuilderError(Box::new(crate::result::EmptyQuery))
+        })?;
+        Ok(Statement { inner_statement })
     }
 
     // The caller of this function has to ensure that:
diff --git a/diesel_cli/Cargo.toml b/diesel_cli/Cargo.toml
index fe0906ba5c8d..b9db32aeab85 100644
--- a/diesel_cli/Cargo.toml
+++ b/diesel_cli/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "diesel_cli"
-version = "2.2.3"
+version = "2.2.4"
 license = "MIT OR Apache-2.0"
 description = "Provides the CLI for the Diesel crate"
 readme = "README.md"


Test patch: