Fix undefined behaviour when loading control file #687
Conversation
|
There are... many fixes in 0.5.0 that will not appear in 0.4. This will be another one of them. |
cargo-pgx/src/command/schema.rs
Outdated
| let symbol: libloading::os::unix::Symbol< | ||
| unsafe extern "C" fn() -> eyre::Result<ControlFile>, | ||
| > = lib |
There was a problem hiding this comment.
It seems unlikely a Result returning function like this is actually FFI-safe, extern "C" fn or no. Rust enums are usually not FFI-safe except in some qualified cases. This probably works but it is probably still UB, unfortunately. Maybe this should be using extern "Rust" fn?
There was a problem hiding this comment.
That does look like UB to me. That issue also affects the other two external function calls in this file (to __pgx_sql_mappings and __pgx_internals*), which all return repr(Rust) enums.
I think even a extern "Rust" fn would still technically be UB since the compiler doesn't guarantee that the representation of an item is the same across compilation sessions (although in practice it usually is, at least when the target and compiler version are the same). Serializing data to a C array as it goes across the FFI boundary would remove that UB (although doing so would probably result in a minor hit to performance).
There was a problem hiding this comment.
I agree the guarantees are very thin here, but a repr(Rust) enum should have a consistent result, in practice, with the same compiler (there are various reasons why this has to be true), and is less likely to be a bear (on performance or otherwise).
There was a problem hiding this comment.
I updated the PR to make the relevant functions extern "Rust". extern "Rust" is implied if nothing is specified but I wrote it out explicitly on the relevant functions for clarity.
I also documented that the cargo-pgx compiler should match the compiler used to compile crates that use it.
There was a problem hiding this comment.
It turns out rustfmt doesn't like explicit extern "Rust", so I've made them implict. (rustfmt only changed the ones that weren't in a macro but I did all of them for consistency)
There was a problem hiding this comment.
I think #[rustfmt::skip] is better here.
There was a problem hiding this comment.
Updated to use #[rustfmt::skip].
|
I was able to determine that the cause of this issue is a rustc change sometime between beta 1.64 and nightly 2022-09-11. I don't think having an aarch64 CPU actually caused the issue, but instead the LLVM 14 -> 15 bump might be responsible. Edit to confirm: just tested on x86 Linux with the latest nightly and the same issue occurs |
syvb
force-pushed
the
0.5_fix-controlfile-ub
branch
2 times, most recently
from
9de4455
to
af09eb0
Compare
Various functions used in schema generator were marked as extern "C" despite not having FFI-safe types (and not being useful to call from C code).
syvb
force-pushed
the
0.5_fix-controlfile-ub
branch
from
af09eb0
to
9ee567e
Compare
…n#687) * Fix undefined behaviour when loading control file * Use extern "Rust" when function types not FFI-safe Various functions used in schema generator were marked as extern "C" despite not having FFI-safe types (and not being useful to call from C code). * Document requirement for compiler to match
536: Document requirement to use same compiler for cargo-pgx and Toolkit r=Smittyvb a=Smittyvb The same compiler must be used to compile cargo-pgx and Toolkit, or bad things (undefined behaviour) might happen. See pgcentralfoundation/pgrx#687 for why this is the case. This PR updates the installation documentation in the README to indicate this. I split the command in two since you only need to reinstall cargo-pgx, there's no need to re-run `cargo pgx init`. Co-authored-by: Smitty <smitty@timescale.com>
The type of
__pgx_markerisextern "C" fn() -> eyre::Result<ControlFile>, but it was incorrectly called asextern "C" fn() -> SqlGraphEntityincargo-pgx, which is undefined behavior since it's UB to assume that a valid value for aneyre::Resultis also a valid value for aSqlGraphEntity. On my M1 macOS device the undefined behavior results in an unconditionalFailed to get control file informationpanic. This PR fixes that.The only reason this ever worked was because on some platforms
eyre::Result<ControlFile>andSqlGraphEntityboth can have aControlFileafter a 1 byte long prefix. And it happened to be the case thatResult::OkandSqlGraphEntity::ExtensionRootboth correpond to a prefix of0.I've also wrote a similar fix for the same issue 0.4, I can make a PR against
masterwith that commit if you want to backport this to the 0.4.x release series.