Expose sqlite's sqlite3_create_collation() #2564
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weiznich
reviewed
Nov 20, 2020
diesel/src/sqlite/connection/raw.rs
Outdated
| }; | ||
| } | ||
| let args = unsafe { slice::from_raw_parts(value_ptr, num_args as _) }; | ||
| let args = build_sql_function_args::<ArgsSqlType, Args>(args); |
There was a problem hiding this comment.
Technically this calls FromSql::from_sql internally which also can panic. Therefore I think that should be moved into the catch_unwind block below.
diesel/src/sqlite/connection/raw.rs
Outdated
Comment on lines
454
to
525
| assert_fail!( | ||
| "An unknown error occurred. user_ptr is a null pointer. This should never happen." | ||
| ); | ||
| }); | ||
|
|
||
| for (ptr, len, side) in &[(rhs_ptr, rhs_len, "rhs"), (lhs_ptr, lhs_len, "lhs")] { | ||
| if *len < 0 { | ||
| assert_fail!( | ||
| "An unknown error occurred. {}_len is negative. This should never happen.", | ||
| side | ||
| ); | ||
| } | ||
| if ptr.is_null() { | ||
| assert_fail!( | ||
| "An unknown error occurred. {}_ptr is a null pointer. This should never happen.", | ||
| side | ||
| ); |
There was a problem hiding this comment.
The other function implementations return errors in this case instead of aborting. I would favor that here too as this is more consistent + we do not control the sqlite source code so we cannot assume to much there.
diesel/src/sqlite/connection/raw.rs
Outdated
| std::cmp::Ordering::Less => -1, | ||
| }); | ||
| let result = std::panic::catch_unwind(f); | ||
| result.unwrap_or_else(|_| std::process::abort()) |
There was a problem hiding this comment.
We should just set the an error here like done for the other both cases above.
There was a problem hiding this comment.
So you mean I should set an error by using context_error_str?
Panics across FFI boundaries cause undefined behavior in current Rust. The aggregator functions are callbacks invoked from C (libsqlite). To safe-guard against panics, std::panic::catch_unwind() is used. On panic the functions now return with an error result indicating the unexpected panic occurred. std::panic::catch_unwind() requires types to implement std::panic::UnwindSafe, a marker trait indicating that care must be taken since panics introduce control-flow that is not very visible. Refer to https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html for a more detailed explanation. For SqliteAggregatorFunction::step() we must use std::panic::AssertUnwindSafe, since &mut references are never considered UnwindSafe, and the requirement to ensure unwind-safety is documented on the method. Of note is that in safe Rust, even if the method is not unwind-safe the language still guarantees memory-safety. The marker trait is mainly to prevent logic bugs.
This encompasses the pattern of eprintln!() + std::process::abort() and adds a request to open an issue if the message is observed.
This is essentially the same treatment that custom aggregate functions got in ee2f792.
Co-authored-by: Georg Semmler <Georg_semmler_05@web.de>
This fixes the handling of panics in custom SQLite functions by having a `catch_unwind` before ffi the boundary and by correctly setting the required `UnwindSafe` trait bounds in a similar way as it is done by rusqlite.
weiznich
force-pushed
the
sqlite-collation-function
branch
from
01a6a38
to
bfcf3d5
Compare
weiznich
changed the title
|
@weiznich |
weiznich
reviewed
Apr 15, 2021
weiznich
reviewed
Apr 15, 2021
weiznich
reviewed
Apr 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've taken over #2495.