Support for sqlite vtables as loadable extensions #631
Conversation
…sions # Conflicts: # Cargo.toml # libsqlite3-sys/Cargo.toml # libsqlite3-sys/build.rs # src/inner_connection.rs # src/lib.rs
|
|
|
The It was not referenced from rust code, only serving as a more semantically meaningful alias that required the earliest version of sqlite that we support that includes that stub in the header file (which is |
|
Would it possible to generate only one other bindgen file instead of two ? --- sqlite3/bindgen_bundled_version-ext.rs 2020-02-15 11:50:57.736323438 +0000
+++ sqlite3/bindgen_bundled_version-ext-embed.rs 2020-02-15 11:51:14.337342051 +0000
@@ -7895,11 +7895,13 @@
);
}
-// bindings were built with (non-embedded) loadable_extension:
-// we define our own sqlite_api static variable and export it
-// to C
-#[no_mangle]
-pub static mut sqlite3_api: *mut sqlite3_api_routines = 0 as *mut sqlite3_api_routines;
+// bindings were built with loadable_extension_embedded:
+// define sqlite3_api as an extern since this code will be embedded
+// within a loadable extension that defines and exports this itself
+extern "C" {
+ #[no_mangle]
+ pub static mut sqlite3_api: *mut sqlite3_api_routines;
+}
// sqlite3 API wrappers to support loadable extensions (Note: these were generated from build.rs - not by rust-bindgen)
|
If we need to regenerate old bindings, we can still do it with upgrade.sh
|
I don't know why we cannot use if cfg!(feature = "loadable_extension") {
panic!("Building a loadable extension bundled is not supported");
} |
Like sqlcipher
There was a problem hiding this comment.
Had been meaning to take a look at this (and couldn't sleep, so...). I'm a bit concerned by the potential for data races...
Not to mention, failing to initialize this will cause a segfault from safe code (admittedly, dereferencing null is unlikely to be that problematic in practice...).
That said, I have hit this issue in the past, and understand the desire for a fix...
LICENSE
Outdated
| @@ -1,4 +1,5 @@ | |||
| Copyright (c) 2014 John Gallagher <johnkgallagher@gmail.com> | |||
| Copyright (c) 2019 Genomics plc <info@genomicsplc.com> | |||
There was a problem hiding this comment.
We should consider just making this Copyright (c) 2014-2020 Rusqlite contributors or something. Since you haven't been requesting contributors assign copyright (which would be a bit of a pain) they retain it by default, so this would be more accurate anyway.
| }; | ||
| use rusqlite::{to_sqlite_error, Connection, Result}; | ||
|
|
||
| #[allow(clippy::not_unsafe_ptr_arg_deref)] |
There was a problem hiding this comment.
This probably should be unsafe fn though. It both writes to a static mut without synchronization and dereferences the raw pointers passed in as arguments.
There was a problem hiding this comment.
discussion moved to https://github.com/rusqlite/rusqlite/pull/910/files#r765317101
|
|
||
| // handle variadic api functions | ||
| if bare_fn.variadic.is_some() { | ||
| // until rust c_variadic support exists, we can't |
There was a problem hiding this comment.
This isn't really safe. Vararg calling convention isn't guaranteed to be the same as the fixed arg calling convention on all platforms / compilers (https://www.gnu.org/software/libc/manual/html_node/Calling-Variadics.html mentions this).
That said I guess it should be fine in practice, but still is worrisome...
There was a problem hiding this comment.
Discussion moved to https://github.com/rusqlite/rusqlite/pull/910/files#r765322613
| // within a loadable extension that defines and exports this itself | ||
| extern "C" { | ||
| #[no_mangle] | ||
| pub static mut sqlite3_api: *mut sqlite3_api_routines; |
There was a problem hiding this comment.
This worries me. I'd feel better if we synchronized access to it somehow...
At a minimum I guess we should document that this may be read from without synchronization by any call into rusqlite, and so the safety contract for use is that it must be initialized strictly before any thread could possibly read from it.
This goes for both declarations of sqlite3_api.
There was a problem hiding this comment.
The sqlite3_api symbol is the interface provided by sqlite to its run-time loadable extensions. The wrapper functions we generate (see
rusqlite/libsqlite3-sys/build.rs
Line 635 in cf658af
When we are compiled as an embedded extension, the contract with the extension we are embedded within is that the sqlite3_api symbol is initialised before our entrypoint is called.
When we are a non-embedded extension, it is our responsibility to set sqlite3_api as the first thing we do within our extension entrypoint (e.g.
rusqlite/dummy-extension/src/lib.rs
Line 19 in cf658af
I suppose there is no reason to expose the sqlite3_api symbol directly outside of libsqlite3-sys -- the only appropriate direct interactions with it are from the generated wrapper functions and to set it (once) at the start of the extension entrypoint.
Rather than asking the entrypoint to set the symbol directly, we could expose a pub function to initialise the extension, in which case the symbol could be private to the libsqlite3-sys crate, and I suppose we could also make sure that it is only initialised once.
I.e. something like:
use std::sync::Once;
static SQLITE_EXTENSION_INIT: Once = Once::new();
#[cfg(feature = "loadable_extension_embedded")]
pub fn sqlite_extension_init(_p_api: *mut ffi::sqlite3_api_routines) {}
#[cfg(all(
feature = "loadable_extension",
not(feature = "loadable_extension_embedded")
))]
pub fn sqlite_extension_init(p_api: *mut ffi::sqlite3_api_routines) {
SQLITE_EXTENSION_INIT.call_once(|| {
unsafe {
ffi::sqlite3_api = p_api;
}
});
}
Would that alleviate some of your concerns?
There was a problem hiding this comment.
More or less. Of course, sqlite_extension_init should be an unsafe fn.
There was a problem hiding this comment.
Discussion moved to #910 (comment)
|
Extension API function wrappers will be generated for all API functions in the sqlite bindings present at build time. It seems there is a trend towards encouraging building rusqlite against a very recent version of sqlite when any non-ancient features are requested (gating them all with the rusqlite/libsqlite3-sys/build.rs Line 638 in cf658af If we are going to encourage people to disable the version check by making it difficult to build against a minimum required version of sqlite, then we should probably protect the wrappers by including a runtime version check in them (probably using Once) and refusing to attempt to execute any belonging to a version newer than the runtime environment supports. |
|
Version requirements for the API routines are documented in comments in |
unresolved import `crate::util::alloc`
Codecov Report
@@ Coverage Diff @@
## master #631 +/- ##
==========================================
- Coverage 76.97% 76.70% -0.28%
==========================================
Files 46 47 +1
Lines 5325 5344 +19
==========================================
Hits 4099 4099
- Misses 1226 1245 +19
Continue to review full report at Codecov.
|
|
Sorry, last commits (on Jun 13) are wrong: at least changes on bindgen files... |
|
I've updated this PR to work with the current version of rusqlite here: https://github.com/phiresky/rusqlite/tree/gwenntest |
Thanks. Maybe I should close this PR/branch ? |
|
Uh probably not, I'm just a random person, I don't have access to this branch and am not enough knowledge to actually champion these changes (e.g. I might have broken Windows builds with this change phiresky@c409018#diff-c1c013e096409a5aea1f123c04254379057791785963eba5ef253fdd686570b8L526 ) |
|
I have opened a new PR here: #868 |
|
See a note in #910 regarding this PR not adhering to the conditions of the license under which code contributed in #532 was provided: #910 (comment) The issue here appears to have arisen out of a commit in which the "Genomics plc" copyright notice was removed and not (as far as I can tell) recreated elsewhere: 68bea40 I am sure this was not done maliciously, but these contributions are not owned by me and I only have permission from the owner (my employer) to contribute it under the terms of the license, including the specific copyright notice. Please stop distributing this code without including the required copyright notice. The easiest way to do that might be to revert (or rebase out of existence) the commit 68bea40 |
See https://github.com/rusqlite/rusqlite/blob/68bea40e806df992d11ff8d12480738fc34bb611/LICENSE
So [68bea40] explicitly removed |
@gwenn apologies for missing the move of our notice from build.rs to LICENSE had already happened by the time of 68bea40 and we are happy with that move. The problem actually occurred as you point out when master was merged: cb9e702#diff-c693279643b8cd5d248172d9c22cb7cf4ed163a3c98c8a3f69c2717edd3eacb7 |
Discussion moved to: https://github.com/rusqlite/rusqlite/pull/910/files#r765327393 |
Try to fix PR #532