sqlite enable/disable extension loading #2180

alamminsalo · 2019-10-04T15:57:34Z

resolves #1867

alamminsalo · 2019-10-05T16:40:28Z

That was a bit fiddle, but I managed to get the tests running ok. Testing is done by trying to load dummy extension and checking the error message that it produces. This is because it started to be too big of a hassle just to get mod_spatialite included as a dependency for every environment and I couldn't find any alternative test modules that would be easier to use in that sense.

weiznich · 2019-10-07T19:37:53Z

diesel/src/sqlite/connection/mod.rs

+    /// details.
+    ///
+    /// [`enable_load_extension` C function]: https://www.sqlite.org/c3ref/enable_load_extension.html
+    pub fn enable_load_extension(&self, enabled: bool) -> QueryResult<()> {


I think it would result in a clearer API if we have two functions here, instead of one with a boolean flag.

Added a separate method disable_load_extension for sqlite connection api (in b43a91f)

weiznich · 2019-10-07T19:38:31Z

diesel_tests/tests/load_extension.rs

@@ -0,0 +1,30 @@
+use diesel::{sql_query, Connection, RunQueryDsl, SqliteConnection};
+
+fn conn() -> SqliteConnection {


There is already such a function somewhere in diesel_tests

Fixed in b43a91f

diesel_tests/tests/load_extension.rs

weiznich · 2021-02-02T09:10:34Z

Rebased as part of the 2.0 release effort. Should be fine to go if CI is green.

weiznich · 2021-02-02T13:44:38Z

diesel/src/sqlite/connection/raw.rs

+
+    pub fn enable_load_extension(&self, enabled: bool) -> QueryResult<()> {
+        let result = unsafe {
+            ffi::sqlite3_enable_load_extension(self.internal_connection.as_ptr(), enabled as i32)


@diesel-rs/reviewers Seems like this function is not available on all supported platforms. For example it is missing in the default installation on MacOS and there is a configure flag to disable this function. Now this opens the question how we should handle this. I see the following options:

Don't implement this feature, which the obvious downsides of not supporting extension loading on sqlite.

Drop support for sqlite3 versions that do not have this functions. Not sure how large the impact of that would be, as you can just bundle sqlite3 in your program.

Do some checking here if that function is available.

On compile time, as this is a property of the provided library, but how would one implement that in a portable way.

As sqlite3 could be dynamically linked compile time checking could break, so we would need to do a dynamic lookup here, but against which library and how would that work with static linking

@weiznich , you've wrote in #3925 (comment)

For extensions there is #2180 which basically stalled because I found no way to detect at build time if the underlying sqlite library was build with extension support enabled or disabled.

Is it is not enough just to load sqlite library and try to get desired function using GetProcAddress / dlsym? Or maybe just try to compile a small program that links to that library and depends on the presence of that function and check the compiler status code?

I remember playing around with these functions back then while trying to get this working. It helps for builds that dynamically link libsqlite3, but it is problematic for static builds. Also it did not work for musl based builds. At that point I stopped trying to do that.

As for the test build option: That actually might work.

It helps for builds that dynamically link libsqlite3, but it is problematic for static builds.

I mean, try to load library dynamically in build script (not sure how to get path to load although) and then check presense of needed functions. Then output the necessary defines for the rust compiler

I don't think that will work as we cannot assume that there is a dynamic linkable library at all. For example the bundled feature flag for libsqlite3-sys only produces a static library.

In bundled mode you've already known which sqlite features are enabled or no? If it is compiled during build, maybe you even can control which build flags to use

That might work with the bundled feature, if it's even possible to detect that it's set on a dependency. (We could likely patch libsqlite3-sys to expose that). It won't work if the user supplies another statically build libsqlite3.a file on it's own. It also won't work for cross compilation.

This behavior is untested since we would need to add some extensions in order to test it. Fixes diesel-rs#1867

This reverts commit a227a61.

…all platforms. Now just tests if error message changes from not authorized

…nnection loading in test

Sytten · 2021-06-11T19:52:15Z

Any progress?

weiznich · 2021-06-11T20:08:05Z

@Sytten If you don't see any progress that means there is no progress. Asking such questions does not magically implement the feature, so please stop doing this.
That written: We are happy to receive your contribution to implement this feature.

donutsonhudson · 2021-11-28T03:29:52Z

diesel/src/sqlite/connection/raw.rs

+            let result = ffi::sqlite3_db_config(
+                self.internal_connection.as_ptr(),
+                SQLITE_DBCONFIG_ENABLE_LOAD_EXTENSION,
+                if enabled { 2 } else { 0 },


Hiya! I'm new to the Diesel project, and wanted to see what I could do to help this PR along. I'm also very new to the SQLite3 C API.

I was getting a MISUSE error from the ffi call after rebasing this branch off of the latest master, bcd32c8 and running ./bin/test.

While not documented in the official C API docs, I did find a mention in an O'Reilly source that sqlite3_db_config "must be called immediately following a call to one of the sqlite3_open_xxx() functions".

Modifying RawConnection::establish above (and also changing the first additional argument from 2 to 1), I think I've confirmed that the O'Reilly source is correct: we do have to call sqlite3_db_config immediately after the sqlite3_open_ function.

If this squares with your understanding of the SQLite C API, then I think the next step is to decide how we want to pass in the configuration options for sqlite3_db_config. Some open questions that I haven't really thought through yet:

How closely after the sqlite3_open call do we need to make the sqlite3_db_config call?

Do we want to do this as part of the establish function?

Is there a downside to automatically enable extension loading in the C API? Or should we keep the two explicit public functions to enable/disable extension loading?

The main issue here is that generally you don't want to enable extension loading for the whole application lifetime, but only for a very specific section of your code. Having it enabled for the whole application lifetime can result in a potential exploitable security issue due to a unauthorized user loading a malicious extension. Additionally this interface can be disabled at compile time. In that case just trying to use these ffi functions will cause a linker error as they are just not available. I've outlined several options how to handle that here. Ideally I would like to handle that without introducing an additional feature, so that the corresponding functions just return an error if the underlying functionality is not available from sqlite's side. At least the last time I've tried that I found no way to make this possible.

That all written: Any help implementing this feature is welcome.

Thanks for the quick reply! Your summary makes a lot of sense. From what you're saying I take it these are the two outstanding points from your perspective that need to be solved before we can merge this PR:

The duration of extension loading

Handling unsupported sqlite3 versions

I'll spend some time this week on both, specifically trying to answer these questions:

When will the Sqlite3 C API allow us to enable and disable extension loading via sqlite3_db_config?

If it needs to happen immediately upon creating the connection, how will we design the Diesel API to allow a client to safely enable extensions, and not expose themselves to malicious extension loading?

How can we seamlessly integrate checking support for extension loading, introducing nothing more than perhaps a new error message for the client to handle?

I'm also basing some of my research and thinking off of this comment from another sqlite3 thread about RETURNING.

The additional constraints are:

We must maintain support for older sqlite3 versions

We might be able to use a feature flag if we come up with a reasonable paradigm

At first glance this probably rules out using the rusqlite crate, which exactly what we need, but only supports Sqlite3 versions 3.6.8 or newer.

Sorry for the thought dump here... I have one more question: what is the minimum version of Sqlite3 that we want to support? Do we have any usage data from Diesel users?

For context, here's a timeline of Sqlite releases (with 3.6.8 released about 13 years ago)

It's totally fine to collect some ideas here 👍
Let me try to answer some of your questions:

From what you're saying I take it these are the two outstanding points from your perspective that need to be solved before we can merge this PR:

The duration of extension loading

Handling unsupported sqlite3 versions

At least from my perspective both points are not the main question. The main question is: "How can we provide the API in such a way that we don't need to introduce an additional feature flag for this".
Your two points need to be answered but at least I feel the answer is quite straight forward there:

Duration of extension loading:

Either provide an API that does all at once: enabling, loading, disabling

Or provide an API similar to that what rusqlite does

(Or even both)

Handling unsupported sqlite versions: Loading extensions is supported since 3.7.17 which was released in 2013. So we do not really need to care about versions not supporting this feature.
To elaborate a bit more on why I do not want to introduce another feature flag for this: Basically each new feature flag expands our test matrix. Theoretically we need to test all combinations of all feature flags to verify everything compiles with any combination. In practice that can be reduced a bit, but combined with the operating system variants and different rust versions our test matrix is already large. Adding another feature flag for this would increase the test matrix quite a bit. You might be able to convince me about adding a new feature flag if you have a solution to the test matrix problem.

At first glance this probably rules out using the rusqlite crate, which exactly what we need, but only supports Sqlite3 versions 3.6.8 or newer.

The interface provided by rusqlite looks like a good design. That written we cannot just use rusqlite internally in our SqliteConnection impl as that would mean:

That we have to rewrite the whole sqlite backend

We likely loose access to low level features, which are important for certain optimizations

Maybe it's not even possible to implement diesels connection and backend abstraction on top of the API provided by rusqlite as we make certain assumptions there.

what is the minimum version of Sqlite3 that we want to support?

Historically the minimal supported sqlite version was 3.7.16. We will likely bump that version to 3.20.0 with the upcoming 2.0 release. That means we just do not use any C API function that is not available in that release. We do support certain SQL constructs only available in newer releases. There it's normally so that we just expect that the user only constructs queries available supported by their SQLite versions. The only exception there is support for returning clauses as this is a really new feature and something that would be prominently exposed through diesels API. That's the reason why we think about a feature flag for that feature.

weiznich requested a review from a team October 7, 2019 19:35

weiznich reviewed Oct 7, 2019

View reviewed changes

weiznich added this to the 2.0 milestone Jan 26, 2021

weiznich force-pushed the master branch from f323c27 to d4d787e Compare February 2, 2021 09:09

weiznich requested a review from a team February 2, 2021 09:10

weiznich reviewed Feb 2, 2021

View reviewed changes

weiznich force-pushed the master branch 6 times, most recently from 6a1dd72 to cc333e8 Compare February 5, 2021 11:25

weiznich force-pushed the master branch 5 times, most recently from d941248 to adbdcc8 Compare February 16, 2021 08:21

sgrif and others added 11 commits February 16, 2021 09:22

Provide a minimal interface exposing SQLite's enable_load_extension

357bc62

This behavior is untested since we would need to add some extensions in order to test it. Fixes diesel-rs#1867

test loading extension mod_spatialite

b2ffb89

rename mod_spatialite to libspatialite.so.5

22c7a4d

enable_load_extension returns QueryResult

cd14030

rustfmt#

e5527e4

rename to mod_spatialite

5d0e3db

rename to libspatialite.so.7

47b1c5a

use mod_spatialite apt package and module in test

6ea30f8

try to get tests working

f527d8e

Revert "try to get tests working"

1e9aef2

This reverts commit a227a61.

fix tests, use schema connection

b74dd78

alamminsalo and others added 4 commits February 16, 2021 09:22

stop trying to load spatialite, as this proved to be a big hassle on …

f875697

…all platforms. Now just tests if error message changes from not authorized

add separate function for disabling sqlite ext loading. use common co…

c138f89

…nnection loading in test

revert the splitting api change into sqlite raw connection

a75cd1d

Remove unused imports

ef35744

weiznich force-pushed the master branch 21 times, most recently from 315f950 to 5fd79bf Compare February 17, 2021 07:25

Try to fix ci

a477eb7

weiznich force-pushed the master branch from 5fd79bf to a477eb7 Compare February 17, 2021 11:56

donutsonhudson reviewed Nov 28, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sqlite enable/disable extension loading #2180

sqlite enable/disable extension loading #2180

alamminsalo commented Oct 4, 2019

alamminsalo commented Oct 5, 2019

weiznich Oct 7, 2019

alamminsalo Oct 8, 2019 •

edited

weiznich Oct 7, 2019

alamminsalo Oct 8, 2019

weiznich commented Feb 2, 2021

weiznich Feb 2, 2021

Mingun Feb 8, 2024

weiznich Feb 8, 2024

Mingun Feb 8, 2024

weiznich Feb 8, 2024

Mingun Feb 8, 2024

weiznich Feb 9, 2024

Sytten commented Jun 11, 2021

weiznich commented Jun 11, 2021

donutsonhudson Nov 28, 2021

weiznich Nov 28, 2021

donutsonhudson Nov 30, 2021

donutsonhudson Nov 30, 2021

donutsonhudson Nov 30, 2021

weiznich Nov 30, 2021

		@@ -0,0 +1,30 @@
		use diesel::{sql_query, Connection, RunQueryDsl, SqliteConnection};

		fn conn() -> SqliteConnection {

sqlite enable/disable extension loading #2180

Are you sure you want to change the base?

sqlite enable/disable extension loading #2180

Conversation

alamminsalo commented Oct 4, 2019

alamminsalo commented Oct 5, 2019

Choose a reason for hiding this comment

alamminsalo Oct 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sytten commented Jun 11, 2021

weiznich commented Jun 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamminsalo Oct 8, 2019 •

edited