diff --git a/Cargo.lock b/Cargo.lock index a36f8aee0d..d2660d7b57 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -159,7 +159,26 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fd4865004a46a0aafb2a0a5eb19d3c9fc46ee5f063a6cfc605c69ac9ecf5263d" dependencies = [ "bitflags", - "cexpr", + "cexpr 0.4.0", + "clang-sys", + "lazy_static", + "lazycell", + "peeking_take_while", + "proc-macro2", + "quote", + "regex", + "rustc-hash", + "shlex 0.1.1", +] + +[[package]] +name = "bindgen" +version = "0.59.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "453c49e5950bb0eb63bb3df640e31618846c89d5b7faa54040d76e98e0134375" +dependencies = [ + "bitflags", + "cexpr 0.5.0", "clang-sys", "clap", "env_logger", @@ -171,7 +190,7 @@ dependencies = [ "quote", "regex", "rustc-hash", - "shlex", + "shlex 1.0.0", "which", ] @@ -181,6 +200,18 @@ version = "1.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf1de2fe8c75bc145a2f577add951f8134889b4795d47466a54a5c846d691693" +[[package]] +name = "bitvec" +version = "0.19.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8942c8d352ae1838c9dda0b0ca2ab657696ef2232a20147cf1b30ae1a9cb4321" +dependencies = [ + "funty", + "radium", + "tap", + "wyz", +] + [[package]] name = "block-buffer" version = "0.9.0" @@ -244,7 +275,16 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f4aedb84272dbe89af497cf81375129abda4fc0a9e7c5d317498c15cc30c0d27" dependencies = [ - "nom", + "nom 5.1.2", +] + +[[package]] +name = "cexpr" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "db507a7679252d2276ed0dd8113c6875ec56d3089f9225b2b42c30cc1f8e5c89" +dependencies = [ + "nom 6.1.2", ] [[package]] @@ -542,6 +582,12 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2022715d62ab30faffd124d40b76f4134a550a87792276512b18d63272333394" +[[package]] +name = "funty" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fed34cd105917e91daa4da6b3728c47b068749d6a62c59811f06ed2ac71d9da7" + [[package]] name = "futures" version = "0.3.15" @@ -922,7 +968,7 @@ version = "6.17.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5da125e1c0f22c7cae785982115523a0738728498547f415c9054cb17c7e89f9" dependencies = [ - "bindgen", + "bindgen 0.57.0", "cc", "glob", "libc", @@ -1072,6 +1118,18 @@ dependencies = [ "version_check", ] +[[package]] +name = "nom" +version = "6.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e7413f999671bd4745a7b624bd370a569fb6bc574b23c83a3c5ed2e453f3d5e2" +dependencies = [ + "bitvec", + "funty", + "memchr", + "version_check", +] + [[package]] name = "ntapi" version = "0.3.6" @@ -1381,7 +1439,7 @@ name = "postgres_ffi" version = "0.1.0" dependencies = [ "anyhow", - "bindgen", + "bindgen 0.59.1", "byteorder", "bytes", "chrono", @@ -1392,6 +1450,7 @@ dependencies = [ "memoffset", "rand", "regex", + "serde", "thiserror", "workspace_hack", "zenith_utils", @@ -1480,6 +1539,12 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "radium" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "941ba9d78d8e2f7ce474c015eea4d9c6d25b6a3327f9832ee29a4de27f91bbb8" + [[package]] name = "rand" version = "0.8.4" @@ -1841,6 +1906,12 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7fdf1b9db47230893d76faad238fd6097fd6d6a9245cd7a4d90dbd639536bbd2" +[[package]] +name = "shlex" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "42a568c8f2cd051a4d283bd6eb0343ac214c1b0f1ac19f93e1175b2dee38c73d" + [[package]] name = "signal-hook-registry" version = "1.4.0" @@ -1987,6 +2058,12 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f764005d11ee5f36500a149ace24e00e3da98b0158b3e2d53a7495660d3f4d60" +[[package]] +name = "tap" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "55937e1799185b12863d447f42597ed69d9928686b8d88a1df17376a097d8369" + [[package]] name = "tar" version = "0.4.35" @@ -2531,6 +2608,12 @@ dependencies = [ "syn", ] +[[package]] +name = "wyz" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85e60b0d1b5f99db2556934e21937020776a5d31520bf169e851ac44e6420214" + [[package]] name = "xattr" version = "0.2.2" diff --git a/postgres_ffi/Cargo.toml b/postgres_ffi/Cargo.toml index 0060ed94b4..b4d21ce665 100644 --- a/postgres_ffi/Cargo.toml +++ b/postgres_ffi/Cargo.toml @@ -19,8 +19,9 @@ lazy_static = "1.4" log = "0.4.14" memoffset = "0.6.2" thiserror = "1.0" +serde = { version = "1.0", features = ["derive"] } workspace_hack = { path = "../workspace_hack" } zenith_utils = { path = "../zenith_utils" } [build-dependencies] -bindgen = "0.57" +bindgen = "0.59.1" diff --git a/postgres_ffi/README b/postgres_ffi/README index bc4c6c6806..5656314fd7 100644 --- a/postgres_ffi/README +++ b/postgres_ffi/README @@ -13,11 +13,6 @@ in each major PostgreSQL version. Currently, this module is based on PostgreSQL v14, but in the future we will probably need a separate copy for each PostgreSQL version. -To interact with the C structs, there is some unsafe code in this -module. Do not copy-paste that to the rest of the codebase! Keep the -amount of unsafe code to a minimum, and limited to this module only, -and only where it's truly needed. - TODO: Currently, there is also some code that deals with WAL records in pageserver/src/waldecoder.rs. That should be moved into this module. The rest of the codebase should not have intimate knowledge of diff --git a/postgres_ffi/build.rs b/postgres_ffi/build.rs index a29f2bbb6d..20270d03b5 100644 --- a/postgres_ffi/build.rs +++ b/postgres_ffi/build.rs @@ -3,6 +3,44 @@ extern crate bindgen; use std::env; use std::path::PathBuf; +use bindgen::callbacks::ParseCallbacks; + +#[derive(Debug)] +struct PostgresFfiCallbacks; + +impl ParseCallbacks for PostgresFfiCallbacks { + fn include_file(&self, filename: &str) { + // This does the equivalent of passing bindgen::CargoCallbacks + // to the builder .parse_callbacks() method. + let cargo_callbacks = bindgen::CargoCallbacks; + cargo_callbacks.include_file(filename) + } + + // Add any custom #[derive] attributes to the data structures that bindgen + // creates. + fn add_derives(&self, name: &str) -> Vec { + // This is the list of data structures that we want to serialize/deserialize. + let serde_list = [ + "XLogRecord", + "XLogPageHeaderData", + "XLogLongPageHeaderData", + "CheckPoint", + "FullTransactionId", + "ControlFileData", + ]; + + if serde_list.contains(&name) { + vec![ + "Default".into(), // Default allows us to easily fill the padding fields with 0. + "Serialize".into(), + "Deserialize".into(), + ] + } else { + vec![] + } + } +} + fn main() { // Tell cargo to invalidate the built crate whenever the wrapper changes println!("cargo:rerun-if-changed=pg_control_ffi.h"); @@ -19,20 +57,23 @@ fn main() { // Tell cargo to invalidate the built crate whenever any of the // included header files changed. // - .parse_callbacks(Box::new(bindgen::CargoCallbacks)) + .parse_callbacks(Box::new(PostgresFfiCallbacks)) // // These are the types and constants that we want to generate bindings for // - .whitelist_type("ControlFileData") - .whitelist_type("CheckPoint") - .whitelist_type("FullTransactionId") - .whitelist_type("XLogRecord") - .whitelist_type("XLogPageHeaderData") - .whitelist_type("XLogLongPageHeaderData") - .whitelist_var("XLOG_PAGE_MAGIC") - .whitelist_var("PG_CONTROL_FILE_SIZE") - .whitelist_var("PG_CONTROLFILEDATA_OFFSETOF_CRC") - .whitelist_type("DBState") + .allowlist_type("ControlFileData") + .allowlist_type("CheckPoint") + .allowlist_type("FullTransactionId") + .allowlist_type("XLogRecord") + .allowlist_type("XLogPageHeaderData") + .allowlist_type("XLogLongPageHeaderData") + .allowlist_var("XLOG_PAGE_MAGIC") + .allowlist_var("PG_CONTROL_FILE_SIZE") + .allowlist_var("PG_CONTROLFILEDATA_OFFSETOF_CRC") + .allowlist_type("DBState") + // Because structs are used for serialization, tell bindgen to emit + // explicit padding fields. + .explicit_padding(true) // // Path the server include dir. It is in tmp_install/include/server, if you did // "configure --prefix=". But if you used "configure --prefix=/", diff --git a/postgres_ffi/src/controlfile_utils.rs b/postgres_ffi/src/controlfile_utils.rs index 58c8a374c0..c9b11b28a9 100644 --- a/postgres_ffi/src/controlfile_utils.rs +++ b/postgres_ffi/src/controlfile_utils.rs @@ -43,6 +43,8 @@ impl ControlFileData { /// Interpret a slice of bytes as a Postgres control file. /// pub fn decode(buf: &[u8]) -> Result { + use zenith_utils::bin_ser::LeSer; + // Check that the slice has the expected size. The control file is // padded with zeros up to a 512 byte sector size, so accept a // larger size too, so that the caller can just the whole file @@ -55,26 +57,8 @@ impl ControlFileData { let OFFSETOF_CRC = Self::pg_control_crc_offset(); let expectedcrc = crc32c::crc32c(&buf[0..OFFSETOF_CRC]); - // Convert the slice into an array of the right size, and use `transmute` to - // reinterpret the raw bytes as a ControlFileData struct. - // - // NB: Ideally we would use 'zerocopy::FromBytes' for this, but bindgen doesn't - // derive FromBytes for us. The safety of this depends on the same constraints - // as for FromBytes, namely, all of its fields must implement FromBytes. That - // includes the primitive integer types, like `u8`, `u16`, `u32`, `u64` and their - // signed variants. But `bool` is not safe, because the contents of the high bits - // in a rust bool are undefined. In practice, PostgreSQL uses 1 to represent - // true and 0 for false, which is compatible with Rust bool, but let's try not to - // depend on it. - // - // FIXME: ControlFileData does contain 'bool's at the moment. - // - // See https://github.com/zenithdb/zenith/issues/207 for discussion on the safety - // of this. - let mut b: [u8; SIZEOF_CONTROLDATA] = [0u8; SIZEOF_CONTROLDATA]; - b.copy_from_slice(&buf[0..SIZEOF_CONTROLDATA]); - let controlfile: ControlFileData = - unsafe { std::mem::transmute::<[u8; SIZEOF_CONTROLDATA], ControlFileData>(b) }; + // Use serde to deserialize the input as a ControlFileData struct. + let controlfile = ControlFileData::des(buf)?; // Check the CRC if expectedcrc != controlfile.crc { @@ -93,21 +77,10 @@ impl ControlFileData { /// /// The CRC is recomputed to match the contents of the fields. pub fn encode(&self) -> Bytes { - // - // Use `transmute` to reinterpret struct as raw bytes. - // - // FIXME: This triggers undefined behavior, because the contents - // of the padding bytes are undefined, and this leaks those - // undefined bytes into the resulting array. The Rust code won't - // care what's in those bytes, and PostgreSQL doesn't care - // either. HOWEVER, it is a potential security issue, because the - // bytes can contain arbitrary pieces of memory from the page - // server. In the worst case, that could be private keys or - // another tenant's data. - // - // See https://github.com/zenithdb/zenith/issues/207 for discussion. - let b: [u8; SIZEOF_CONTROLDATA] = - unsafe { std::mem::transmute::(*self) }; + use zenith_utils::bin_ser::LeSer; + + // Serialize into a new buffer. + let b = self.ser().unwrap(); // Recompute the CRC let OFFSETOF_CRC = Self::pg_control_crc_offset(); diff --git a/postgres_ffi/src/lib.rs b/postgres_ffi/src/lib.rs index 88128bb7df..d7fe9b951d 100644 --- a/postgres_ffi/src/lib.rs +++ b/postgres_ffi/src/lib.rs @@ -4,6 +4,9 @@ // suppress warnings on rust 1.53 due to bindgen unit tests. // https://github.com/rust-lang/rust-bindgen/issues/1651 #![allow(deref_nullptr)] + +use serde::{Deserialize, Serialize}; + include!(concat!(env!("OUT_DIR"), "/bindings.rs")); pub mod controlfile_utils; diff --git a/postgres_ffi/src/xlog_utils.rs b/postgres_ffi/src/xlog_utils.rs index 2b6297fa10..e94da1de0a 100644 --- a/postgres_ffi/src/xlog_utils.rs +++ b/postgres_ffi/src/xlog_utils.rs @@ -271,23 +271,13 @@ pub fn main() { impl XLogRecord { pub fn from_bytes(buf: &mut Bytes) -> XLogRecord { - XLogRecord { - xl_tot_len: buf.get_u32_le(), - xl_xid: buf.get_u32_le(), - xl_prev: buf.get_u64_le(), - xl_info: buf.get_u8(), - xl_rmid: buf.get_u8(), - xl_crc: { - buf.advance(2); - buf.get_u32_le() - }, - } + use zenith_utils::bin_ser::LeSer; + XLogRecord::des_from(&mut buf.reader()).unwrap() } pub fn encode(&self) -> Bytes { - let b: [u8; XLOG_SIZE_OF_XLOG_RECORD]; - b = unsafe { std::mem::transmute::(*self) }; - Bytes::copy_from_slice(&b[..]) + use zenith_utils::bin_ser::LeSer; + self.ser().unwrap().into() } // Is this record an XLOG_SWITCH record? They need some special processing, @@ -298,34 +288,20 @@ impl XLogRecord { impl XLogPageHeaderData { pub fn from_bytes(buf: &mut B) -> XLogPageHeaderData { - let hdr: XLogPageHeaderData = XLogPageHeaderData { - xlp_magic: buf.get_u16_le(), - xlp_info: buf.get_u16_le(), - xlp_tli: buf.get_u32_le(), - xlp_pageaddr: buf.get_u64_le(), - xlp_rem_len: buf.get_u32_le(), - }; - buf.get_u32_le(); //padding - hdr + use zenith_utils::bin_ser::LeSer; + XLogPageHeaderData::des_from(&mut buf.reader()).unwrap() } } impl XLogLongPageHeaderData { pub fn from_bytes(buf: &mut B) -> XLogLongPageHeaderData { - XLogLongPageHeaderData { - std: XLogPageHeaderData::from_bytes(buf), - xlp_sysid: buf.get_u64_le(), - xlp_seg_size: buf.get_u32_le(), - xlp_xlog_blcksz: buf.get_u32_le(), - } + use zenith_utils::bin_ser::LeSer; + XLogLongPageHeaderData::des_from(&mut buf.reader()).unwrap() } pub fn encode(&self) -> Bytes { - let b: [u8; XLOG_SIZE_OF_XLOG_LONG_PHD]; - b = unsafe { - std::mem::transmute::(*self) - }; - Bytes::copy_from_slice(&b[..]) + use zenith_utils::bin_ser::LeSer; + self.ser().unwrap().into() } } @@ -333,17 +309,13 @@ pub const SIZEOF_CHECKPOINT: usize = std::mem::size_of::(); impl CheckPoint { pub fn encode(&self) -> Bytes { - let b: [u8; SIZEOF_CHECKPOINT]; - b = unsafe { std::mem::transmute::(*self) }; - Bytes::copy_from_slice(&b[..]) + use zenith_utils::bin_ser::LeSer; + self.ser().unwrap().into() } pub fn decode(buf: &[u8]) -> Result { - let mut b = [0u8; SIZEOF_CHECKPOINT]; - b.copy_from_slice(&buf[0..SIZEOF_CHECKPOINT]); - let checkpoint: CheckPoint; - checkpoint = unsafe { std::mem::transmute::<[u8; SIZEOF_CHECKPOINT], CheckPoint>(b) }; - Ok(checkpoint) + use zenith_utils::bin_ser::LeSer; + Ok(CheckPoint::des(buf)?) } // Update next XID based on provided new_xid and stored epoch. @@ -385,6 +357,7 @@ pub fn generate_wal_segment(pg_control: &ControlFileData) -> Bytes { xlp_tli: 1, // FIXME: always use Postgres timeline 1 xlp_pageaddr: pg_control.checkPoint - XLOG_SIZE_OF_XLOG_LONG_PHD as u64, xlp_rem_len: 0, + ..Default::default() // Put 0 in padding fields. } }, xlp_sysid: pg_control.system_identifier, @@ -404,6 +377,7 @@ pub fn generate_wal_segment(pg_control: &ControlFileData) -> Bytes { xl_info: pg_constants::XLOG_CHECKPOINT_SHUTDOWN, xl_rmid: pg_constants::RM_XLOG_ID, xl_crc: 0, + ..Default::default() // Put 0 in padding fields. }; let mut rec_shord_hdr_bytes = BytesMut::new();