Bounds-checking debug assertions for array access #1514
Conversation
…within get() and the next() methods on Array's iterator types).
|
Thanks! First impression is this looks good, though I have to meditate on some of the possible pointer-inbounds edge cases here and figure out if any of them matter for this check. |
pgrx/src/datum/array.rs
Outdated
| /// Returns true if the pointer provided is within the bounds of the array. | ||
| /// Primarily intended for use with debug_assert!()s. | ||
| /// Note that this will return false for the 1-past-end, which is a useful | ||
| /// position for a pointer to be in, but not valid to dereference. | ||
| #[inline] | ||
| pub(crate) fn is_within_bounds(&self, ptr: *const u8) -> bool { | ||
| (self.raw.data_ptr() <= ptr) && (ptr < self.raw.end_ptr()) | ||
| } | ||
| /// Similar to [`Self::is_within_bounds()`], but also returns true for the | ||
| /// 1-past-end position. | ||
| #[inline] | ||
| pub(crate) fn is_within_bounds_inclusive(&self, ptr: *const u8) -> bool { | ||
| (self.raw.data_ptr() <= ptr) && (ptr <= self.raw.end_ptr()) | ||
| } |
There was a problem hiding this comment.
Something I am currently thinking through is whether these are resilient in the face of UB, LLVM optimizations, and arithmetic offset wrapping values around the integer addressing space.
There was a problem hiding this comment.
For RawArray::end_ptr, the implementation is:
/// "one past the end" pointer for the entire array's bytes
pub(crate) fn end_ptr(&self) -> *const u8 {
let ptr = self.ptr.as_ptr().cast::<u8>();
ptr.wrapping_add(unsafe { crate::varlena::varsize_any(ptr.cast()) })
}Notably, we should not trust varsize_any to be correct, but the ptr.wrapping_add returns a pointer that may be invalid, but is not, according to my understanding of LLVM's mechanics, inherently "bad" to compare against (where "bad" is much more nebulous than "UB"). But I am slightly suspicious because pointer comparisons are ill-defined in too many cases...
There was a problem hiding this comment.
Okay!
I think we have to trust the implementation of varsize_any anyways, thinking about it. Ideally, we should set up more tests for it. Opened #1539 for that.
After recaching my memory from the original issue and PR, I now remember: What we really need is to defend against LLVM deciding pointers with the same address are unequal, as that means we can't rely on comparison semantics being coherent:
- Miscompilation: Equal pointers comparing as unequal rust-lang/rust#107975
- incorrect transformation around icmp: unclear semantics of "lifetime" intrinsics leads to miscompilation llvm/llvm-project#45725
This means that we need to cast the pointer addresses to usize, and do the comparisons purely on integral values.
There was a problem hiding this comment.
Terse version:
Casting to usize won't save you, but I'll bet that you don't need this anyway, for several reasons. In the remote case you do, a core::hint::black_box() will serve you much better, and will be much more readable.
Verbose version:
Forgive me for intruding, but I'm worried you might be misremembering some small details of the bug you're worried about. Chief amongst them is the fact that casting the pointer addresses to usize does nothing to alleviate this problem. This example makes this absolutely clear.
That said, I'm not sure how much of a problem this is for your use-case. There are two reasons for this, one incidental and one fundamental.
The incidental one: In every single reproduction of this issue that I've managed to find, the pointers only miscompare when they're dangling. This pull-request appears to only compare non-dangling pointers, so it ought to dodge this specific issue.
The fundamental one: The main problem is that LLVM (and GCC, for that matter) think that two pointers can never be equal if they “come from” different allocations. If the pointers you're comparing are both “coming from” the same allocation –as appears to be the case as best as I can tell– then comparison will never misbehave.
The solution I propose: Rust has a specific function whose entire purpose is to tell the compiler “please pessimise this as best as you can”; that function is black_box, found in core::hint and std::hint. I've yet to encounter a reproduction where this didn't immediately fix the issue. (There's also (a^b).to_ne_bytes().into_iter().map(|x| x.count_ones()).sum() == 0, but that's both slow and plain unreadable.)
One thing to note: I can't help but notice, due to your title, that this entire discussion happens for some debug assertions. Thus far, this issue has only ever been encountered in release mode. If you manage to reproduce it with debug assertions enabled, that would be important information that should be noted in the issue that tracks it.
There was a problem hiding this comment.
@workingjubilee …was the above useful, after all?
There was a problem hiding this comment.
- Debug assertions and optimization levels are not values that are intrinsically connected, so I would prefer not to reason on that grounds.
std::hint::black_boxis not "please pessimize this as much as you can", it has a specific and more narrow behavior than that.- I have in fact noticed usize-casting interfering, though it has been a while so I don't have the reproduction code for such. Shrug.
| // Ensure we are not attempting to dereference a pointer | ||
| // outside of the array. | ||
| debug_assert!(self.is_within_bounds(ptr)); | ||
| // Prevent a datum that begins inside the array but would end | ||
| // outside the array from being dereferenced. | ||
| debug_assert!(self.is_within_bounds_inclusive( | ||
| ptr.wrapping_add(unsafe { self.slide_impl.hop_size(ptr) }) | ||
| )); |
There was a problem hiding this comment.
If we used a fn from the .offset family instead of .wrapping_offset then it's simply UB and any such check is elided, but hop_size does not do this inherently...
|
Opened related issue #1540 |
Welcome to pgrx 0.12.0-alpha.1! Say the magic words with me! ```shell cargo install cargo-pgrx --locked --version 0.12.0-alpha.1 ``` # Breaking Changes ## No more dlopen! Perhaps the most exciting change this round is @usamoi's contribution in #1468 which means that we no longer perform a `dlopen` in order to generate the schema. The cost, such as it is, is that your pgrx extensions now require a `src/bin/pgrx_embed.rs`, which will be used to generate the schema. This has much less cross-platform issues and will enable supporting things like `cargo binstall` down the line. It may be a bit touchy on first-time setup for transitioning older repos. If necessary, you may have to directly add a `src/bin/pgrx_embed.rs` and add the following code (which should be the only code in the file, though you can add comments if you like?): ```rust ::pgrx::pgrx_embed!(); ``` Your Cargo.toml will also want to update its crate-type key for the library: ```toml [lib] crate-type = ["cdylib", "lib"] ``` ## Library Code - pgrx-pg-sys will now use `ManuallyDropUnion` thanks to @NotGyro in #1547 - VARHDRSZ `const`s are no longer `fn`, thanks to @workingjubilee in #1584 - We no longer have `Interval::is_finite` since #1594 - We translate more `*_tree_walker` functions to the same signature their `*_impl` version in Postgres 16 has: #1596 - Thanks to @eeeebbbbrrrr in #1591 we no longer have the `pg_sql_graph_magic!()` macro, which should help with more things in the future! # What's New We have quite a lot of useful additions to our API: - `SpiClient::prepare_mut` was added thanks to @XeniaLu in #1275 - @usamoi also contributed bindings subscripting code in #1562 - For `#[pg_test]`, you have been able to use `#[should_panic(expected = "string")]` to anticipate a panic that contains that string in that test. For various reasons, `#[pg_test(error = "string")]` is much the same. Now, you can also use `#[pg_test(expected = "string")]`, in the hopes that is easier to stumble across, as of #1570 ## `Result<composite_type!("..."), E>` support - In #1560 @NotGyro contributed support for using `Result<composite_type!("Name"), E>`, as a case that had not been handled before. ## Significantly expanded docs Thanks to @rjuju, @NotGyro, and @workingjubilee, we now have significantly expanded docs for cargo-pgrx and pgrx in general. Some of these are in the API docs on https://docs.rs or the READMEs, but there's also a guide, now! It's not currently published, but is available as an [mdbook](https://github.com/rust-lang/mdBook) in the repo. Some diagnostic information that is also arguably documentation, like comments and the suggestion to `cargo install`, have also been improved, thanks to @workingjubilee in - #1579 - #1573 ## `#[pg_cast]` An experimental macro for a `CREATE CAST` was contributed by @xwkuang5 in #1445! ## Legal Stuff Thanks to @the-kenny in #1490 and @workingjubilee in #1504, it was brought to our attention that some dependencies had unusual legal requirements. So we fixed this with CI! We now check our code included into pgrx-using binaries is MIT/Apache 2.0 licensed, as is common across crates.io, using `cargo deny`!. The build tools will have more flexible legal requirements (partly due to the use of Mozilla Public License code in rustls). # Internal Changes Many internal cleanups were done thanks to - @workingjubilee in too many PRs to count! - @thomcc found a needless condition in #1501 - @nyurik in too many PRs to count! In particular: - we now actually `pfree` our `Array`s we detoasted as-of #1571 - creating a `RawArray` is now low-overhead due to #1587 ## Soundness Fixes We had a number of soundness issues uncovered or have added more tests to catch them. - Bounds-checking debug assertions for array access by @NotGyro in #1514 - Fix unsound `&` and `&mut` in `fcinfo.rs` by @workingjubilee in #1595 ## Less Deps Part of the cleanup by @workingjubilee was reducing the number of deps we compile: * cargo-pgrx: reduce trivial dep usages in #1499 * Update 2 syn in #1557 Hopefully it will reduce compile time and disk usage! ## New Contributors * @the-kenny made their first contribution in #1490 * @xwkuang5 made their first contribution in #1445 * @rjuju made their first contribution in #1516 * @nyurik made their first contribution in #1533 * @NotGyro made their first contribution in #1514 * @XeniaLu made their first contribution in #1275 **Full Changelog**: v0.12.0-alpha.0...v0.12.0-alpha.1
This introduces several
debug_assert!()calls around any attempt to dereference an element in a Postgres array, in order to catch memory bugs. It was written to resolve #1195.If RawArray's
end_ptr()ever pointed at something other than one byte past the end of the array's buffer, this might fail to catch memory bugs. However, it should be pointing to the end of the ArrayType's varlena, which includes the ArrayType header, its null bitmap, and its data, so this should work.