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

Issue:
Binding a null value should never fail
<!--
If you want to report a bug, we added some points below which help us track down the problem faster.
-->

## Setup

### Versions

- **Rust:**
`rustc 1.68.0`
- **Diesel:**
`2.0.0`
- **Database:**
`Sqlite`
- **Operating System**
`NixOS`

### Feature Flags

- **diesel:**
`["sqlite", "r2d2"]`

## Problem Description

### What are you trying to accomplish?

Perform a recursive CTE query to load an item from a path spec, using `sql_query` and a struct deriving `QueryableByName`

### What is the expected output?

The columns desired are selected

### What is the actual output?

`thread 'rocket-worker-thread' panicked at 'Binding a null value should never fail. If you ever see this error message please open an issue at diesels issue tracker containing code how to trigger this message.: DatabaseError(Unknown, "column index out of range")', /home/leonid/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.0.3/src/sqlite/connection/stmt.rs:344:22`

### Are you seeing any additional errors?

N/A


### Steps to reproduce

<!--
Please include as much of your codebase as needed to reproduce the error.  If the relevant files are large, please consider linking to a public repository or a [Gist](https://gist.github.com/). This includes normally the following parts:

* The exact code where your hit the problem
* Relevant parts your schema, so any `table!` macro calls required for the
* Any other type definitions involved in the code, which produces your problem
-->

#### main.rs
```rust
use diesel::sql_types::Text;
extern crate diesel;
use diesel::sqlite::SqliteConnection;
use diesel::{prelude::*, sql_query};
use dotenvy::dotenv;
use std::env;

mod schema;
mod models;

pub fn establish_connection() -> SqliteConnection {
    dotenv().ok();
    let database_url = env::var("DATABASE_URL").expect("DATABASE_URL must be set");
    SqliteConnection::establish(&database_url)
        .unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
}

pub fn main() {
    let connection = &mut establish_connection();
    use self::models::Page;

    use self::schema::pages::dsl::*;

    // RECURSIVE SQL QUERY

    // Get all paths: then, compare with the path provided
    let child = sql_query(
        r#"WITH RECURSIVE CTE AS (
             SELECT id, slug AS path
             FROM pages
             WHERE parent_id IS NULL
             UNION ALL
             SELECT p.id, path || '/' || slug
             FROM pages p
             JOIN CTE ON p.parent_id = CTE.id
           )
           SELECT *
           FROM CTE
           WHERE path = '?'
"#
    );
    let path = String::from("foo");
    println!("path spec is {:?}", path);
    let child = child.bind::<Text, _>(path).load::<Page>(connection);
    println!("Child is: {:?}", child);
}
```

#### models.rs
```rust
use crate::schema::pages;
use diesel::{prelude::*};
use diesel::sql_types::{Nullable, Integer, Text};

#[derive(Queryable, QueryableByName, Insertable, Debug)]
#[diesel(primary_key(id))]
#[diesel(table_name = pages)]
pub struct Page {
    #[diesel(sql_type = Nullable<Integer>)]
    pub id: Option<i32>,
    #[diesel(sql_type = Nullable<Integer>)]
    pub parent_id: Option<i32>,
    #[diesel(sql_type = Text)]
    pub title: String,
    #[diesel(sql_type = Text)]
    pub slug: String,
    #[diesel(sql_type = Text)]
    pub html_content: String,
}
```

#### schema.rs
```rust
// @generated automatically by Diesel CLI.

diesel::table! {
    pages (id) {
        id -> Nullable<Integer>,
        parent_id -> Nullable<Integer>,
        title -> Text,
        slug -> Text,
        html_content -> Text,
    }
}

```

#### up.sql for pages
```sql
CREATE TABLE pages (
  id INTEGER PRIMARY KEY AUTOINCREMENT,
  parent_id INTEGER,
  title TEXT NOT NULL,
  slug TEXT NOT NULL,
  html_content TEXT NOT NULL,
  FOREIGN KEY(parent_id) REFERENCES pages(id) ON DELETE CASCADE
)
```
 
Database contains one page row in table pages,

`1 | NULL | foo | foo | <p>bar</p>`


## Checklist

- [X] I have already looked over the [issue tracker](https://github.com/diesel-rs/diesel/issues)  and the [disussion 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

I use dotenvy to set up the db, but it is not required.

<!--
Thank you for your submission!  You're helping make Diesel more robust 🎉

We'll try to respond as quickly as possible.
-->



Discussion / hints:
Thanks for opening this issue.

It seems like you triggered an internal error that is not supposed to be triggered. That written: It does not change the result, your query will error anyway because it does not contain a bind parameter. 

Fixed query: (note the removed `'` around the `?`, that turns the string `'?'` into a bind parameter).
```rust
    let child = sql_query(
        r#"WITH RECURSIVE CTE AS (
             SELECT id, slug AS path
             FROM pages
             WHERE parent_id IS NULL
             UNION ALL
             SELECT p.id, path || '/' || slug
             FROM pages p
             JOIN CTE ON p.parent_id = CTE.id
           )
           SELECT *
           FROM CTE
           WHERE path = ?
"#,
    );
```

I'm leaving this issue open for now, because the error message should be improved in diesel.

Fix patch:
diff --git a/diesel/src/sqlite/connection/stmt.rs b/diesel/src/sqlite/connection/stmt.rs
index d673af4a9358..9aeebe5dc6c3 100644
--- a/diesel/src/sqlite/connection/stmt.rs
+++ b/diesel/src/sqlite/connection/stmt.rs
@@ -293,24 +293,29 @@ impl<'stmt, 'query> BoundStatement<'stmt, 'query> {
                 .count(),
         );
         for (bind_idx, (bind, tpe)) in (1..).zip(binds) {
-            if matches!(
+            let is_borrowed_bind = matches!(
                 bind,
                 InternalSqliteBindValue::BorrowedString(_)
                     | InternalSqliteBindValue::BorrowedBinary(_)
-            ) {
-                // Store the id's of borrowed binds to unbind them on drop
-                self.binds_to_free.push((bind_idx, None));
-            }
+            );
 
             // It's safe to call bind here as:
             // * The type and value matches
             // * We ensure that corresponding buffers lives long enough below
             // * The statement is not used yet by `step` or anything else
             let res = unsafe { self.statement.bind(tpe, bind, bind_idx) }?;
+
+            // it's important to push these only after
+            // the call to bind succeded, otherwise we might attempt to
+            // call bind to an non-existing bind position in
+            // the destructor
             if let Some(ptr) = res {
                 // Store the id + pointer for a owned bind
                 // as we must unbind and free them on drop
                 self.binds_to_free.push((bind_idx, Some(ptr)));
+            } else if is_borrowed_bind {
+                // Store the id's of borrowed binds to unbind them on drop
+                self.binds_to_free.push((bind_idx, None));
             }
         }
         Ok(())
@@ -471,3 +476,30 @@ impl<'stmt, 'query> StatementUse<'stmt, 'query> {
         NonNull::new(ptr)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::prelude::*;
+    use crate::sql_types::Text;
+    use crate::SqliteConnection;
+
+    // this is a regression test for
+    // https://github.com/diesel-rs/diesel/issues/3558
+    #[test]
+    fn check_out_of_bounds_bind_does_not_panic_on_drop() {
+        let mut conn = SqliteConnection::establish(":memory:").unwrap();
+
+        let e = crate::sql_query("SELECT '?'")
+            .bind::<Text, _>("foo")
+            .execute(&mut conn);
+
+        assert!(e.is_err());
+        let e = e.unwrap_err();
+        if let crate::result::Error::DatabaseError(crate::result::DatabaseErrorKind::Unknown, m) = e
+        {
+            assert_eq!(m.message(), "column index out of range");
+        } else {
+            panic!("Wrong error returned");
+        }
+    }
+}


Test patch: