Open hawkinsw opened 5 years ago
Note that procedural macros do not support retaining state between executions or sharing state between macros.
That said, fatal runtime error: failed to initiate panic, error 5
is not something I'd expect to see in that case. What machine are you running this on?
cc @eddyb -- We get this question every now and then... I think we need to clarify somewhere that this is explicitly unsupported.
use-after-free in 'proc_macro' handle
is the correct error, it's saying that you kept something for longer than you should have (if you have any suggestions, feel free to open a PR modifying libproc_macro/bridge/client.rs
).
I understand that to mean that the "server" for the proc_macro implementation is not alive, for some reason
No, that would behave much more differently. There is a server attached, it just didn't recognize that handle you held onto (we use atomic counters to guarantee we can detect this).
@Centril Something more drastic we could attempt is leak-checking (note that this only works for non-interned handles, so TokenStream
and a few others but not Ident
or Span
) when exiting a proc macro invocation (since we'd have undropped handles in the map, we can easily detect this).
The problem is that leaks could be legitimate (e.g. mem::forget
or Rc
cycles), so this might need to be a warning instead of a panic/error.
The problem is that leaks could be legitimate (e.g.
mem::forget
orRc
cycles), so this might need to be a warning instead of a panic/error.
@eddyb That does sound drastic but perhaps worth a try -- could we perhaps look for thread_local!
and friends instead and emit a warning there? It's a less sound analysis but it would have caught this instance. Other than that, I was mainly thinking about documentation changes in some relevant places (idk where proc macros are primarily documented...).
Kinda tricky to mess around with the layers of indirection around thread_local!
.
What I was suggesting was a dynamic check, which is almost trivial to perform, along the lines of !handles.TokenStream.is_empty()
(also, it could even print the values being leaked since they are still available at that point).
@eddyb Let's give it a try as an error (at first) and crater that?
Thank you for responding! Can you explain to me why I am keeping something too long? Both of these proc macro implementations are invoked from the same thread, I believe. If they weren't, then they would access different different vectors through the refcell. I'm not keeping references to the destination items across calls.
On Thu, Aug 22, 2019, 9:09 AM Mazdak Farrokhzad notifications@github.com wrote:
@eddyb https://github.com/eddyb Let's give it a try as an error (at first) and crater that?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/63804?email_source=notifications&email_token=ACCP2CQWQAVOC4LCXGCPFYTQF2FYRA5CNFSM4IOS6LTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45A3FA#issuecomment-523898260, or mute the thread https://github.com/notifications/unsubscribe-auth/ACCP2CQETZZD3AN7HWM2QXDQF2FYRANCNFSM4IOS6LTA .
The only thing I could imagine that is being retained incorrectly is something internal to the implementation of a tokenstream that is specific to the proc macro implementation in which it is constructed. Otherwise, my use case seems entirely legitimate.
In other words, it is not that sharing generally is unsupported in this case it is, specifically, the sharing of tokenstream across invocations. If I retained state across implementations with a refcell of a vector of "thing" where the implementation of "thing" does not make use of any global state, then all should be well and good.
Please correct me where I am wrong. Thanks again!
On Thu, Aug 22, 2019, 10:08 AM Will Hawkins hawkinsw@obs.cr wrote:
Thank you for responding! Can you explain to me why I am keeping something too long? Both of these proc macro implementations are invoked from the same thread, I believe. If they weren't, then they would access different different vectors through the refcell. I'm not keeping references to the destination items across calls.
On Thu, Aug 22, 2019, 9:09 AM Mazdak Farrokhzad notifications@github.com wrote:
@eddyb https://github.com/eddyb Let's give it a try as an error (at first) and crater that?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/63804?email_source=notifications&email_token=ACCP2CQWQAVOC4LCXGCPFYTQF2FYRA5CNFSM4IOS6LTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45A3FA#issuecomment-523898260, or mute the thread https://github.com/notifications/unsubscribe-auth/ACCP2CQETZZD3AN7HWM2QXDQF2FYRANCNFSM4IOS6LTA .
In the original issue report, you wrote:
I share a vector of
TokenStream
s across multiple invocations of those proc macros using storage allocated withthread_local!
Which is definitely unsupported (generally you shouldn't assume you can communicate between invocations in any way, we don't really guarantee the order your functions will be called in).
What might be happening (and which I overlooked above, my bad) is whenever the thread_local!
contents are dropped, just before the thread exits, the TokenStream
attempts to notify the server to drop the data associated with the client-side handle.
If you want to keep it in a thread-local for the duration of one invocation, you could look at scoped_thread_local!
, which would avoid anything being left around between invocation, or after the last invocation.
In the original issue report, you wrote:
I share a vector of
TokenStream
s across multiple invocations of those proc macros using storage allocated withthread_local!
Which is definitely unsupported (generally you shouldn't assume you can communicate between invocations in any way, we don't really guarantee the order your functions will be called in).
I completely understand that this is completely unsupported. That said, if possible, I'd like to continue this discussion based on the current implementation and assume that I am willing to accept the attendant risks should the implementation change. I know that you are busy, so please feel free to say that you have better things to do than discuss this, obviously stupid choice ...
What might be happening (and which I overlooked above, my bad) is whenever the
thread_local!
contents are dropped, just before the thread exits, theTokenStream
attempts to notify the server to drop the data associated with the client-side handle.
If I understand correctly, there is some global state shared among all instantiated TokenStream
objects. Therefore, I am seeing two separate, but related, issues:
Use of a TokenStream
instantiation t
beyond the function in which it was created could (and, in this case, does) result in a use-after-free because the global state referred to by t
could have been changed during subsequent TokenStream
instantiations (or even other proc_macro
functions). This is the reason that I occasionally see the use-after-free error with the code above.
The fatal runtime error: failed to initiate panic, error 5
is caused by attempting to print to stderr in a panic in thread exit, a place where io cannot happen. The panic comes starts at 315 in libproc_macro/bridge/client.rs
because of exactly what you said above (ie,"the TokenStream
attempts to notify the server to drop the data associated with the client-side handle." but, at that point, the server is no longer alive). This does not happen because of a Drop
of the TokenStream
itself (I have implemented the container of the shared TokenStream
s using something that eats Drop
s to show this empirically) but rather the destruction of the global state underlying the implementation of TokenStream
s.
Does this accurately summarize the issue? If it does, then I am completely satisfied with the state of this very productive discussion and feel like I completely understand. Obviously I would be very happy knowing that I understand and will offer to make suggestions (through PRs) to changes to the documentation to make it more obvious for others why what I am attempting to do is "not good".
Going back to the hypothetical discussion (where I am okay relying on implementation-specific behavior that is completely unsupported and may change at any time), this gives me a path forward.
Again, thank you for the discussion. It's been very helpful.
If you want to keep it in a thread-local for the duration of one invocation, you could look at
scoped_thread_local!
, which would avoid anything being left around between invocation, or after the last invocation.
could (and, in this case, does) result in a use-after-free
I think that message is a bit too confusing (but I didn't know what to call it). It's not a memory-safety "use-after-free", it's a "conceptual" one.
I should explain a bit more about how the system works.
Whenever you get a proc_macro
"object", such as a TokenStream
, either in the invocation itself, or from the proc_macro
public API, what is being passed from the server (rustc) to the client (your proc macro) is just an integer.
So if you had two uses of #[arm]
, what happens is:
// server connects and is told counter starts at 1
server calls arm(TokenStream(1), TokenStream(2))
TokenStream::new() returns TokenStream(3)
TokenStream(3) ends up in TLS (leaking it)
TokenStream(2) is returned
// server still knows about TokenStream(3)
// server disconnects (taking TokenStream(3)'s actual data with it)
// server connects and is told counter starts at 4
server calls arm(TokenStream(4), TokenStream(5))
TokenStream::new() returns TokenStream(6)
TokenStream(6) ends up in TLS (leaking it)
TokenStream(5) is returned
// server still knows about TokenStream(6)
// server disconnects (taking TokenStream(6)'s actual data with it)
So at the end you have TokenStream(3)
and TokenStream(6)
in TLS, and whenever that is being dropped, the current server is told to destroy its 3
and 6
token streams (but I guess since no server is active, it tries to panic, and panicking is sadly currently broken in a proc macro, outside of an invocation).
If you tried to access the saved TokenStream(3)
during the second invocation, it would panic with the "use-after-free" message - all this means is that its map from "handle" integers to the server-specific token stream implementation doesn't contain 3
.
You are guaranteed this panic! TokenStream(3)
is completely unknown to that second server instance.
I am okay relying on implementation-specific behavior that is completely unsupported and may change at any time
No implementation-specific behavior here. Keeping proc_macro
objects between/after invocations is not merely unsupported, it's actively and fully banned. You can't even use unsafe code to do it!
This is the reason that I occasionally see the use-after-free error with the code above.
I assume the reason for "occasionally" here has to do with whether your code accesses the handles stored in TLS from other invocations, or there is something non-deterministic about TLS drop order.
This does not happen because of a
Drop of the
TokenStreamitself (I have implemented the container of the shared
TokenStreams using something that eats
Drops to show this empirically) but rather the destruction of the global state underlying the implementation of
TokenStream`s.
All the associated data is server-side and it's gone by the time an invocation is finished, before the thread exits - all of that is harmless (e.g. if you were leaking handles with mem::forget
- nothing would happen atm).
The only way a TokenStream
in TLS could cause issues is its Drop
running, which effectively calls current_server.token_stream_drop(self)
.
I would like to see that code that eats Drop
s, I suspect it doesn't work perfectly.
Wait, I'm glad I'm wrong about the TLS panics, they're just... always broken:
struct Bomb;
impl Drop for Bomb {
fn drop(&mut self) {
panic!("bomb dropped");
}
}
thread_local!(static BOMB: Bomb = Bomb);
fn main() {
BOMB.with(|_| {}); // arm the bomb
}
try on playpen - results in this output:
thread 'main' panicked at 'bomb dropped', src/main.rs:4:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
fatal runtime error: failed to initiate panic, error 5
timeout: the monitored command dumped core
cc @RalfJung @alexcrichton @gnzlbg Has this been reported before? Since C calls into the TLS destructor hooks, I don't think we can unwind out into C, so TLS destructor panics should abort?
Another clue: if you see fatal runtime error: failed to initiate panic, error 5
but there's no panic message above it, that probably means that the panic silencing logic is kicking in (which generally shouldn't happen outside of an active server, which has its own way of getting the panic message and turning it into a rustc error).
As you can see above, panicking should always print a message, before trying to unwind ("failed to initiate panic" should probably say "failed to (start) unwind(ing)" instead). It's just that for proc macros we have a hack to silence the panic output when the unwinding would've been caught by the server anyway.
could (and, in this case, does) result in a use-after-free
I think that message is a bit too confusing (but I didn't know what to call it). It's not a memory-safety "use-after-free", it's a "conceptual" one.
I should explain a bit more about how the system works.
After reading your explanation, I am convinced that we are on the same page. I was just using different words since I didn't understand the behind-the-scenes details. Thank you for taking the time to explain. I get it so much more. Please see below for an additional question/clarification! Thanks again for this description.
Whenever you get a
proc_macro
"object", such as aTokenStream
, either in the invocation itself, or from theproc_macro
public API, what is being passed from the server (rustc) to the client (your proc macro) is just an integer.So if you had two uses of
#[arm]
, what happens is:// server connects and is told counter starts at 1 server calls arm(TokenStream(1), TokenStream(2)) TokenStream::new() returns TokenStream(3) TokenStream(3) ends up in TLS (leaking it) TokenStream(2) is returned // server still knows about TokenStream(3) // server disconnects (taking TokenStream(3)'s actual data with it)
// server connects and is told counter starts at 4 server calls arm(TokenStream(4), TokenStream(5)) TokenStream::new() returns TokenStream(6) TokenStream(6) ends up in TLS (leaking it) TokenStream(5) is returned // server still knows about TokenStream(6) // server disconnects (taking TokenStream(6)'s actual data with it)
So at the end you have
TokenStream(3)
andTokenStream(6)
in TLS, and whenever that is being dropped, the current server is told to destroy its3
and6
token streams (but I guess since no server is active, it tries to panic, and panicking is sadly currently broken in a proc macro, outside of an invocation).If you tried to access the saved
TokenStream(3)
during the second invocation, it would panic with the "use-after-free" message - all this means is that its map from "handle" integers to the server-specific token stream implementation doesn't contain3
. You are guaranteed this panic!TokenStream(3)
is completely unknown to that second server instance.I am okay relying on implementation-specific behavior that is completely unsupported and may change at any time
No implementation-specific behavior here. Keeping
proc_macro
objects between/after invocations is not merely unsupported, it's actively and fully banned. You can't even use unsafe code to do it!
I think that we are saying exactly the same thing but with different words because I didn't know exactly the implementation of proc_macro.
Let me play this back and see if I have it correct:
For every invocation of a function f that implements a proc macro, there is a server connection that may be used by the implementation in proc_macro features (I am assuming this is to facilitate a single point of contact with a thread that knows how to parse/tokenize/etc). One such feature would be TokenStream
, obviously. So, the TokenStream
s created during the execution of f are just integer handles (like an fd in *nix) and refer to resources in the server that are specific to that connection with the server. That connection with the server is alive only for the duration of that invocation of f. Just like
int fd = open(...);
close(fd);
write(fd, ...);
will not work, using a proc_macro resource with handle h created during the lifetime of connection c outside c's lifetime won't work either.
String
invocations are not linked in any way to data or connections that live only as long as the invocation of a function implementing a proc macro. Therefore, I should be able to use those in place of a TokenStream
s in the TLS vector and have no problem. In other words,
thread_local!(static ARMS: RefCell<Vec<String>> = RefCell::new(Vec::<String>::new()));
will work fine. This is why I was seeing these errors only when the RefCell in TLS owned a vector of TokenStream
s.
Please correct me where I am wrong, but I feel pretty confident that I get it. That's only because you spent the time explaining -- thank you!
This is the reason that I occasionally see the use-after-free error with the code above.
I assume the reason for "occasionally" here has to do with whether your code accesses the handles stored in TLS from other invocations, or there is something non-deterministic about TLS drop order.
This does not happen because of a
Drop of the
TokenStreamitself (I have implemented the container of the shared
TokenStreams using something that eats
Drops to show this empirically) but rather the destruction of the global state underlying the implementation of
TokenStream`s.All the associated data is server-side and it's gone by the time an invocation is finished, before the thread exits - all of that is harmless (e.g. if you were leaking handles with
mem::forget
- nothing would happen atm).The only way a
TokenStream
in TLS could cause issues is itsDrop
running, which effectively callscurrent_server.token_stream_drop(self)
. I would like to see that code that eatsDrop
s, I suspect it doesn't work perfectly.
I should've just compared proc_macro handles to file descriptors, your write-after-close example is spot on!
Another way to see the TLS situation is something like running the same program multiple times, and every time it appends a fd into a file. You could then look at all of those fd's but they are meaningless, because those processes no longer exist.
You can use strings in TLS, yes, but keep in mind we might not run all of your invocations, or always in the same order, so it's not usable for what you want, I'm afraid. (In fact, we planned to use one thread per invocation but that was too slow - not that it would stop other side-channels like global variables or even files)
@Centril hmm we should also do a crater run in which we error if there are non-frozen statics in a proc-macro crate (would catch thread_local! too).
You should instead have another attribute on the enclosing function (for example) in which you use #[arm]
, that gathers all the information at once.
cc @petrochenkov @dtolnay (who might be able to help you come up with a working design)
I should've just compared proc_macro handles to file descriptors, your write-after-close example is spot on!
I'm glad that I grok!
Another way to see the TLS situation is something like running the same program multiple times, and every time it appends a fd into a file. You could then look at all of those fd's but they are meaningless, because those processes no longer exist.
Yes, this is a great example, too! In fact, I think this is a better example!
You can use strings in TLS, yes, but keep in mind we might not run all of your invocations, or always in the same order, so it's not usable for what you want, I'm afraid. (In fact, we planned to use one thread per invocation but that was too slow - not that it would stop other side-channels like global variables or even files)
Completely understand. See below for additional context on this design/implementation.
@Centril hmm we should also do a crater run in which we error if there are non-frozen statics in a proc-macro crate (would catch thread_local! too).
You should instead have another attribute on the enclosing function (for example) in which you use
#[arm]
, that gathers all the information at once.cc @petrochenkov @dtolnay (who might be able to help you come up with a working design)
Yes, this is exactly the design that I initially considered. I'll tell you why I didn't go for it immediately: I wanted a POC for whether or not this type of crate API is useful in my own project. The correct design would require more in-depth implementation and I just wanted to get something work in the short term. I will go back and rewrite now that I've proven that the API is something that works!
Thank you so much for digging in to this issue with me. I really appreciate your patience and your willingness to read and comprehend each my responses before sending your responses (that's really hard to do in a communication medium like this).
I understand it so much better. Is there something that I can do to help make the documentation around this better? If so, please let me know. I am a compiler geek and eager to contribute to rustc if/where I can.
Thanks again!
Has this been reported before? Since C calls into the TLS destructor hooks, I don't think we can unwind out into C, so TLS destructor panics should abort?
I have not heard about this issue before, but what you are saying makes sense. TLS destructor hooks are basically "extern" fn. We probably need a panic-aborting wrapper around them.
Can you report an issue specifically about that?
@eddyb
Has this been reported before?
Looks like an instance of: https://github.com/rust-lang/rust/issues/24479
We have an A-thread-locals
label. I'm not sure if this issue belongs there, but we probably should triage those at some point.
I am relatively new to rust, but I am a huge, huge fan. I hate to report an issue and hope that it's something worthwhile.
I am attempting to use
proc_macro[2]::TokenStream
in the implementation of a proc macro. I share a vector ofTokenStream
s across multiple invocations of those proc macros using storage allocated withthread_local!
. That sounds complicated, but I promise it's not:Here's how I could use these proc macros:
Here is my Cargo.toml
If it's easier, you can grab these files from this git repository: http://git.obs.cr/hawkinsw/rust_macro_error
When I run
cargo build
, I get arustc
failure. I have attempted to compile this with stable, nightly and "head" (stage2 compiler of HEAD). I get the same error no matter which compiler:fatal runtime error: failed to initiate panic, error 5
I can run the compiler under
gdb
but see nothing worthwhile:There are a few interesting things that I have noticed while attempting to debug this:
String
, in the TLS vector, everything works fine.use-after-free in 'proc_macro' handle
and can narrow that down to line 315 in libproc_macro/bridge/client.rs. I understand that to mean that the "server" for the proc_macro implementation is not alive, for some reason. I have seen https://github.com/rust-lang/rust/issues/60593 and think that they are related but cannot seem to find the common thread.I have debugged as much as I can and I am reaching out for your help! I would love to help solve the problem if you can point me in the right direction.
Again, thank you for all the work that you are doing for rust and the community around the language.