plrust icon indicating copy to clipboard operation
plrust copied to clipboard

Limiting and eliminating `unsafe` in user functions

Open workingjubilee opened this issue 3 years ago • 3 comments

Some code that can be written for trusted language handlers might still soundly call unsafe code, technically speaking (in Rust terms), but PL/Rust should probably provide a suite of blessed functions that do the typical things we expect safely. There are many ways to lock down Safe Rust, but we should probably develop a plan for allowing easily auditing and eliminating unsafe code, including in dependencies (see https://github.com/tcdi/plrust/issues/26), and thereby prevent PL/Rust code from using unsafe to escape any boxes we put it in.

For dependencies, I believe this may be easy to do with cargo build --build-plan or --unit-graph or a similar functionality and simply using rustc --forbid unsafe_code.

Slightly more complicated is the code handed directly to PL/Rust. We can't simply ban unsafe code in that because using pgx expands to include unsafe code currently! We instead probably need to audit the code for unsafe while it is flowing through the slightly misleadingly named pgx-utils, which is PGX's SQL generation proc macro. We could hypothetically make sure everything we expand is safe code but that might be misleading and/or unsound so I am not inclined to presume that is on the table. Instead, we should find some approach to auditing the way pgx expands.

  • [ ] Checking unsafe deps
  • [ ] Checking unsafe in user fn itself
  • [x] Auditing the PGX expansions we expect
  • [ ] Guarding against proc macro cleverness in general

workingjubilee avatar Jul 27 '22 22:07 workingjubilee

One of the inescapable problems in PL/Rust is that even if all the dependencies are very carefully audited and then the function is fully sandboxed by whatever means, the bindings to PostgreSQL themselves must be sound in at least the memory-safe sense and also ideally the logical sense.

As if to answer the call of this issue, several soundness issues materialized in PGX:

  • https://github.com/tcdi/pgx/issues/627
  • https://github.com/tcdi/pgx/issues/633
  • https://github.com/tcdi/pgx/issues/648

I have almost finished redesigning the current "ArrayType handle" that PGX uses to fix some of these issues, starting with a subcomponent that offers much better checks on its soundness properties and more meticulously documented safety conditions. (It will also, amusingly, probably be much faster, not that such is a primary goal.) I hope to return to more... "surface" level concerns like this one very soon.

workingjubilee avatar Aug 26 '22 11:08 workingjubilee

https://github.com/tcdi/plrust/pull/95 brings up proc macros as a special concern. Yes, they are. It also will take care of both most concerns regarding PGX and some initial concerns regarding unsafe in user fn, by guaranteeing that any new pgx versions will not insert code with unqualified unsafe. It will not address dependencies or proc macros.

workingjubilee avatar Sep 27 '22 04:09 workingjubilee

We also need to forbid:

  • Unsafe attributes like no_mangle, link, link_section, etc (need to look to see if this is a complete list of stable ones). These are unsafe, but do not require the unsafe keyword (although this may change).
  • Any extern blocks, as they allow confusing LLVM about the actual signature of functions, and grant access to functions we would like users to not even have the address of. They may become unsafe extern in a future Rust edition, but I've only seen this suggested and not proposed.

thomcc avatar Oct 06 '22 17:10 thomcc

As of https://github.com/tcdi/plrust/pull/128 we cover a lot of these cases for the main function crate using a significantly modified compilation flow that allows a lot of other shell games that should make auditing functions easier. I'm going to claim victory on this for now and move the remaining concerns to other issues, namely

  • #132
  • #137
  • #138

workingjubilee avatar Dec 11 '22 00:12 workingjubilee