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

[weiznich]

We've missused sqlite3_column_name. The
SQLite 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.

cc @diesel-rs/contributors and @diesel-rs/core: I've plan to release this change tomorrow. Additionally I see if we need to get a RUSTSEC advisory for this or not.

Fixes #2662

[Ten0]

dbg

[Ten0]

dbg

[Ten0]

dbg

[Ten0]

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 :)

[weiznich]

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.

[Ten0]

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

[weiznich]

Thats correct 👍

[Ten0]

The doc states that "The lifetime of the returned CStr is shorter than self.", but it looks like this signature actually allows extending the lifetime.

fn field_name(&self, idx: usize) -> Option<&CStr>;

or (if we want to be more explicit)

fn field_name<'a, 'b: 'a>(&'a self, idx: usize) -> Option<&'b CStr>;

would solve part of the issue.

[Ten0]

So this seems to not break memory safety specifically because this call to into_named enforces downgrading the lifetime of 'a in HashMap<&'a str, usize> to the lifetime of self.
Maybe we could explicit it there in order to contain why this works closer? (It looks that currently it wouldn't be hard to change the signature of into_named, see it compile and call it a day.)

impl<'a, T> Iterator for NamedStatementIterator<'a, T>
where
    T: QueryableByName<Sqlite>,
{
    type Item = QueryResult<T>;

    fn next<'s>(&'s mut self) -> Option<Self::Item> {
        let row = match self.stmt.step() {
            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| {
            // explicitly downgrade the lifetime to a correct one
            let column_indices: &'s HashMap<&'s str, usize> = self.column_indices.as_ref().expect("it's there because we populated it above");
            let row = row.into_named(column_indices);
            T::build(&row).map_err(DeserializationError)
        })
    }
}

Additionally it looks like we could add a doc comment on column_indices of to specify that 'a is not precisely correct in that context.

[weiznich]

This is not possible, it results in the following error message:

error[E0597]: `self` does not live long enough
  --> diesel/src/sqlite/connection/statement_iterator.rs:94:34
   |
83 |     fn next<'s>(&'s mut self) -> Option<Self::Item> {
   |             -- lifetime `'s` defined here
...
93 |         row.map(|row| {
   |                 ----- value captured here
94 |             let indices: &'s _ = self.column_indices.as_ref().expect("it's there because we populated it above");
   |                          -----   ^^^^ borrowed value does not live long enough
   |                          |
   |                          type annotation requires that `self` is borrowed for `'s`
...
98 |     }
   |     - `self` dropped here while still borrowed

That written: I'm quite sure no one will actually touch this code again, as this is already rewritten on the master branch in such a way that this cannot happen there. (Though we should check if we could trigger there the multiple calls to sqlite3_column_name thing there.)