tcdi / postgrestd

The most peculiar std you have ever seen
Other
37 stars 4 forks source link

Neturalize `include_{str,bytes}!`'s ability to access the filesystem. #23

Closed thomcc closed 1 year ago

thomcc commented 1 year ago

Implements the idea in https://github.com/tcdi/plrust/issues/172#issuecomment-1407071033. I wasn't going to do this, as I have no easy way to test this locally RN, so I'm hoping CI does it for me (does... postgrestd have CI?)

thomcc commented 1 year ago

Local testing result: It compiles at least, which means I didn't miss a place where we need #![feature(include_macros)]. I'll see how easily I can setup a test with plrust.

thomcc commented 1 year ago

Ugh, this works too well. Trying to use it in plrust causes the following failure:

   error[E0658]: use of unstable library feature 'include_macros': no file access in postgrestd
      --> /home/thom/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-1.0.107/src/token.rs:882:1
       |
   882 | include!("await.rs"); // export_token_macro! {[await]}
       | ^^^^^^^

We could only forbid include_str! and include_bytes! (and not include!), but that's not quite as good of a defense. Still, better than we have now.

thomcc commented 1 year ago

It seems this fails if we just forbid include_str! too:

   error[E0658]: use of unstable library feature 'include_macros': no file access in postgrestd
    --> /home/thom/.cargo/registry/src/github.com-1ecc6299db9ec823/bitvec-1.0.1/src/slice/traits.rs:1:10
     |
   1 | #![doc = include_str!("../../doc/slice/traits.md")]
     |          ^^^^^^^^^^^

I've wanted to remove our bitvec dependency for some time, but it's a larger task.

thomcc commented 1 year ago

This last attempt worked. It's a completely different approach, and allows these to compile (because our deps need them) but neutralizes their ability to access the filesystem.

thomcc commented 1 year ago

This feels like a bit of a thin victory due to include! not working with this

Yeah our dependencies use it too much to do anything about that. We even use it (well, prior to the PR I just filed), although we don't need to (our dependencies don't need to either -- you pretty much never need it over #[path] anymore, although you used to).

That said, I wasn't able to use it to actually get file contents as a string in a meaningful way (plausibly combination of include! and stringify! could do it, but I couldn't make it work), and if it ends up that someone finds a way it seems likely possible for us to neutralize stringify in a similar way without breaking much (this would be quite unfortunate as it would degrade assert messages considerably[^1], so I'd rather wait for a PoC before considering it).

[^1]: As I mentioned in discord, that downside could be avoided by keeping the #[rustc_builtin_macro] version of stringify! as a private thing that assert and friends use, while exporting a neutralized version.