Skip to content
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

Change default minimal SQLite API version #1221

Merged
merged 5 commits into from
Aug 30, 2022

Conversation

gwenn
Copy link
Collaborator

@gwenn gwenn commented Aug 21, 2022

From 3.6.8 to 3.14.0.
Use old_sqlite feature to keep 3.6.8 (or 3.7.16) as the minimal version.
Use modern_sqlite for SQLite API > 3.14.0.
Also remove old 3.6.23 and 3.7.7 bindings.

From 3.6.8 to 3.14.0.
Use `old_sqlite` feature to keep 3.6.8 (or 3.7.16) as the minimal version.
Use `modern_sqlite` for SQLite API > 3.14.0.
Also remove old 3.6.23 and 3.7.7 bindings.
@gwenn gwenn requested a review from thomcc August 21, 2022 09:26
@thomcc
Copy link
Member

thomcc commented Aug 21, 2022

I think there's an argument we shouldn't support these at all, as they have tons of security vulnerabilities. There's even an argument that under these our APIs aren't safe.

@gwenn
Copy link
Collaborator Author

gwenn commented Aug 21, 2022

I think there's an argument we shouldn't support these at all, as they have tons of security vulnerabilities. There's even an argument that under these our APIs aren't safe.

But in this case what should be the minimal version supported by rusqlite ?
The last available version ?

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Merging #1221 (3e4d24c) into master (5ea4c3b) will increase coverage by 0.40%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1221      +/-   ##
==========================================
+ Coverage   76.70%   77.11%   +0.40%     
==========================================
  Files          48       48              
  Lines        6238     6209      -29     
==========================================
+ Hits         4785     4788       +3     
+ Misses       1453     1421      -32     
Impacted Files Coverage Δ
src/error.rs 21.91% <0.00%> (ø)
src/functions.rs 78.34% <ø> (ø)
src/hooks.rs 88.51% <ø> (ø)
src/inner_connection.rs 66.29% <ø> (ø)
src/lib.rs 75.03% <ø> (+2.90%) ⬆️
src/raw_statement.rs 86.90% <ø> (ø)
src/statement.rs 92.70% <ø> (+0.78%) ⬆️
src/util/sqlite_string.rs 91.13% <ø> (ø)
src/vtab/mod.rs 59.23% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@thomcc
Copy link
Member

thomcc commented Aug 21, 2022

But in this case what should be the minimal version supported by rusqlite ? The last available version ?

Yeah I'm not sure. I think that's probably too aggressive. It's a tricky problem... We probably should not have an old_sqlite feature though, would be my hunch.

@gwenn
Copy link
Collaborator Author

gwenn commented Aug 22, 2022

But in this case what should be the minimal version supported by rusqlite ? The last available version ?

Yeah I'm not sure. I think that's probably too aggressive. It's a tricky problem... We probably should not have an old_sqlite feature though, would be my hunch.

Ok, il will try to remove old_sqlite feature but I guess we will have the same issue with winsqlite3 (< 3.14)...

@thomcc
Copy link
Member

thomcc commented Aug 22, 2022

winsqlite3 is a mess. It only works now on targets where extern "C" and extern "system" are the same. In retrospect, we probably shouldn't have added it since statically linking SQLite only adds ~1.0-1.5MB (for comparison, the Rust stdlib is ~7MB).

But yeah, idk. I think it's reasonable for us to cut off old versions at some point. Otherwise we have to support this stuff forever, no matter how broken and dangerous to use it is.

And associated bindgen_3.6.8.rs
@gwenn
Copy link
Collaborator Author

gwenn commented Aug 22, 2022

I was wrong: winsqlite3 seems not impacted which means >= 3.14.0 on the VM or I missed something.

@gwenn
Copy link
Collaborator Author

gwenn commented Aug 22, 2022

I chose version 3.14.0 but you may have a better choice ?

@thomcc
Copy link
Member

thomcc commented Aug 30, 2022

Seems fine for now, eventually libsqlite3-sys should be mostly agnostic to the version (and then i'd feel better about being more aggressive about this in rusqlite).

@thomcc thomcc merged commit 2dd1114 into rusqlite:master Aug 30, 2022
@gwenn gwenn deleted the min_sqlite_version branch September 2, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants