From 2694143e35a7969cb20cd59cd57ce0463ac9944d Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 24 Aug 2023 14:49:25 -0700 Subject: [PATCH 1/4] GetMemoryContextChunk -> ChunkContext --- pgrx-pg-sys/src/port.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pgrx-pg-sys/src/port.rs b/pgrx-pg-sys/src/port.rs index 5dca7b6a3..c2bde1a26 100644 --- a/pgrx-pg-sys/src/port.rs +++ b/pgrx-pg-sys/src/port.rs @@ -73,7 +73,7 @@ pub const unsafe fn MAXALIGN(len: usize) -> usize { /// This function will panic if `pointer` is null, if it's not properly aligned, or if the memory /// it points do doesn't have the a header that looks like a memory context pointer #[allow(non_snake_case)] -pub unsafe fn GetMemoryContextChunk(pointer: *mut std::os::raw::c_void) -> pg_sys::MemoryContext { +pub unsafe fn GetMemoryChunkContext(pointer: *mut std::os::raw::c_void) -> pg_sys::MemoryContext { // Postgres versions <16 don't export the "GetMemoryChunkContext" function. It's a "static inline" // function in `memutils.h`, so we port it to Rust right here #[cfg(any( From a78cd414ae7b77591aea38767d039cd703a29e53 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 24 Aug 2023 14:59:44 -0700 Subject: [PATCH 2/4] Fix doc comment --- pgrx-pg-sys/src/port.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pgrx-pg-sys/src/port.rs b/pgrx-pg-sys/src/port.rs index c2bde1a26..ab66b759c 100644 --- a/pgrx-pg-sys/src/port.rs +++ b/pgrx-pg-sys/src/port.rs @@ -71,9 +71,9 @@ pub const unsafe fn MAXALIGN(len: usize) -> usize { /// # Panics /// /// This function will panic if `pointer` is null, if it's not properly aligned, or if the memory -/// it points do doesn't have the a header that looks like a memory context pointer +/// it points to doesn't have a prefix that looks like a memory context pointer #[allow(non_snake_case)] -pub unsafe fn GetMemoryChunkContext(pointer: *mut std::os::raw::c_void) -> pg_sys::MemoryContext { +pub unsafe fn GetMemoryContextChunk(pointer: *mut std::os::raw::c_void) -> pg_sys::MemoryContext { // Postgres versions <16 don't export the "GetMemoryChunkContext" function. It's a "static inline" // function in `memutils.h`, so we port it to Rust right here #[cfg(any( From 5a045e874e08848c952e715f756d18a4eb0a175c Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 24 Aug 2023 16:05:05 -0700 Subject: [PATCH 3/4] Promote cfg and flatten --- pgrx-pg-sys/src/port.rs | 64 +++++++++++++++++++---------------------- pgrx/src/memcxt.rs | 4 +-- 2 files changed, 31 insertions(+), 37 deletions(-) diff --git a/pgrx-pg-sys/src/port.rs b/pgrx-pg-sys/src/port.rs index ab66b759c..954911b1c 100644 --- a/pgrx-pg-sys/src/port.rs +++ b/pgrx-pg-sys/src/port.rs @@ -73,46 +73,40 @@ pub const unsafe fn MAXALIGN(len: usize) -> usize { /// This function will panic if `pointer` is null, if it's not properly aligned, or if the memory /// it points to doesn't have a prefix that looks like a memory context pointer #[allow(non_snake_case)] -pub unsafe fn GetMemoryContextChunk(pointer: *mut std::os::raw::c_void) -> pg_sys::MemoryContext { +#[cfg(any( + feature = "pg11", + feature = "pg12", + feature = "pg13", + feature = "pg14", + feature = "pg15" +))] +pub unsafe fn GetMemoryChunkContext(pointer: *mut std::os::raw::c_void) -> pg_sys::MemoryContext { // Postgres versions <16 don't export the "GetMemoryChunkContext" function. It's a "static inline" // function in `memutils.h`, so we port it to Rust right here - #[cfg(any( - feature = "pg11", - feature = "pg12", - feature = "pg13", - feature = "pg14", - feature = "pg15" - ))] - { - /* - * Try to detect bogus pointers handed to us, poorly though we can. - * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an - * allocated chunk. - */ - assert!(!pointer.is_null()); - assert_eq!(pointer, MAXALIGN(pointer as usize) as *mut ::std::os::raw::c_void); + /* + * Try to detect bogus pointers handed to us, poorly though we can. + * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an + * allocated chunk. + */ + assert!(!pointer.is_null()); + assert_eq!(pointer, MAXALIGN(pointer as usize) as *mut ::std::os::raw::c_void); - /* - * OK, it's probably safe to look at the context. - */ - // context = *(MemoryContext *) (((char *) pointer) - sizeof(void *)); - let context = unsafe { - // SAFETY: the caller has assured us that `pointer` points to palloc'd memory, which - // means it'll have this header before it - *(pointer - .cast::<::std::os::raw::c_char>() - .sub(std::mem::size_of::<*mut ::std::os::raw::c_void>()) - .cast()) - }; + /* + * OK, it's probably safe to look at the context. + */ + // context = *(MemoryContext *) (((char *) pointer) - sizeof(void *)); + let context = unsafe { + // SAFETY: the caller has assured us that `pointer` points to palloc'd memory, which + // means it'll have this header before it + *(pointer + .cast::<::std::os::raw::c_char>() + .sub(std::mem::size_of::<*mut ::std::os::raw::c_void>()) + .cast()) + }; - assert!(MemoryContextIsValid(context)); + assert!(MemoryContextIsValid(context)); - context - } - - // Postgres 16 does **and** it's implemented different, so we'll just call it now that we can - #[cfg(feature = "pg16")] - pg_sys::GetMemoryChunkContext(pointer) + context } /// Returns true if memory context is valid, as Postgres determines such a thing. diff --git a/pgrx/src/memcxt.rs b/pgrx/src/memcxt.rs index 96a036c17..eac920b09 100644 --- a/pgrx/src/memcxt.rs +++ b/pgrx/src/memcxt.rs @@ -233,10 +233,10 @@ impl PgMemoryContexts { pub unsafe fn of(ptr: void_mut_ptr) -> Option { let parent = unsafe { // (un)SAFETY: the caller assumes responsibility for ensuring the provided pointer is - // going to be accepted by Postgres `GetMemoryContextChunk`. Postgres will ERROR + // going to be accepted by Postgres `GetMemoryChunkContext`. Postgres will ERROR // if its invariants aren't met, but who really knows when the ptr could have come // come from anywhere - pg_sys::GetMemoryContextChunk(ptr) + pg_sys::GetMemoryChunkContext(ptr) }; let memcxt = NonNull::new(parent)?; Some(PgMemoryContexts::Of(OwnerMemoryContext { memcxt })) From 05f7cb4eba696f750b3f3bd14dcc2b3ba01232df Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 24 Aug 2023 16:49:33 -0700 Subject: [PATCH 4/4] Fix MemoryContextIsValid port The previous version read an entire MemoryContextData. However, this is slightly dubious: the original Postgres version uses a macro, `IsA`, that casts to `*mut pg_sys::Node` first, then it reads the type-tag. The difference is in alignment, and in "spurious reads", as an entire struct may be assumed-valid. --- pgrx-pg-sys/src/port.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/pgrx-pg-sys/src/port.rs b/pgrx-pg-sys/src/port.rs index 954911b1c..3c144f12d 100644 --- a/pgrx-pg-sys/src/port.rs +++ b/pgrx-pg-sys/src/port.rs @@ -109,19 +109,19 @@ pub unsafe fn GetMemoryChunkContext(pointer: *mut std::os::raw::c_void) -> pg_sy context } -/// Returns true if memory context is valid, as Postgres determines such a thing. +/// Returns true if memory context is tagged correctly according to Postgres. /// /// # Safety /// -/// Caller must determine that the specified `context` pointer, if it's probably a [`MemoryContextData`] -/// pointer, really is. This function is a best effort, not a guarantee. +/// The caller must only attempt this on a pointer to a Node. +/// This may clarify if the pointee is correctly-initialized [`MemoryContextData`]. /// /// # Implementation Note /// /// If Postgres adds more memory context types in the future, we need to do that here too. #[allow(non_snake_case)] #[inline(always)] -pub unsafe fn MemoryContextIsValid(context: *mut crate::MemoryContextData) -> bool { +pub unsafe fn MemoryContextIsValid(context: crate::MemoryContext) -> bool { // #define MemoryContextIsValid(context) \ // ((context) != NULL && \ // (IsA((context), AllocSetContext) || \ @@ -130,11 +130,12 @@ pub unsafe fn MemoryContextIsValid(context: *mut crate::MemoryContextData) -> bo !context.is_null() && unsafe { - // SAFETY: we just determined that context isn't null, so it's safe to `.as_ref()` - // and `.unwrap_unchecked()` - (*context).type_ == crate::NodeTag_T_AllocSetContext - || (*context).type_ == crate::NodeTag_T_SlabContext - || (*context).type_ == crate::NodeTag_T_GenerationContext + // SAFETY: we just determined the pointer isn't null, and + // the caller asserts that it is being used on a Node. + let tag = (*context.cast::()).type_; + tag == crate::NodeTag_T_AllocSetContext + || tag == crate::NodeTag_T_SlabContext + || tag == crate::NodeTag_T_GenerationContext } }