Fix issues with error handling across FFI boundaries (#250)

* Fix issues with error handling across FFI boundaries This also makes a few functions unsafe, so we had to litter parts of the code with unsafe{} blocks. The primary user-facing change here is if users are using `pgx::direct_function_call()` to call Rust-based functions with the `#[pg_extern]` annotation, those functions now need to be called using `pgx::direct_pg_extern_function_call()`. In general, this is yet another follow-up to issue #241. * bump version to 0.1.25
pgcentralfoundation · Oct 19, 2021 · f38ab31 · f38ab31
1 parent 3ee5839
commit f38ab31
Show file tree

Hide file tree

Showing 20 changed files with 260 additions and 119 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "pgx-parent"
-version = "0.1.24"
+version = "0.1.25"
 authors = ["ZomboDB, LLC <[email protected]>"]
 edition = "2018"
 license = "MIT"

diff --git a/cargo-pgx/Cargo.toml b/cargo-pgx/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "cargo-pgx"
-version = "0.1.24"
+version = "0.1.25"
 authors = ["ZomboDB, LLC <[email protected]>"]
 edition = "2018"
 license = "MIT"
@@ -19,7 +19,7 @@ clap = { version = "2.33.3", features = [ "yaml" ] }
 colored = "2.0.0"
 env_proxy = "0.4.1"
 num_cpus = "1.13.0"
-pgx-utils = { path = "../pgx-utils", version = "0.1.24" }
+pgx-utils = { path = "../pgx-utils", version = "0.1.25" }
 proc-macro2 = { version = "1.0.28", features = [ "span-locations" ] }
 quote = "1.0.9"
 rayon = "1.5.1"

diff --git a/cargo-pgx/src/templates/cargo_toml b/cargo-pgx/src/templates/cargo_toml
@@ -15,11 +15,11 @@ pg13 = ["pgx/pg13", "pgx-tests/pg13" ]
 pg_test = []
 
 [dependencies]
-pgx = "0.1.24"
-pgx-macros = "0.1.24"
+pgx = "0.1.25"
+pgx-macros = "0.1.25"
 
 [dev-dependencies]
-pgx-tests = "0.1.24"
+pgx-tests = "0.1.25"
 
 [profile.dev]
 panic = "unwind"

diff --git a/pgx-macros/Cargo.toml b/pgx-macros/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "pgx-macros"
-version = "0.1.24"
+version = "0.1.25"
 authors = ["ZomboDB, LLC <[email protected]>"]
 edition = "2018"
 license = "MIT"
@@ -14,7 +14,7 @@ readme = "README.md"
 proc-macro = true
 
 [dependencies]
-pgx-utils = { path = "../pgx-utils", version = "0.1.24" }
+pgx-utils = { path = "../pgx-utils", version = "0.1.25" }
 proc-macro2 = "1.0.28"
 quote = "1.0.9"
 syn = { version = "1.0.75", features = [ "extra-traits", "full", "fold", "parsing" ] }

diff --git a/pgx-macros/src/rewriter.rs b/pgx-macros/src/rewriter.rs
@@ -484,7 +484,7 @@ impl PgGuardRewriter {
             #[allow(clippy::missing_safety_doc)]
             #[allow(clippy::redundant_closure)]
             #[allow(improper_ctypes_definitions)] /* for i128 */
-            pub unsafe extern "C" fn #func_name ( #arg_list_with_types ) #return_type {
+            pub unsafe fn #func_name ( #arg_list_with_types ) #return_type {
                 // as the panic message says, we can't call Postgres functions from threads
                 // the value of IS_MAIN_THREAD gets set through the pg_module_magic!() macro
                 #[cfg(debug_assertions)]

diff --git a/pgx-pg-sys/Cargo.toml b/pgx-pg-sys/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "pgx-pg-sys"
-version = "0.1.24"
+version = "0.1.25"
 authors = ["ZomboDB, LLC <[email protected]>"]
 edition = "2018"
 license = "MIT"
@@ -25,14 +25,14 @@ targets = ["x86_64-unknown-linux-gnu"]
 [dependencies]
 memoffset = "0.6.4"
 once_cell = "1.8.0"
-pgx-macros = { path = "../pgx-macros/", version = "0.1.24" }
+pgx-macros = { path = "../pgx-macros/", version = "0.1.25" }
 
 [build-dependencies]
 bindgen = "0.59.1"
 build-deps = "0.1.4"
 colored = "2.0.0"
 num_cpus = "1.13.0"
-pgx-utils = { path = "../pgx-utils/", version = "0.1.24" }
+pgx-utils = { path = "../pgx-utils/", version = "0.1.25" }
 proc-macro2 = "1.0.28"
 quote = "1.0.9"
 rayon = "1.5.1"

diff --git a/pgx-tests/Cargo.toml b/pgx-tests/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "pgx-tests"
-version = "0.1.24"
+version = "0.1.25"
 authors = ["ZomboDB, LLC <[email protected]>"]
 edition = "2018"
 license = "MIT"
@@ -29,9 +29,9 @@ no-default-features = true
 colored = "2.0.0"
 lazy_static = "1.4.0"
 libc = "0.2.101"
-pgx = { path = "../pgx", default-features = false, version= "0.1.24" }
-pgx-macros = { path = "../pgx-macros", version= "0.1.24" }
-pgx-utils = { path = "../pgx-utils", version= "0.1.24" }
+pgx = { path = "../pgx", default-features = false, version= "0.1.25" }
+pgx-macros = { path = "../pgx-macros", version= "0.1.25" }
+pgx-utils = { path = "../pgx-utils", version= "0.1.25" }
 postgres = "0.19.1"
 regex = "1.5.4"
 serde = "1.0.130"

diff --git a/pgx-tests/src/tests/array_tests.rs b/pgx-tests/src/tests/array_tests.rs
@@ -9,7 +9,19 @@ use serde_json::*;
 /// ```
 #[pg_extern]
 fn sum_array_i32(values: Array<i32>) -> i32 {
-    values.iter().map(|v| v.unwrap_or(0i32)).sum()
+    // we implement it this way so we can trap an overflow (as we have a test for this) and
+    // catch it correctly in both --debug and --release modes
+    let mut sum = 0_i32;
+    for v in values {
+        let v = v.unwrap_or(0);
+        let tmp = sum.overflowing_add(v);
+        if tmp.1 {
+            panic!("attempt to add with overflow");
+        } else {
+            sum = tmp.0;
+        }
+    }
+    sum
 }
 
 /// ```funcname

diff --git a/pgx-tests/src/tests/fcinfo_tests.rs b/pgx-tests/src/tests/fcinfo_tests.rs
@@ -117,53 +117,65 @@ mod tests {
     #[pg_test]
     unsafe fn test_takes_i16() {
         let input = 42i16;
-        let result =
-            direct_function_call::<i16>(super::takes_i16_wrapper, vec![input.into_datum()]);
+        let result = direct_pg_extern_function_call::<i16>(
+            super::takes_i16_wrapper,
+            vec![input.into_datum()],
+        );
         let result = result.expect("result is NULL");
         assert_eq!(result, input);
     }
 
     #[pg_test]
     unsafe fn test_takes_i32() {
         let input = 42i32;
-        let result =
-            direct_function_call::<i32>(super::takes_i32_wrapper, vec![input.into_datum()]);
+        let result = direct_pg_extern_function_call::<i32>(
+            super::takes_i32_wrapper,
+            vec![input.into_datum()],
+        );
         let result = result.expect("result is NULL");
         assert_eq!(result, input);
     }
 
     #[pg_test]
     unsafe fn test_takes_i64() {
         let input = 42i64;
-        let result =
-            direct_function_call::<i64>(super::takes_i64_wrapper, vec![input.into_datum()]);
+        let result = direct_pg_extern_function_call::<i64>(
+            super::takes_i64_wrapper,
+            vec![input.into_datum()],
+        );
         let result = result.expect("result is NULL");
         assert_eq!(result, input);
     }
 
     #[pg_test]
     unsafe fn test_takes_bool() {
         let input = true;
-        let result =
-            direct_function_call::<bool>(super::takes_bool_wrapper, vec![input.into_datum()]);
+        let result = direct_pg_extern_function_call::<bool>(
+            super::takes_bool_wrapper,
+            vec![input.into_datum()],
+        );
         let result = result.expect("result is NULL");
         assert_eq!(result, input);
     }
 
     #[pg_test]
     unsafe fn test_takes_f32() {
         let input = 42.424_244;
-        let result =
-            direct_function_call::<f32>(super::takes_f32_wrapper, vec![input.into_datum()]);
+        let result = direct_pg_extern_function_call::<f32>(
+            super::takes_f32_wrapper,
+            vec![input.into_datum()],
+        );
         let result = result.expect("result is NULL");
         assert!(result.eq(&input));
     }
 
     #[pg_test]
     unsafe fn test_takes_f64() {
         let input = 42.424_242_424_242f64;
-        let result =
-            direct_function_call::<f64>(super::takes_f64_wrapper, vec![input.into_datum()]);
+        let result = direct_pg_extern_function_call::<f64>(
+            super::takes_f64_wrapper,
+            vec![input.into_datum()],
+        );
         let result = result.expect("result is NULL");
         assert!(result.eq(&input));
     }
@@ -182,32 +194,36 @@ mod tests {
 
     #[pg_test]
     unsafe fn test_takes_option_with_null_arg() {
-        let result = direct_function_call::<i32>(super::takes_option_wrapper, vec![None]);
+        let result = direct_pg_extern_function_call::<i32>(super::takes_option_wrapper, vec![None]);
         assert_eq!(-1, result.expect("result is NULL"))
     }
 
     #[pg_test]
     unsafe fn test_takes_option_with_non_null_arg() {
         let input = 42i32;
-        let result =
-            direct_function_call::<i32>(super::takes_option_wrapper, vec![input.into_datum()]);
+        let result = direct_pg_extern_function_call::<i32>(
+            super::takes_option_wrapper,
+            vec![input.into_datum()],
+        );
         let result = result.expect("result is NULL");
         assert_eq!(result, input);
     }
 
     #[pg_test]
     unsafe fn test_takes_str() {
         let input = "this is a test";
-        let result =
-            direct_function_call::<&str>(super::takes_str_wrapper, vec![input.into_datum()]);
+        let result = direct_pg_extern_function_call::<&str>(
+            super::takes_str_wrapper,
+            vec![input.into_datum()],
+        );
         let result = result.expect("result is NULL");
         assert_eq!(result, input);
     }
 
     #[pg_test]
     unsafe fn test_takes_string() {
         let input = "this is a test".to_string();
-        let result = direct_function_call::<String>(
+        let result = direct_pg_extern_function_call::<String>(
             super::takes_str_wrapper,
             vec![input.clone().into_datum()],
         );
@@ -217,13 +233,13 @@ mod tests {
 
     #[pg_test]
     unsafe fn test_returns_some() {
-        let result = direct_function_call::<i32>(super::returns_some_wrapper, vec![]);
+        let result = direct_pg_extern_function_call::<i32>(super::returns_some_wrapper, vec![]);
         assert!(result.is_some());
     }
 
     #[pg_test]
     unsafe fn test_returns_none() {
-        let result = direct_function_call::<i32>(super::returns_none_wrapper, vec![]);
+        let result = direct_pg_extern_function_call::<i32>(super::returns_none_wrapper, vec![]);
         assert!(result.is_none())
     }
 

diff --git a/pgx-tests/src/tests/pg_try_tests.rs b/pgx-tests/src/tests/pg_try_tests.rs
@@ -1,13 +1,41 @@
 // Copyright 2020 ZomboDB, LLC <[email protected]>. All rights reserved. Use of this source code is
 // governed by the MIT license that can be found in the LICENSE file.
 
+use pgx::*;
+
+// if our Postgres ERROR and Rust panic!() handling is incorrect, this little bit of useless code
+// will crash postgres.  If things are correct it'll simply raise an ERROR saying "panic in walker".
+#[pg_extern]
+fn crash() {
+    unsafe {
+        let mut node = PgList::<pg_sys::Node>::new();
+        node.push(PgList::<pg_sys::Node>::new().into_pg() as *mut pg_sys::Node);
+
+        pg_sys::raw_expression_tree_walker(
+            node.into_pg() as *mut pg_sys::Node,
+            Some(walker),
+            std::ptr::null_mut(),
+        );
+    }
+}
+
+#[pg_guard]
+extern "C" fn walker() -> bool {
+    panic!("panic in walker");
+}
+
 #[cfg(any(test, feature = "pg_test"))]
 mod tests {
     #[allow(unused_imports)]
     use crate as pgx_tests;
 
     use pgx::*;
 
+    #[pg_test(error = "panic in walker")]
+    fn test_panic_in_extern_c_fn() {
+        Spi::get_one::<()>("SELECT crash()");
+    }
+
     #[pg_test]
     fn test_pg_try_unwrap_no_error() {
         let result = pg_try(|| 42).unwrap();