I'm looking for a pair or two of additional eyes on a complicated change in diesel, involving some tricky unsafe code blocks to uphold a few invariant of sqlites API.
The interesting part of this change handles binding values to sqlites prepared statements here. Our goal there is to reuse the allocations on rust side and skip copying the bind values in sqlite internally. This results in quite notable improvements in workloads involving binds with allocated data, like strings. (Up to 50% faster or so, the PR contains exact numbers).
Unfortunately does this change require quite a bit of unsafe code. By itself that's nothing tragic, as interacting with an ffi interface will always require unsafe code, but I would welcome a few additional reviewers here.
We differentiate between owned and borrowed allocated values there.
Borrowed values can be owned by the query there, that's the reason for leaking and freeing it later on (that's the part I'm not really comfortable with)
Owned values are owned by the bind collector, that's why we leak them and deallocate them later as part of the destructor .
I've already tried to write hostile a tests here and also run the whole test suite with the address sanitizer enabled. This does not show any error at all.
Thanks for taking the time to read that and looking forward to your response. If there is anything unclear about the context of code please ask for context, as I'm likely simplifying a lot of things here already.
The relevant section is in this file. More specifically the StatementUse::bind function and the Drop implementation for StatementUse. Everything else in that PR is required to make this work, but likely not that relevant to reason about the unsafe code in those two functions.
Okay, so you are actually writing a custom self-referential struct. Self-referential structs generally require some sort of manual cleanup. The design described here is seems reasonable at a high level, but I have several specific comments.
My first comment is don't use Box::leak for memory you aren't leaking. There's a Box::into_raw method you should be using instead. Additionally, to destroy a string you got from a Box<str>, you should be doing drop(Box::from_raw(...)) instead of using String::from_raw_parts.
Regarding the query / drop_query trick here, I note that T has no lifetime restrictions, so the type T could contain references to something that lives for a shorter timer than the StatementUse object. You might want to add a 'a bound on the T type, or perhaps even restrict it to 'static. Additionally, you can eliminate your null pointer by just moving it into the boxed closure and having it take no arguments.
I would simplify the owned_slices_to_free part by having only one field of type Vec<*mut [u8]>. The items would be the return value of Box::into_raw. If the box is a Box<str>, then simply convert it to a Box<[u8]> first using the appropriate From impl in the standard library.
You can replace the std::mem::replace with std::mem::take.
Thanks for the hints. I've pushed a improved version here. On the bright side this now rejects a compile tests, that drops a bind before the query (and the underlying statement) is dropped.
To summarize the changes:
I've introduced another lifetime to track the lifetime of the query. It's unfortunately not possible to reuse the lifetime 'a for this as this is bound to a mutable database connection, while the new lifetime 'b tracks only the lifetime of the query, which could be much shorter. This addresses the issue with the lifetime bound of T.
(The largest part of the diff contains just changes to make this work, through the various layers of diesels abstraction)
Instead of storing a raw pointer there I've just choose to store a boxed trait object of the query type there, as this removes some unsafe code.
Instead of doing the Box::leak/Box::into_raw/Box::from_raw I've chosen to just store the corresponding owned bind values directly, as again this removes some unsafe.
There is only one open point now, where I'm a bit unsure. What this whole code does is basically passing out shared references to the underlying buffer via sqlites ffi interface (see the Statement::bind function). Now in my understanding this results into having a owned value + a const * to the underlying buffer at the same time, which would violate rusts aliasing rules, right? This means I would need to wrap certain parts of the code (likely thebinds_to_free and _query field of StatementUse) in a UnsafeCell to communicate that to the compiler, or I'm wrong here?
In the end this simplifies the implementation quite a bit and I'm much more confident now that it does not contain a large bug, so thanks again for your review.
I don't think so. AFAIK raw pointers don't need to obey the "shared xor mutable" rule, and they are considered interior mutability primitives on their own right (from the PoV of compiler optimizations). So you only need to worry about aliasing across raw pointers when something explicitly requires that they do not alias (e.g. ptr::copy_nonoverlapping()). But otherwise, having multiple *const T and/or *mut T pointers to the same object is fine.
#[allow(missing_debug_implementations)]
pub struct StatementUse<'a, 'b> {
statement: MaybeCached<'a, Statement>,
column_names: OnceCell<Vec<*const str>>,
// we need to store the query here to ensure noone does
// drop it till the end ot the statement
// We use a boxed queryfragment here just to erase the
// generic type
_query: Box<dyn QueryFragment<Sqlite> + 'b>,
binds_to_free: Vec<(i32, Option<SqliteBindValue<'static>>)>,
}
impl<'a, 'b> StatementUse<'a, 'b> {
pub(super) fn bind<T>(mut statement: MaybeCached<'a, Statement>, query: T) -> QueryResult<Self>
where
T: QueryFragment<Sqlite> + QueryId + 'b,
Subjective nit: when using complex lifetimes, especially with unsafe code, despite popular belief, the Self alias ought to be replaced with what it mentions, so that the signature is fully readable on its own. This is especially meaningful if the associated function is far from the impl line:
Also, since 'b represents the region of owned-usability of the given query, then maybe you should name it accordingly, as well as 'a -> 'stmt for the maybe-borrow of the Statement?
impl<'a, 'b> StatementUse<'a, 'b> {
pub(super) fn bind<T>(mut statement: MaybeCached<'a, Statement>, query: T) -> QueryResult<Self>
where
T: QueryFragment<Sqlite> + QueryId + 'b,
{
let mut bind_collector = SqliteBindCollector::new();
query.collect_binds(&mut bind_collector, &mut ())?;
let SqliteBindCollector { metadata, binds } = bind_collector;
let mut binds_to_free = Vec::new();
for (idx, (bind, tpe)) in binds.into_iter().zip(metadata).enumerate() {
// Sqlite starts to count by 1
let bind_idx = idx as i32 + 1;
Note that iterator.enumerate() is the same as (0 ..).zip(iterable), so if you want to start by 1, and use i32s, I highly recommend that you do:
for (bind_idx, (bind, tpe))
in (1_i32 ..).zip(binds.into_iter().zip(metadata))
{
// It's safe to call bind here as:
// * The type and value matches
// * We ensure that corresponding buffers lives long enough below
// * The statement is not used yet by `step` or anything else
unsafe { statement.bind(tpe, &bind, bind_idx)? };
I cannot judge of the safety of this call, since Statement::bind does not spell out its /// Safety requirements.
After looking at this, it does look like the value parameter involves two lifetimes (you should try to enable the deny_elided_lifetimes_in_paths lint, especially if you are using unsafe code), so I'm assuming the lifetimes to live for at least as long as either the statement is dropped, or the bindings are "cleared" by being overwritten with bindings to "null placeholders" (which the drop impl does; for further clarity you could write the Dropinside this very function).
EDIT: After reading this whole snippet as a whole, I realize it's a wrapper offering a non-unsafe API for two "orthogonal issues": "bound arguments should not dangle" (which is tackled by the Drop impl and the vec) and the "column names shan't dangle". From there, I'd recommend that two wrappers be used, one tackling only the first issue, and then a second wrapper, containing it, which adds the "column names" invariant.
// We want to unbind the buffers later to ensure
// that sqlite does not access uninitilized memory
match bind {
SqliteBindValue::BorrowedString(_) | SqliteBindValue::BorrowedBinary(_) => {
binds_to_free.push((bind_idx, None));
}
SqliteBindValue::Binary(b) => {
binds_to_free.push((bind_idx, Some(SqliteBindValue::Binary(b))));
}
SqliteBindValue::String(b) => {
binds_to_free.push((bind_idx, Some(SqliteBindValue::String(b))));
}
_ => (),
}
}
Ok(Self {
statement,
column_names: OnceCell::new(),
binds_to_free,
_query: Box::new(query) as Box<_>,
})
}
According to the docs of column_name, IIUC, this one invalidates the potentially-cached column_names since it calls sqlite3_step, and yet offers Self nonetheless, thus allowing for:
let stmt_use = …;
let _ = stmt_use.field_name(1); // calls `column_name` and caches a pointer
if let Some(stmt_use) = stmt_use.step()? { // calls `sqlite3_step`
if let Some(coll_name) = stmt_use.field_name(1) { // dereferences the cached pointer
// According to the warnings written on the raw `column_name`
// function, this could be a dangling reference!
}
}
// 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.
//
// https://sqlite.org/c3ref/column_name.html
pub(super) unsafe fn column_name(&self, idx: i32) -> *const str {
let name = {
let column_name =
ffi::sqlite3_column_name(self.statement.inner_statement.as_ptr(), idx);
assert!(
!column_name.is_null(),
"The Sqlite documentation states that it only returns a \
null pointer here if we are in a OOM condition."
);
CStr::from_ptr(column_name)
};
name.to_str().expect(
"The Sqlite documentation states that this is UTF8. \
If you see this error message something has gone \
horribliy wrong. Please open an issue at the \
diesel repository.",
) as *const str
}
Note: I was initially surprised by having a function yielding a raw pointer and also being an unsafe fn on its own, but since .field_name() is a non-unsafe function yielding a Rust reference which would be invalidated by this function, the unsafe fn is warranted (although I'd personally make this function private inside field_name to make it clearer).
As it stands now, a /// Safety annotation regarding its misinteraction with .field_name() would be in order.
pub(super) fn index_for_column_name(&mut self, field_name: &str) -> Option<usize> {
(0..self.column_count())
.find(|idx| self.field_name(*idx) == Some(field_name))
.map(|v| v as usize)
}
pub(super) fn field_name(&mut self, idx: i32) -> Option<&str> {
if let Some(column_names) = self.column_names.get() {
return column_names
.get(idx as usize)
.and_then(|c| unsafe { c.as_ref() });
}
let values = (0..self.column_count())
.map(|idx| unsafe { self.column_name(idx) })
.collect::<Vec<_>>();
let ret = values.get(idx as usize).copied();
let _ = self.column_names.set(values);
ret.and_then(|p| unsafe { p.as_ref() })
}
Nit: this uses a &mut self receiver as well as a &-friendly OnceCell; which is kind of an oxymoron.
Either embrace &mut here:
column_names: Option<Vec<*const str>>,
let column_names = self.column_names.get_or_insert_with(|| {
/* define the raw `column_name` here (c.f. previous comment) */
(0 .. self.column_count())
.map(|idx| self.column_name(idx))
.collect()
});
column_names
.get(idx as usize)
.and_then(|c| unsafe { c.as_ref() })
or feature & access (AFAIK, this won't cause soundness problems with the lifetime, provided column_name remained private (or else it being an unsafe fn would indeed be warranted)):
impl<'a, 'b> Drop for StatementUse<'a, 'b> {
fn drop(&mut self) {
// First reset the statement, otherwise the bind calls
// below will fails
self.statement.reset();
// Reset the binds that may point to memory that will be/needs to be freed
for (idx, _buffer) in std::mem::take(&mut self.binds_to_free) {
unsafe {
// It's always safe to bind null values
self.statement
.bind(SqliteType::Text, &SqliteBindValue::Null, idx)
.expect("Binding nulls shouldn't ever fail");
}
}
}
}
to make the semantics involved explicit, so as to be more robust to refactorings.
And, if the .expect were to be remotely reachable, which would cause the binds_to_free to drop the remaining elements without clearing them from the statement, I'd then suggest to use ManuallyDrop in this snippet:
let mut binds_to_free = ManuallyDrop::new(mem::take(&mut self.binds_to_free));
binds_to_free.iter().for_each(|&(idx, _)| unsafe {
self.statement.bind…
});
// Now that all the stuff has been un-bound, we can finally free the buffers:
drop(ManuallyDrop::into_inner(binds_to_free));
And at that point, even more generally, you could be wrapping in a ManuallyDrop the .binds_to_free field altogether: it would avoid the .take(), as well as show the "subtle drop semantics" intent involved:
self.binds_to_free.iter()…
// Now that all the stuff has been un-bound, we can finally free the buffers:
ManuallyDrop::drop(&mut self.binds_to_free);
Way more unwind-safe this way, I'd say
Other than these things (the most important of those being the potential unsound interaction between .field_name() and .step()), everything looks quite good to me —I really like the Box<dyn …> improvements over the manual free (the one from the previous commit quoted in this thread), for instance
UnsafeCell wouldn't be the right tool here, since that one interacts with &-access that wants to temporarily feature mutable, and what you are mentioning is &mut or by-value access to the owned value (which is an exclusive access to that value, and, if high-level Rust types (≠ raw pointers) are used, such assumption from the compiler could be transitive through those.
I've detailed this problem in the following post (when talking about owning_ref problems).
Indeed, your code technically suffers from the same issues, but:
this is a Stacked Borrows issue which may not be applied to Rust (precisely because of how problematic in can be in cases such as yours);
in your case, the code dereferencing those pointers would be FFI code (sqlite3), so, in practice, it is unexploitable by the compiler. Although cross-language LTO could change that .
and the main argument: there is currently no general-purpose tool to opt out of the aforementioned aliasing requirements / assumptions. In that post, I do showcase a hand-rolled AliasableBox definition. In general, that would be the only true tool to opt out of "pointer-transitive lack-of-aliasing assumptions", so you'd need to do an AliasableVec definition as well, whereby wrapping the Vec raw parts (an example).
EDIT: I just found out about the ::aliasable crate, which does feature both an AliasableBox and an AliasableVec!
There is currently a partially-hacky general-purpose tool for these things, though: ManuallyDrop and MaybeUninit. Since both may be carrying stale data, their being valid to assume usable is a runtime property through which the alias analysis ought not be able to see. This means that ManuallyDrop<Box<…>> may have the same properties as AliasableBox<…> (or rather, lack thereof), as well as for Vec. Given that I've already recommended that the Vec be wrapped in a ManuallyDrop, it's all the more reason to do so, and do it for the Box as well (just don't forget to ManuallyDrop::drop(…) it after un-bind-ding the values).
Thanks again for the very helpful comments. I've pushed another improved version here. While working on that I noticed that the error path for StatementUse::bind could cause sqlite accessing values that are already freed, so another subtility to is now fixed.
Overall I'm quite confident in the corresponding code now, so thanks again for all the help .
.
.