Skip to content

Commit

Permalink
fix: crash from drop impls that panic via leak_and_drop_on_delete()
Browse files Browse the repository at this point in the history
If `PgMemoryContexts::leak_and_drop_on_delete()` leaks a type that `impl
Drop`, and that implementation happens to panic, then Postgres will
segfault because the callback function was not properly guarded.

As a drive-by, slightly cleanup error reporting in the test framework.
  • Loading branch information
eeeebbbbrrrr committed Oct 27, 2024
1 parent 0193fd0 commit 4e214c5
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 1 deletion.
6 changes: 5 additions & 1 deletion pgrx-tests/src/framework.rs
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,11 @@ fn monitor_pg(mut command: Command, cmd_string: String, loglines: LogLines) -> S

if line.contains("database system is ready to accept connections") {
// Postgres says it's ready to go
sender.send(session_id.clone()).unwrap();
if let Err(_) = sender.send(session_id.clone()) {
// The channel is closed. This is really early in the startup process
// and likely indicates that a test crashed Postgres
panic!("{}: `monitor_pg()`: failed to send back session_id `{session_id}`. Did Postgres crash?", "ERROR".red().bold());
}
is_started_yet = true;
}

Expand Down
44 changes: 44 additions & 0 deletions pgrx-tests/src/tests/memcxt_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,50 @@ mod tests {
assert!(did_drop.load(Ordering::SeqCst))
}

#[pg_test]
fn test_leak_and_drop_with_panic() {
let result = std::panic::catch_unwind(|| unsafe {
struct Thing;
impl Drop for Thing {
fn drop(&mut self) {
panic!("please don't crash")
}
}

PgMemoryContexts::Transient {
parent: PgMemoryContexts::CurrentMemoryContext.value(),
name: "test",
min_context_size: 4096,
initial_block_size: 4096,
max_block_size: 4096,
}
.switch_to(|context| {
context.leak_and_drop_on_delete(Thing);
});
});

assert!(result.is_err());
let err = result.unwrap_err();
let caught = err.downcast_ref::<pg_sys::panic::CaughtError>();

match caught {
// would indicate some kind of terribleness with pgrx panic handling
None => panic!("caught a panic that isn't a `pg_sys::panic::CaughtError`"),

// this is the type of error we expect. It comes to us here as the PostgresError
// variant because, while it was a Rust panic, it was from a function with #[pg_guard]
// directly called from Postgres
Some(&pg_sys::panic::CaughtError::PostgresError(ref report))
if report.message() == "please don't crash" =>
{
// ok!
}

// got some other kind of CaughtError variant we shouldn't have
_ => panic!("did not catch the correct error/panic type: {caught:?}"),
}
}

#[pg_test]
fn parent() {
unsafe {
Expand Down
2 changes: 2 additions & 0 deletions pgrx/src/memcxt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,8 @@ impl PgMemoryContexts {
/// this MemoryContext, the original instance of `T` will be resurrected and its `impl Drop`
/// will be called.
pub fn leak_and_drop_on_delete<T>(&mut self, v: T) -> *mut T {
use crate as pgrx;
#[pgrx::pg_guard]
unsafe extern "C" fn drop_on_delete<T>(ptr: void_mut_ptr) {
let boxed = Box::from_raw(ptr as *mut T);
drop(boxed);
Expand Down

0 comments on commit 4e214c5

Please sign in to comment.