Undefined behavior when using `diesel::sql_query` with an empty string for Sqlite

stormshield-kg commented 2 weeks ago

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:

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 call (see sqlite doc):

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 and the discussion forum for similar possible closed issues.
[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

weiznich commented 2 weeks ago

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.

weiznich commented 2 weeks ago

I just published diesel 2.2.4 to address this issue

diesel-rs / diesel