Initial valgrind support (#1218)

Co-authored-by: Jubilee <[email protected]>

MEMORY_CHECKING.md

@@ -0,0 +1,21 @@
+# Memory Checking
+
+For some background see the writeup in <https://github.com/pgcentralfoundation/pgrx/issues/1217>.
+
+## Running with Memory Checking
+
+### Valgrind
+
+1. Install valgrind, headers, libs (on Fedora `sudo dnf install valgrind valgrind-devel valgrind-tools-devel` is enough).
+
+2. Run `cargo pgrx init --valgrind`. The only major downside to using this as your primary pgrx installation is that its slow, but you can still run without valgrind.
+
+3. Set `USE_VALGRIND` in the environment when running tests, for example `USE_VALGRIND=1 cargo test`. valgrind must be on the path for this to work. This is slow -- taking about 15 minutes on a very beefy cloud server, so manage your timing expectations accordingly.
+
+### Sanitizers
+
+TODO
+
+### Hardened Allocators
+
+For basic usage of electric fence or scudo, `LD_PRELOAD=libefence.so cargo test` or `LD_PRELOAD=libscudo.so cargo test`. More advanced usage (like GWP-ASAN) is still TODO.

cargo-pgrx/src/command/init.rs

@@ -70,6 +70,13 @@ pub(crate) struct Init {
     base_testing_port: Option<u16>,
     #[clap(long, help = "Additional flags to pass to the configure script")]
     configure_flag: Vec<String>,
+    /// Compile PostgreSQL with the necessary flags to detect a good amount of
+    /// memory errors when run under Valgrind.
+    ///
+    /// Building PostgreSQL with these flags requires that Valgrind be
+    /// installed, but the resulting build is usable without valgrind.
+    #[clap(long)]
+    valgrind: bool,
 }
 
 impl CommandExecute for Init {
@@ -352,8 +359,23 @@ fn configure_postgres(pg_config: &PgConfig, pgdir: &PathBuf, init: &Init) -> eyr
     let mut configure_path = pgdir.clone();
     configure_path.push("configure");
     let mut command = std::process::Command::new(configure_path);
+    // Some of these are redundant with `--enable-debug`.
+    let mut existing_cppflags = std::env::var("CPPFLAGS").unwrap_or_default();
+    existing_cppflags += " -DMEMORY_CONTEXT_CHECKING=1 \
+        -DCLOBBER_FREED_MEMORY=1 -DRANDOMIZE_ALLOCATED_MEMORY=1 ";
+    if init.valgrind {
+        // `USE_VALGRIND` allows valgrind to understand PG's memory context
+        // shenanigans. It requires Valgrind be installed (since it causes
+        // `postgres` to include a valgrind header), but uses macros which
+        // expand to asm statements that ultimately do nothing when valgrind is
+        // not connected, so using valgrind is not required even if the build is
+        // performed with `-DUSE_VALGRIND`.
+        let valgrind_flags = "-DUSE_VALGRIND=1 ";
+        existing_cppflags += valgrind_flags;
+    }
 
     command
+        .env("CPPFLAGS", existing_cppflags)
         .arg(format!("--prefix={}", get_pg_installdir(pgdir).display()))
         .arg(format!("--with-pgport={}", pg_config.port()?))
         .arg("--enable-debug")

pgrx-examples/arrays/src/lib.rs

@@ -107,8 +107,8 @@ mod vectors {
     extension_sql!(
         r#"
             do $$ begin raise warning 'creating sample data in the table ''vectors.data''.'; end; $$;
-            create table vectors.data as select vectors.random_vector(768) v from generate_series(1, 100000);
-    "#,
+            create table vectors.data as select vectors.random_vector(768) v from generate_series(1, 1000);
+        "#,
         name = "vectors_data",
         requires = [random_vector]
     );

pgrx-tests/src/framework.rs

@@ -446,8 +446,31 @@ fn start_pg(loglines: LogLines) -> eyre::Result<String> {
     wait_for_pidfile()?;
 
     let pg_config = get_pg_config()?;
-    let mut command =
-        Command::new(pg_config.postmaster_path().wrap_err("unable to determine postmaster path")?);
+    let postmaster_path =
+        pg_config.postmaster_path().wrap_err("unable to determine postmaster path")?;
+
+    let mut command = if use_valgrind() {
+        let mut cmd = Command::new("valgrind");
+        cmd.args([
+            "--leak-check=no",
+            "--gen-suppressions=all",
+            "--time-stamp=yes",
+            "--error-markers=VALGRINDERROR-BEGIN,VALGRINDERROR-END",
+            "--trace-children=yes",
+        ]);
+        // Try to provide a suppressions file, we'll likely get false positives
+        // if we can't, but that might be better than nothing.
+        if let Ok(path) = valgrind_suppressions_path(&pg_config) {
+            if path.exists() {
+                cmd.arg(format!("--suppressions={}", path.display()));
+            }
+        }
+
+        cmd.arg(postmaster_path);
+        cmd
+    } else {
+        Command::new(postmaster_path)
+    };
     command
         .arg("-D")
         .arg(get_pgdata_path()?.to_str().unwrap())
@@ -469,6 +492,13 @@ fn start_pg(loglines: LogLines) -> eyre::Result<String> {
     Ok(session_id)
 }
 
+fn valgrind_suppressions_path(pg_config: &PgConfig) -> Result<PathBuf, eyre::Report> {
+    let mut home = Pgrx::home()?;
+    home.push(pg_config.version()?);
+    home.push("src/tools/valgrind.supp");
+    Ok(home)
+}
+
 fn wait_for_pidfile() -> Result<(), eyre::Report> {
     const MAX_PIDFILE_RETRIES: usize = 10;
 
@@ -756,3 +786,7 @@ fn find_on_path(program: &str) -> Option<PathBuf> {
     let paths = std::env::var_os("PATH")?;
     std::env::split_paths(&paths).map(|p| p.join(program)).find(|abs| abs.exists())
 }
+
+fn use_valgrind() -> bool {
+    std::env::var_os("USE_VALGRIND").is_some_and(|s| s.len() > 0)
+}

pgrx-tests/src/tests/srf_tests.rs

@@ -257,7 +257,7 @@ mod tests {
     fn test_srf_setof_datum_detoasting_with_borrow() {
         let cnt = Spi::connect(|mut client| {
             // build up a table with one large column that Postgres will be forced to TOAST
-            client.update("CREATE TABLE test_srf_datum_detoasting AS SELECT array_to_string(array_agg(g),' ') s FROM (SELECT 'a' g FROM generate_series(1, 1000000)) x;", None, None)?;
+            client.update("CREATE TABLE test_srf_datum_detoasting AS SELECT array_to_string(array_agg(g),' ') s FROM (SELECT 'a' g FROM generate_series(1, 1000)) x;", None, None)?;
 
             // and make sure we can use the DETOASTED value with our SRF function
             let table = client.select(
@@ -268,14 +268,14 @@ mod tests {
 
             Ok::<_, spi::Error>(table.len() as i64)
         });
-        assert_eq!(cnt, Ok(1000000))
+        assert_eq!(cnt, Ok(1000))
     }
 
     #[pg_test]
     fn test_srf_table_datum_detoasting_with_borrow() {
         let cnt = Spi::connect(|mut client| {
             // build up a table with one large column that Postgres will be forced to TOAST
-            client.update("CREATE TABLE test_srf_datum_detoasting AS SELECT array_to_string(array_agg(g),' ') s FROM (SELECT 'a' g FROM generate_series(1, 1000000)) x;", None, None)?;
+            client.update("CREATE TABLE test_srf_datum_detoasting AS SELECT array_to_string(array_agg(g),' ') s FROM (SELECT 'a' g FROM generate_series(1, 1000)) x;", None, None)?;
 
             // and make sure we can use the DETOASTED value with our SRF function
             let table = client.select(
@@ -286,7 +286,7 @@ mod tests {
 
             Ok::<_, spi::Error>(table.len() as i64)
         });
-        assert_eq!(cnt, Ok(1000000))
+        assert_eq!(cnt, Ok(1000))
     }
 
     #[pg_test(error = "column \"cause_an_error\" does not exist")]