Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not safe to share Spans from one ParseSess to another #591

Merged
merged 1 commit into from
Oct 19, 2016
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Oct 18, 2016

Fixes #590 and probably fixes #574.

Spans in the AST returned by parse_item_from_source_str and other parsing
functions contain byte offsets into the source code they were parsed from. The
pretty printer uses these Spans here to preserve the representation of
literals when parsing and printing back out unmodified.

In this bug, the byte offset of a string in the input to
parse_item_from_source_str coincidentally matched the byte offset of a totally
different string in the input to parse_crate_from_file called here by
Syntex. The Span from the former triggered the pretty printer to write out the
content of the latter.

By using the same ParseSess, Spans from the two parse_* calls never collide.

Spans in the AST returned by `parse_item_from_source_str` and other parsing
functions contain byte offsets into the source code they were parsed from. The
pretty printer uses these Spans [here][1] to preserve the representation of
literals when parsing and printing back out unmodified.

In this bug, the byte offset of a string in the input to
`parse_item_from_source_str` coincidentally matched the byte offset of a totally
different string in the input to `parse_crate_from_file` called [here][2] by
Syntex. The Span from the former triggered the pretty printer to write out the
content of the latter.

By using the same ParseSess, Spans from the two `parse_*` calls never collide.

[1]: https://github.com/rust-lang/rust/blob/1.12.0/src/libsyntax/print/pprust.rs#L628
[2]: https://github.com/serde-rs/syntex/blob/v0.45.0/syntex/src/registry.rs#L134
@dtolnay
Copy link
Member Author

dtolnay commented Oct 18, 2016

@erickt @oli-obk I found two other suspicious ParseSess::new() calls here and here. Let me know if you agree those need to be fixed. I also found this but I think it is okay because the parsed item immediately gets respanned.

@oli-obk
Copy link
Member

oli-obk commented Oct 18, 2016

Isn't the issue only if the newly created ParseSess is used to parse from a str? So only the second one is bad. But I might be wrong.

@dtolnay
Copy link
Member Author

dtolnay commented Oct 18, 2016

The first one passes the bad ParseSess to to_tokens here. The to_tokens can use the ParseSess to parse strings, for example here.

@erickt
Copy link
Member

erickt commented Oct 18, 2016

@dtolnay: r+ on this change. Do you think it'd just be best for us to replace all spans with dummy spans in these cases? Or if we should thread ExtCtxts through these cases to use the same ParseSess objects?

@dtolnay
Copy link
Member Author

dtolnay commented Oct 19, 2016

@erickt as far as I understand the issue, in theory respanning to DUMMY_SP is sufficient to fix any problems of not using the right ParseSess.

In practice fold.rs has a ton of holes around not handling spans correctly. I have hit 9 so far with syn (I have a unit test that parses a file with syn, prints it back out, parses both the output and the original using syntex, respans them to dummy spans, and asserts they are the same). I still need to push those fixes upstream.

@dtolnay
Copy link
Member Author

dtolnay commented Oct 19, 2016

I released serde_codegen 0.8.14 with this fix and I filed serde-deprecated/aster#116 and serde-deprecated/syntex#100 to follow up on the other occurrences.

@norru
Copy link

norru commented Oct 28, 2016

Fixes #574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants