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.

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

Stop SpiClient soundness from regressing #1214

Merged
18 changes: 11 additions & 7 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ env:
# configured by `.cargo/config.toml` on macOS.
RUST_BACKTRACE: 1
CARGO_INCREMENTAL: "false"
SCCACHE_MAX_FRAME_LENGTH: 100000000
# CARGO_LOG: cargo::core::compiler::fingerprint=info # Uncomment this to output compiler fingerprint info

jobs:
Expand All @@ -25,7 +26,6 @@ jobs:
env:
RUSTC_WRAPPER: sccache
SCCACHE_DIR: /home/runner/.cache/sccache
RUSTFLAGS: -Copt-level=0

strategy:
fail-fast: false
Expand All @@ -45,7 +45,6 @@ jobs:
mv -f sccache-v0.2.15-x86_64-unknown-linux-musl/sccache $HOME/.local/bin/sccache
chmod +x $HOME/.local/bin/sccache
echo "$HOME/.local/bin" >> $GITHUB_PATH
echo 'SCCACHE_CACHE_SIZE="20G"' >> $GITHUB_ENV
mkdir -p /home/runner/.cache/sccache
echo ""

Expand Down Expand Up @@ -181,6 +180,11 @@ jobs:
--features "pg$PG_VER" \
--package pgrx-tests

# Midway cleanup?
- name: Clean up midway checkpoint
run: |
rm -rf ./target

- name: Run aggregate example tests
run: cargo test --package aggregate --features "pg$PG_VER" --no-default-features

Expand Down Expand Up @@ -253,8 +257,11 @@ jobs:
- name: Run triggers example tests
run: cargo test --package triggers --features "pg$PG_VER" --no-default-features

# - name: Run versioned_custom_libname_so example tests
# run: cargo test --package versioned_custom_libname_so --features "pg$PG_VER" --no-default-features
- name: Run versioned_custom_libname_so example tests
run: cargo test --package versioned_custom_libname_so --features "pg$PG_VER" --no-default-features

- name: Run versioned_so example tests
run: cargo test --package versioned_so --features "pg$PG_VER" --no-default-features

# Attempt to make the cache payload slightly smaller.
- name: Clean up built PGRX files
Expand All @@ -281,7 +288,6 @@ jobs:
env:
RUSTC_WRAPPER: sccache
SCCACHE_DIR: /home/runner/.cache/sccache
RUSTFLAGS: -Copt-level=0

strategy:
matrix:
Expand All @@ -301,7 +307,6 @@ jobs:
mv -f sccache-v0.2.15-x86_64-unknown-linux-musl/sccache $HOME/.local/bin/sccache
chmod +x $HOME/.local/bin/sccache
echo "$HOME/.local/bin" >> $GITHUB_PATH
echo 'SCCACHE_CACHE_SIZE="20G"' >> $GITHUB_ENV
mkdir -p /home/runner/.cache/sccache
echo ""

Expand Down Expand Up @@ -401,7 +406,6 @@ jobs:
if: "!contains(github.event.head_commit.message, 'nogha')"
env:
RUSTC_WRAPPER: sccache
SCCACHE_CACHE_SIZE: 20G
SCCACHE_DIR: /Users/runner/Library/Caches/Mozilla.sccache
SCCACHE_IDLE_TIMEOUT: 0

Expand Down
34 changes: 34 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pgrx-tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ rand = "0.8.5"

[dev-dependencies]
eyre = "0.6.8" # testing functions that return `eyre::Result`
trybuild = "1"

[dependencies.pgrx]
path = "../pgrx"
Expand Down
7 changes: 7 additions & 0 deletions pgrx-tests/tests/ui.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
use trybuild::TestCases;

#[test]
fn ui() {
let t = TestCases::new();
t.compile_fail("tests/ui/*.rs");
}
27 changes: 27 additions & 0 deletions pgrx-tests/tests/ui/escaping-spiclient-1209-cursor.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
use pgrx::prelude::*;
use std::error::Error;

#[pg_test]
fn issue1209() -> Result<Option<String>, Box<dyn Error>> {
// create the cursor we actually care about
let mut res = Spi::connect(|c| {
c.open_cursor("select 'hello world' from generate_series(1, 1000)", None)
.fetch(1000)
.unwrap()
});

// here we just perform some allocations to make sure that the previous cursor gets invalidated
for _ in 0..100 {
Spi::connect(|c| c.open_cursor("select 1", None).fetch(1).unwrap());
}

// later elements are probably more likely to point to deallocated memory
for _ in 0..100 {
res.next();
}

// segfault
Ok(res.next().unwrap().get::<String>(1)?)
}

fn main() {}
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like the frivolous mains but whatever.

Copy link
Contributor

Choose a reason for hiding this comment

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

might be able to avoid with #![crate_type = "lib"]? not sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried, unfortunately didn't work.

45 changes: 45 additions & 0 deletions pgrx-tests/tests/ui/escaping-spiclient-1209-cursor.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
error: lifetime may not live long enough
--> tests/ui/escaping-spiclient-1209-cursor.rs:8:9
|
7 | let mut res = Spi::connect(|c| {
| -- return type of closure is SpiTupleTable<'2>
| |
| has type `SpiClient<'1>`
8 | / c.open_cursor("select 'hello world' from generate_series(1, 1000)", None)
9 | | .fetch(1000)
10 | | .unwrap()
| |_____________________^ returning this value requires that `'1` must outlive `'2`

error[E0515]: cannot return value referencing temporary value
--> tests/ui/escaping-spiclient-1209-cursor.rs:8:9
|
8 | c.open_cursor("select 'hello world' from generate_series(1, 1000)", None)
| ^------------------------------------------------------------------------
| |
| _________temporary value created here
| |
9 | | .fetch(1000)
10 | | .unwrap()
| |_____________________^ returns a value referencing data owned by the current function
|
= help: use `.collect()` to allocate the iterator

error: lifetime may not live long enough
--> tests/ui/escaping-spiclient-1209-cursor.rs:15:26
|
15 | Spi::connect(|c| c.open_cursor("select 1", None).fetch(1).unwrap());
| -- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
| ||
| |return type of closure is SpiTupleTable<'2>
| has type `SpiClient<'1>`

error[E0515]: cannot return value referencing temporary value
--> tests/ui/escaping-spiclient-1209-cursor.rs:15:26
|
15 | Spi::connect(|c| c.open_cursor("select 1", None).fetch(1).unwrap());
| -------------------------------^^^^^^^^^^^^^^^^^^
| |
| returns a value referencing data owned by the current function
| temporary value created here
|
= help: use `.collect()` to allocate the iterator
13 changes: 13 additions & 0 deletions pgrx-tests/tests/ui/escaping-spiclient-1209-prep-stmt.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
use pgrx::prelude::*;
use std::error::Error;

#[pg_extern]
fn issue1209_prepared_stmt(q: &str) -> Result<Option<String>, Box<dyn Error>> {
use pgrx::spi::Query;

let prepared = { Spi::connect(|c| c.prepare(q, None))? };

Ok(Spi::connect(|c| prepared.execute(&c, Some(1), None)?.first().get(1))?)
}

fn main() {}
8 changes: 8 additions & 0 deletions pgrx-tests/tests/ui/escaping-spiclient-1209-prep-stmt.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: lifetime may not live long enough
--> tests/ui/escaping-spiclient-1209-prep-stmt.rs:8:39
|
8 | let prepared = { Spi::connect(|c| c.prepare(q, None))? };
| -- ^^^^^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
| ||
| |return type of closure is std::result::Result<PreparedStatement<'2>, pgrx::spi::SpiError>
| has type `SpiClient<'1>`