Skip to content

Bug/fix potential use after free #2676

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 19, 2021

Conversation

weiznich
Copy link
Member

@weiznich weiznich commented Mar 12, 2021

This is basically #2663 but targeting the master branch

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 concequence this means we cannot just call sqlite3_column_name in
Field::field_name(). To fix this issue I decided to get all column
names pointers once after calling step() the first time and cache them
as part of the specific Statement instance. This introduces a lot of
lifetimes to be very explicit about what is valid for which time.

This is a full fix for diesel-rs#2663.

> 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 concequence this means we cannot just call `sqlite3_column_name` in
`Field::field_name()`. To fix this issue I decided to get all column
names pointers once after calling `step()` the first time and cache them
as part of the specific Statement instance. This introduces a lot of
lifetimes to be very explicit about what is valid for which time.
@weiznich weiznich requested review from Ten0 and a team March 12, 2021 10:00
@weiznich weiznich force-pushed the bug/fix_potential_use_after_free branch from b373b1f to 6576b08 Compare March 12, 2021 10:19
@weiznich weiznich added the run-benchmarks Used to indicate that github actions should run our benchmark suite label Mar 12, 2021
@weiznich
Copy link
Member Author

weiznich commented Mar 12, 2021

As expected this has an impact on performance

  • Insert statements seem to take ~1.15 times as long as before
  • Query statements with only one result seem to take up to 1.3 times as long as before (this is kind of expected as we do a bit more work on the first call to step()
  • QueryableByName get's a significant speedup for larger result sets, something like master is taking up to 1.7 times as long as the code in this PR.

The second and third change is kind of expected, while the first one is rather surprising.

We don't need to have column names for statements where we will only
receive the number of affected rows. This should hopefully remove the
performance impact of the previous fix for insert statements
@weiznich weiznich force-pushed the bug/fix_potential_use_after_free branch from 58e17c8 to 6c511d0 Compare March 15, 2021 09:49
@weiznich weiznich added run-benchmarks Used to indicate that github actions should run our benchmark suite and removed run-benchmarks Used to indicate that github actions should run our benchmark suite labels Mar 15, 2021
@weiznich weiznich merged commit 7578429 into diesel-rs:master Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-benchmarks Used to indicate that github actions should run our benchmark suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants