Fix a potential null pointer deref in the Sqlite backend

[weiznich]

The documentation of sqlite3_prepare_v3 states that:

*ppStmt is left pointing to a compiled prepared statement that can be
executed using sqlite3_step(). If there is an error, *ppStmt is set to NULL. If the input text contains no SQL (if the input is an empty string or a comment) then *ppStmt is set to NULL. The calling procedure is responsible for deleting the compiled SQL statement using sqlite3_finalize() after it has finished with it. ppStmt may not be NULL.

We already guard against the error case there before constructing the NonNull pointer, we do not guard against empty statements yet. This commit fixes that particular issue + adds a test to check if the fix works as expected.

This fixes #4223

[weiznich]

cc @diesel-rs/core I would like to release that as patch release somewhen this week.

[stormshield-kg]

Should we add a check after the call to sqlite3_prepare_v3() to ensure that stmt is not null before calling NonNull::new_unchecked() ?

[weiznich]

Given the documentation of sqlite3_prepare_v3 that shouldn't be necessary as we already cover both variants that are documented to return null. That written: It might still be worth to add that additional check to be sure.

[stormshield-kg]

The above check is not enough, since the pointer will be null if the query string is not empty but contains only whitespaces or comments. We should also add a test for this.

[Ten0]

It looks like that doesn't guard against strings that are considered empty by sqlite, e.g. whitespace and comments-only.
It looks like it would be safer to replace the initial check on the Diesel side by a match on NonNull::new(stmt) after making the sqlite C call, returning EmptyQuery if at that point it's still a null pointer, instead of using new_unchecked.

[stormshield-kg]

It looks like that doesn't guard against strings that are considered empty by sqlite, e.g. whitespace and comments-only. It looks like it would be safer to replace the initial check on the Diesel side by a match on NonNull::new(stmt) after making the sqlite C call, returning EmptyQuery if at that point it's still a null pointer, instead of using new_unchecked.

Yes, but it should be done only if the call returned ffi::SQLITE_OK.

[weiznich]

You are both correct, I misread that statment as literal empty string. Note to myself: Don't fix this kind of issues just before needing to leave.

I've now pushed a fixed version by using NonNull::new instead of the new_unchecked variant. I also also added tests for the other variants.