Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix undefined behaviour when loading control file #687

Merged
merged 3 commits into from
Sep 14, 2022
Merged

Fix undefined behaviour when loading control file #687

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ef01a1
Fix undefined behaviour when loading control file
syvb Sep 13, 2022
75daa91
Use extern "Rust" when function types not FFI-safe
syvb Sep 14, 2022
9ee567e
Document requirement for compiler to match
syvb Sep 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions cargo-pgx/src/command/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use object::Object;
use once_cell::sync::OnceCell;
use owo_colors::OwoColorize;
use pgx_pg_config::{get_target_dir, PgConfig, Pgx};
use pgx_utils::sql_entity_graph::{PgxSql, SqlGraphEntity};
use pgx_utils::sql_entity_graph::{ControlFile, PgxSql, SqlGraphEntity};
use std::{
collections::HashSet,
hash::{Hash, Hasher},
Expand Down Expand Up @@ -390,10 +390,14 @@ pub(crate) fn generate_schema(
.expect(&format!("Couldn't call __pgx_sql_mappings"));
sql_mapping = sql_mappings_symbol();

let symbol: libloading::os::unix::Symbol<unsafe extern "C" fn() -> SqlGraphEntity> = lib
let symbol: libloading::os::unix::Symbol<
unsafe extern "C" fn() -> eyre::Result<ControlFile>,
> = lib
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #[rustfmt::skip] is better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use #[rustfmt::skip].

.get("__pgx_marker".as_bytes())
.expect(&format!("Couldn't call __pgx_marker"));
let control_file_entity = symbol();
let control_file_entity = SqlGraphEntity::ExtensionRoot(
symbol().expect("Failed to get control file information"),
);
entities.push(control_file_entity);

for symbol_to_call in fns_to_call {
Expand Down