Fixed out of bounds reads for open_cursor #1837

YohDeadfall · 2024-09-02T17:08:33Z

The problem arises when the prepared statement has less arguments than
the used plan since there's no parameter to pass the number of arguments
like it's for SPI_execute_with_args. Meanwhile the execute method
does a check.

Introduce a variant, try_open_cursor, that returns an SpiResult instead.

workingjubilee · 2024-09-02T19:37:57Z

can you add a FIXME for it? thanks.

workingjubilee · 2024-09-02T19:39:00Z

The main reason it doesn't return a Result is that all of this was a relatively under-designed API that has been scheduled for total demolition and replacement, so comparatively minor but still breaking changes like "maybe we should have returned a Result here..." have been on-hold.

workingjubilee · 2024-09-02T19:44:54Z

pgrx/src/spi/query.rs

-        let args = args.unwrap_or_default();
-
-        let (mut datums, nulls): (Vec<_>, Vec<_>) = args.into_iter().map(prepare_datum).unzip();
+        let (mut datums, nulls) = self.args_to_datums(args).unwrap();


specifically adding the FIXME right here.

Also alternatively: add fn try_open_cursor, then tag this one with #[deprecated(since = "0.12.2", note = "lol ub")], and then replace the inner impl of this with just self.try_open_cursor(_client, args).unwrap().

Will write the new method too then, that's not an issue for me. And actually, I have other plans for improvements there and around (:

workingjubilee

that said, adding new API is optional, but we do need a #[should_panic] test on this case for open_cursor, or a "returns Err" test on the try_open_cursor fn, either one.

workingjubilee · 2024-09-02T21:11:48Z

It feels a little early to bump to 0.13, not that I'm afraid of such, but yk "a month" seems like a good span.

YohDeadfall · 2024-09-04T15:42:28Z

I addressed your changes, but what I honestly don't like is #[allow(deprecated)] attributes in tests, but if with the next breaking version it will be removed that seems fine.

If you share your plans on the SPI part, then I can contribute by reshaping it.

workingjubilee · 2024-09-05T22:16:29Z

If you share your plans on the SPI part, then I can contribute by reshaping it.

The most fundamental issue is that the API does not enforce lifetime bounds properly. Solving this is hard, but it is why 0.12 landed a huge breaking change for most pgrx-using code: solving the problem with the pgrx::spi API and then not fixing the entire rest of everything was basically closing the barn door well after every cow had not only run out but acquired demonic powers, picked up a hellforged weapon, and turned into a roaming infernal bovine.

It also has some overly complicated parts, like the overly-cutesy Query trait, which I would wish to move away from.

workingjubilee

thank you, this looks good.

workingjubilee · 2024-09-05T22:18:37Z

pgrx-tests/src/tests/spi_tests.rs

+        Spi::connect(|mut client| {
+            client.update("CREATE TABLE tests.cursor_table (id int)", None, None)?;
+            client.update(
+                "INSERT INTO tests.cursor_table (id) \
+            SELECT i FROM generate_series(1, 10) AS t(i)",
+                None,
+                None,
+            )?;
+            let prepared = client.prepare(
+                "SELECT * FROM tests.cursor_table WHERE id = $1",
+                Some([PgBuiltInOids::INT4OID.oid()].to_vec()),
+            )?;
+            client.open_cursor(&prepared, args);
+            unreachable!();
+        })


huh, does this return an error because we panic during the connection? interesting.

Right, it panics and the tests are marked as #[should_panic]. That allows us to verify that there's no undefined behavior.

Interesting.

Okay, it's a bit incorrect without the reason I haven't provided for tests because unreachable!! panics too and gives us a false negative. See #1843 for a fix.

workingjubilee · 2024-09-05T22:20:28Z

pgrx/src/spi/client.rs

+    /// # Panics
+    ///
+    /// Panics if a cursor wasn't opened.
+    #[deprecated(since = "0.12.2", note = "undefined behavior")]


hm. on second thought, not entirely sure if we want to deprecate this just because it panics... otoh most people don't like panics...

So, deprecate or leave it as it is for now? Personally I hate panicking, but there's a lot of upcoming work with breaking changes anyway, I guess, so forcing users to fix their code on each release isn't wise.

Yeah, I've reconsidered and I think we should remove the deprecation note, I don't want to churn people pointlessly. I'll try to mention it in the release notes.

or @eeeebbbbrrrr will. :^)

unless he feels that we should in fact deprecate this because it started panicking (but only when the alternative was UB).

workingjubilee

stripping the deprecated stuff mostly, sorry about changing my mind back and forth.

pgrx-tests/src/tests/spi_tests.rs

workingjubilee · 2024-09-06T06:35:27Z

pgrx-tests/src/tests/spi_tests.rs

@@ -532,6 +577,7 @@ mod tests {
    }

    #[pg_test]
+    #[allow(deprecated)]


Suggested change

#[allow(deprecated)]

pgrx/src/spi/client.rs

pgrx/src/spi/query.rs

workingjubilee · 2024-09-06T06:53:46Z

Thanks!

@SamuelMarks

Welcome to pgrx v0.12.2. This is a minor release that fixes a few bugs, improves compilation times with `cargo pgrx run/install/test`, and adds a few more Postgres headers. As usual, please `cargo install cargo-pgrx --version 0.12.2 --locked`. Then you can run `cargo pgrx upgrade` in your extension crate's root to update its dependencies. ## What's Changed * [ `CPPFLAGS`] Switch to `USE_ASSERT_CHECKING` by @SamuelMarks in #1826 * Add UnboxDatum for Range<T> by @mhov in #1827 * Moved Sized from FromDatum methods to trait by @YohDeadfall in #1831 * Used SPI result type for cursor API by @YohDeadfall in #1836 * always compile `pgrx_embed_*` without opts by @eeeebbbbrrrr in #1838 * Fixed out of bounds reads for open_cursor by @YohDeadfall in #1837 * Include `access/visibilitymap.h` and `utils/tuplestore.h` by @eeeebbbbrrrr in #1841 ## New Contributors Thanks to these folks for their first-time contributions -- it's greatly appreciated! * @SamuelMarks made their first contribution in #1826 * @YohDeadfall made their first contribution in #1831 **Full Changelog**: v0.12.1...v0.12.2

Fixes issues left from #1837, but nothing critical. The problem with `unreachable!` in tests is that it requires to have a non-empty `expected` in the `#[should_panic]` attribute. So we provide a message to verify that panicking happens for the right reason.

@YohDeadfall

Welcome to pgrx v0.12.3. This point release upgrades to use the new Postgres 17rc1. As usual, please `cargo install cargo-pgrx --version 0.12.3 --locked`. Then you can run `cargo pgrx upgrade` in your extension crate's root to update its dependencies. ## What's Changed * Fixups of issues left from #1837 by @YohDeadfall in #1843 * move to Postgres v17rc1 by @eeeebbbbrrrr in #1846 **Full Changelog**: v0.12.2...v0.12.3

@YohDeadfall

Welcome to pgrx v0.12.3. This point release upgrades to use the new Postgres 17rc1. As usual, please `cargo install cargo-pgrx --version 0.12.3 --locked`. Then you can run `cargo pgrx upgrade` in your extension crate's root to update its dependencies. ## What's Changed * Fixups of issues left from pgcentralfoundation#1837 by @YohDeadfall in pgcentralfoundation#1843 * move to Postgres v17rc1 by @eeeebbbbrrrr in pgcentralfoundation#1846 **Full Changelog**: pgcentralfoundation/pgrx@v0.12.2...v0.12.3

Fixed out of bounds reads for open_cursor

c5d010d

workingjubilee reviewed Sep 2, 2024

View reviewed changes

workingjubilee requested changes Sep 2, 2024

View reviewed changes

YohDeadfall added 4 commits September 4, 2024 18:34

Added try_open_cursor method

db3b642

Marked open_cursor as deprecated

d807648

Added regression tests for open_cursor

c9faf1e

Added try open_cursor methods to SpiCLient

2be84f4

YohDeadfall requested a review from workingjubilee September 4, 2024 15:42

Fixed links in the docs

571d7aa

YohDeadfall mentioned this pull request Sep 5, 2024

Changed query methods to accept slices #1840

Closed

workingjubilee approved these changes Sep 5, 2024

View reviewed changes

workingjubilee approved these changes Sep 6, 2024

View reviewed changes

flip-flop on deprecatedness

bc10a92

workingjubilee merged commit a772c49 into pgcentralfoundation:develop Sep 6, 2024
14 checks passed

YohDeadfall deleted the query-open-cursor-safety branch September 6, 2024 08:45

eeeebbbbrrrr mentioned this pull request Sep 6, 2024

Update version to 0.12.2 #1842

Merged

YohDeadfall mentioned this pull request Sep 6, 2024

Fixups of issues left from #1837 #1843

Merged

eeeebbbbrrrr mentioned this pull request Sep 9, 2024

Update version to 0.12.3 #1847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed out of bounds reads for open_cursor #1837

Fixed out of bounds reads for open_cursor #1837

YohDeadfall commented Sep 2, 2024 •

edited by workingjubilee

Loading

workingjubilee commented Sep 2, 2024

workingjubilee commented Sep 2, 2024 •

edited

Loading

workingjubilee Sep 2, 2024 •

edited

Loading

YohDeadfall Sep 2, 2024

workingjubilee left a comment

workingjubilee commented Sep 2, 2024

YohDeadfall commented Sep 4, 2024

workingjubilee commented Sep 5, 2024

workingjubilee left a comment

workingjubilee Sep 5, 2024

YohDeadfall Sep 5, 2024

workingjubilee Sep 6, 2024

YohDeadfall Sep 6, 2024

workingjubilee Sep 5, 2024

YohDeadfall Sep 5, 2024

workingjubilee Sep 6, 2024

workingjubilee Sep 6, 2024 •

edited

Loading

workingjubilee left a comment

workingjubilee Sep 6, 2024

workingjubilee commented Sep 6, 2024

Fixed out of bounds reads for open_cursor #1837

Fixed out of bounds reads for open_cursor #1837

Conversation

YohDeadfall commented Sep 2, 2024 • edited by workingjubilee Loading

workingjubilee commented Sep 2, 2024

workingjubilee commented Sep 2, 2024 • edited Loading

workingjubilee Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee commented Sep 2, 2024

YohDeadfall commented Sep 4, 2024

workingjubilee commented Sep 5, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Sep 6, 2024

YohDeadfall commented Sep 2, 2024 •

edited by workingjubilee

Loading

workingjubilee commented Sep 2, 2024 •

edited

Loading

workingjubilee Sep 2, 2024 •

edited

Loading

workingjubilee Sep 6, 2024 •

edited

Loading