Add an option to annotate paths for inlined modules #17

sunshowers · 2020-07-15T23:35:47Z

Depends on #16.

Add an option to InlinerBuilder, defaulting to false, which causes
syn-inline-mod to annotate inlined modules with the paths they come from.

This is useful when mapping spans back to the files they come from.

(There's a fair bit of added complexity here. I'd totally understand if you
don't want to accept this, but this is essential to the work I'm doing so I'd
unfortunately have to fork the crate in that case.)

Closes #15.

TedDriggs · 2020-07-16T00:45:18Z

Getting caught up on all my GH notifications, but wanted to acknowledge seeing the PR. I'll get a review up tomorrow morning.

sunshowers · 2020-07-16T00:58:52Z

thank you! :)

TedDriggs

This is looking really good; I'm now really curious to know what project you're working on that needs it.

There is one more change that I think is warranted but I couldn't leave a comment on because it's outside the edited ranges: You deserve to be listed as an author of the crate :) If you'd like to be included just put yourself in on the next commit.

tests/resolver.rs

src/lib.rs

TedDriggs · 2020-07-17T13:37:23Z

src/lib.rs

+    let mut path = None;
+    let position = attrs.iter().position(|attr| {
+        if attr.path.is_ident(SYN_INLINE_MOD_PATH) {
+            // Ignore the attribute if it isn't of the correct form.


Is your worry here that some other proc-macro might happen to squat on our attribute name? Finding an attribute of the right name and wrong format feels to me like something must have gone horribly wrong.

Yeah or the user puts the annotation in themselves or something. The options are to panic or ignore -- what do you think about them? I guess panicking is probably better/safer since it'd be totally unexpected to hit this.

Well I realized this may end up dealing with external data so we'd probably rather not panic. I think this should be a pretty rare case though, as you pointed out something's really gone wrong (but we don't want to create an opportunity for an accidental DoS).

DoS seems bad, but if you're dealing with untrusted input couldn't someone put that attribute into the original source code to try and make you follow a path into a directory you shouldn't?

~~I'm wondering if embedding these in-band is fundamentally unsafe.~~

I think this can be done safely, but we should explicitly disallow inlining if the input contains the magic attribute in the inner or outer attributes. That way we avoid attacks that attempt to feed us incorrect paths; those sorts of attack could be used to try and read arbitrary files from wherever this code was being run.

If we need to support the same code being inlined multiple times, then I see two options:

We only fail inlining if the attribute is present and we've been asked to add path annotations. One upside of this approach is that we don't do work scanning attributes unless we're anticipating using the inserted annotations, while a downside of this approach is that someone could still sneak a bad path through because there's no connection between find_mod_path and InliningResult

Rather than being a constant, the attribute path is a property of InlininerBuilder and later of InliningResult. That seems pretty ugly, however, especially since that would mean find_mod_path should take (r: &InliningResult, attributes: &[Attribute]) to avoid callers from hardcoding what they thought the attribute name was in their own code.

src/lib.rs

sunshowers · 2020-07-18T02:15:37Z

I'm now really curious to know what project you're working on that needs it.

A bit of a long-winded explanation, sorry!

I'm writing tooling to make it really easy to write coverage-guided fuzz targets, integrated with property-based tests.
To make it work I need to have a way to collect all functions annotated with the #[propfuzz] macro in a crate.
My initial idea was to use the inventory crate, but there are issues with it that make it currently unsuitable.
@dtolnay and I discussed this and we came up with the alternate plan to scan all the lib.rs files, searching through them for names of fuzz targets.
We already have tooling that can give us all the lib.rs files in a workspace, so the rest we need is a way to traverse those files.
syn-inline-mod is perfect for this.
However we might encounter errors along the way (whether standard Rust issues or domain-specific ones), and we need to report them in a style that users understand.
For that I'm writing a small crate that can take a syn::File, scan through it, collect errors along the way, then report them in a nice manner with the little squigglies underneath and all just like rustc (using codespan-reporting).
This means that given a particular item, we need to figure out the filename its span comes from.
That's what led me to this.

Add an option to `InlinerBuilder`, defaulting to `false`, which causes `syn-inline-mod` to annotate inlined modules with the paths they come from. This is useful when mapping spans back to the files they come from.

TedDriggs

Thanks for making those changes. This is looking even better, but your comment about untrusted input made me look at this through more paranoid eyes.

TedDriggs · 2020-07-21T12:59:21Z

src/lib.rs

+    let mut path = None;
+    let position = attrs.iter().position(|attr| {
+        if attr.path.is_ident(SYN_INLINE_MOD_PATH) {
+            // Ignore the attribute if it isn't of the correct form.


DoS seems bad, but if you're dealing with untrusted input couldn't someone put that attribute into the original source code to try and make you follow a path into a directory you shouldn't?

~~I'm wondering if embedding these in-band is fundamentally unsafe.~~

I think this can be done safely, but we should explicitly disallow inlining if the input contains the magic attribute in the inner or outer attributes. That way we avoid attacks that attempt to feed us incorrect paths; those sorts of attack could be used to try and read arbitrary files from wherever this code was being run.

If we need to support the same code being inlined multiple times, then I see two options:

We only fail inlining if the attribute is present and we've been asked to add path annotations. One upside of this approach is that we don't do work scanning attributes unless we're anticipating using the inserted annotations, while a downside of this approach is that someone could still sneak a bad path through because there's no connection between find_mod_path and InliningResult

Rather than being a constant, the attribute path is a property of InlininerBuilder and later of InliningResult. That seems pretty ugly, however, especially since that would mean find_mod_path should take (r: &InliningResult, attributes: &[Attribute]) to avoid callers from hardcoding what they thought the attribute name was in their own code.

TedDriggs · 2020-07-21T13:05:46Z

src/visitor.rs


            match visitor.visit() {
                Ok(syn::File { attrs, items, .. }) => {
+                    if self.annotate_paths {


To the comment above, I wonder if we should check here that no attribute in the input is using our path, returning an error if it is?

TedDriggs · 2020-07-21T13:07:32Z

src/lib.rs

+    ///
+    /// If this is `true`, `find_mod_path` may be used to figure out the path a module is defined
+    /// in.
+    pub fn paths_annotated(&self) -> bool {


How would you feel about this being called has_path_annotations? In my opinion that makes it clearer this is describing the current state of something.

TedDriggs · 2020-07-21T13:10:15Z

Thanks for the great explanation of what you're up to - that's helpful for me when thinking about the long-term evolution of this crate.

TedDriggs · 2020-07-21T13:24:43Z

src/lib.rs

+
+/// A module path in a list of attributes, found by `find_mod_path`.
+#[derive(Clone, Debug)]
+pub struct InlineModPath<'ast> {


Should this be called InlineModAttributes and have path: Option<PathBuf> as a field? Then the find_mod_path function would be changed to split_mod_attributes(attrs: &[Attribute]) -> InlineModAttributes {}.

sunshowers · 2020-07-26T17:27:37Z

Just wanted to say that this is in my queue of things to do, but I have personal life stuff going on at the moment which make it unlikely I can revisit this before mid-August. Thanks!

TedDriggs · 2020-07-27T19:10:27Z

@sunshowers sorry to hear that. If I have time I'll see if I can move it forward - would you like me to block merging on your review of any subsequent changes I make?

sunshowers · 2020-07-28T00:02:40Z

Thanks -- it would be lovely if you could give me a day or so to respond, but please don't block on me if I don't get to it by then!

sunshowers force-pushed the annotate branch from 92b4c84 to cf428e9 Compare July 16, 2020 01:20

TedDriggs requested changes Jul 17, 2020

View reviewed changes

sunshowers added 2 commits July 17, 2020 20:10

Add an option to annotate paths for inlined modules

348b3f6

Add an option to `InlinerBuilder`, defaulting to `false`, which causes `syn-inline-mod` to annotate inlined modules with the paths they come from. This is useful when mapping spans back to the files they come from.

Add myself to authors list

1d2d663

sunshowers force-pushed the annotate branch from cf428e9 to 1d2d663 Compare July 18, 2020 03:11

sunshowers mentioned this pull request Jul 18, 2020

Add discovery of fuzz targets facebookarchive/propfuzz#22

Open

TedDriggs requested changes Jul 21, 2020

View reviewed changes

TedDriggs reviewed Jul 21, 2020

View reviewed changes

Add an option to annotate paths for inlined modules #17

Are you sure you want to change the base?

Add an option to annotate paths for inlined modules #17

Uh oh!

Conversation

sunshowers commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TedDriggs commented Jul 16, 2020

Uh oh!

sunshowers commented Jul 16, 2020

Uh oh!

TedDriggs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TedDriggs Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

sunshowers Jul 18, 2020

Choose a reason for hiding this comment

Uh oh!

sunshowers Jul 18, 2020

Choose a reason for hiding this comment

Uh oh!

TedDriggs Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sunshowers commented Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TedDriggs left a comment

Choose a reason for hiding this comment

Uh oh!

TedDriggs Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TedDriggs Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

TedDriggs Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

TedDriggs commented Jul 21, 2020

Uh oh!

TedDriggs Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

sunshowers commented Jul 26, 2020

Uh oh!

TedDriggs commented Jul 27, 2020

Uh oh!

sunshowers commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sunshowers commented Jul 15, 2020 •

edited

Loading

TedDriggs Jul 21, 2020 •

edited

Loading

sunshowers commented Jul 18, 2020 •

edited

Loading

TedDriggs Jul 21, 2020 •

edited

Loading