use of proc_macro[2]::TokenStream in a vector in TLS causes rustc panic

[hawkinsw]

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 of TokenStreams across multiple invocations of those proc macros using storage allocated with thread_local!. That sounds complicated, but I promise it's not:

extern crate proc_macro;
extern crate proc_macro2;

use proc_macro::TokenStream;
use quote::quote;
use std::cell::RefCell;

thread_local!(static ARMS: RefCell<Vec<proc_macro2::TokenStream>> = RefCell::new(Vec::<proc_macro2::TokenStream>::new()));

#[proc_macro_attribute]
pub fn arm(
    _ : TokenStream,
    item: TokenStream,
) -> TokenStream {
    let ts = proc_macro2::TokenStream::new();
    ARMS.with(|arms| {
        arms.borrow_mut().push(ts);
    });
    item
}

#[proc_macro]
pub fn make_arms(_: proc_macro::TokenStream) -> proc_macro::TokenStream {
  proc_macro::TokenStream::from(quote!{ pub fn arms(&self) { self.arm1(); } })
}

Here's how I could use these proc macros:

extern crate rust_macro_error;

use rust_macro_error::arm;
use rust_macro_error::make_arms;

struct T {
    _i: usize,
}

impl T {
    pub fn new() -> Self {
        T { _i: 2 }
    }
    #[arm]
    pub fn arm1(&self) {
    }
    make_arms!();
}

fn main() {
    T::new().arms();
}

Here is my Cargo.toml

[package]
name = "rust_macro_error"
version = "0.1.0"
authors = ["Will Hawkins <whh8b@obs.cr>"]
edition = "2018"

[lib]
proc-macro = true

[[bin]]
name = "rust_macro_error"
path = "src/main.rs"

[dependencies]
quote="1.0.0"
proc-macro2="1.0.1"

[dependencies.syn]
version="1.0"
features = ["full"]

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 a rustc 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:

#0  0x00007ffff32999f3 in futex_wait_cancelable (private=<optimized out>, 
    expected=0, futex_word=0x7fffec4f54a8)
    at ../sysdeps/unix/sysv/linux/futex-internal.h:88
#1  __pthread_cond_wait_common (abstime=0x0, mutex=0x7fffec4f5450, 
    cond=0x7fffec4f5480) at pthread_cond_wait.c:502
#2  __pthread_cond_wait (cond=0x7fffec4f5480, mutex=0x7fffec4f5450)
    at pthread_cond_wait.c:655
#3  0x00007ffff3db4e62 in std::sys::unix::condvar::Condvar::wait (
    mutex=<optimized out>, self=<optimized out>)
    at src/libstd/sys/unix/condvar.rs:71
#4  std::sys_common::condvar::Condvar::wait (mutex=<optimized out>, 
    self=<optimized out>) at src/libstd/sys_common/condvar.rs:41
#5  std::sync::condvar::Condvar::wait (self=<optimized out>, guard=...)
    at src/libstd/sync/condvar.rs:204
#6  0x00007ffff3d917db in std::thread::park () at src/libstd/thread/mod.rs:911
#7  0x00007ffff3d746e2 in std::sync::mpsc::blocking::WaitToken::wait (self=...)
    at src/libstd/sync/mpsc/blocking.rs:71
#8  0x00007ffff702275d in std::sync::mpsc::stream::Packet<T>::recv (
    self=0x7fffd8001080, deadline=...)
    at /home/hawkinsw/code/rust/rust/src/libstd/sync/mpsc/stream.rs:194
#9  0x00007ffff70267ac in std::sync::mpsc::Receiver<T>::recv (
    self=0x7ffff04f88f0)
    at /home/hawkinsw/code/rust/rust/src/libstd/sync/mpsc/mod.rs:1197
#10 0x00007ffff7027055 in <std::sync::mpsc::IntoIter<T> as core::iter::traits::iterator::Iterator>::next (self=0x7ffff04f88f0)
    at /home/hawkinsw/code/rust/rust/src/libstd/sync/mpsc/mod.rs:1533
#11 jobserver::imp::spawn_helper::{{closure}} ()
    at /home/hawkinsw/.cargo/registry/src/github.com-1ecc6299db9ec823/jobserver-0.1.16/src/lib.rs:635
#12 std::sys_common::backtrace::__rust_begin_short_backtrace (f=...)
    at /home/hawkinsw/code/rust/rust/src/libstd/sys_common/backtrace.rs:77
#13 0x00007ffff702a983 in std::thread::Builder::spawn_unchecked::{{closure}}::{{closure}} () at /home/hawkinsw/code/rust/rust/src/libstd/thread/mod.rs:470
#14 <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once (self=..., _args=<optimized out>)
    at /home/hawkinsw/code/rust/rust/src/libstd/panic.rs:315
#15 std::panicking::try::do_call (data=<optimized out>)
    at /home/hawkinsw/code/rust/rust/src/libstd/panicking.rs:296
#16 0x00007ffff3db6a14 in __rust_maybe_catch_panic (f=0x0, 
    data=0x7fffec4f54a8 "\002", data_ptr=0x7ffff04f8a00, 
    vtable_ptr=0x7ffff04f8a08) at src/libpanic_unwind/lib.rs:80
#17 0x00007ffff702ab84 in std::panicking::try (f=...)
    at /home/hawkinsw/code/rust/rust/src/libstd/panicking.rs:275
#18 std::panic::catch_unwind (f=...)
    at /home/hawkinsw/code/rust/rust/src/libstd/panic.rs:394
#19 std::thread::Builder::spawn_unchecked::{{closure}} ()
    at /home/hawkinsw/code/rust/rust/src/libstd/thread/mod.rs:469
#20 core::ops::function::FnOnce::call_once{{vtable-shim}} ()
    at /home/hawkinsw/code/rust/rust/src/libcore/ops/function.rs:235
#21 0x00007ffff3d75a6f in <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once (self=..., args=<optimized out>)
    at /home/hawkinsw/code/rust/rust/src/liballoc/boxed.rs:922
#22 0x00007ffff3d7e7d0 in <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once (self=0x7fffec4f55b0, args=<optimized out>)
    at /home/hawkinsw/code/rust/rust/src/liballoc/boxed.rs:922
#23 std::sys_common::thread::start_thread (main=<optimized out>)
    at src/libstd/sys_common/thread.rs:13
#24 std::sys::unix::thread::Thread::new::thread_start (main=0x7fffec4f55b0)
    at src/libstd/sys/unix/thread.rs:79
#25 0x00007ffff32936db in start_thread (arg=0x7ffff04fa700)
    at pthread_create.c:463
#26 0x00007ffff3a4b88f in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

There are a few interesting things that I have noticed while attempting to debug this:

1. If I use another type, say String, in the TLS vector, everything works fine.
2. If I don't push any values to the vector, everything works a-ok.
3. I can sometimes get the error message:
   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 Panic inside panic when procedural macro is called with proc_macro::bridge::client #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.

[jonas-schievink]

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?

[Centril]

cc @eddyb -- We get this question every now and then... I think we need to clarify somewhere that this is explicitly unsupported.

[eddyb]

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.

[Centril]

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.

@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...).

[eddyb]

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).

[Centril]

@eddyb Let's give it a try as an error (at first) and crater that?

[hawkinsw]

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 ***@***.***> 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 <#63804?email_source=notifications&email_token=ACCP2CQWQAVOC4LCXGCPFYTQF2FYRA5CNFSM4IOS6LTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45A3FA#issuecomment-523898260>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACCP2CQETZZD3AN7HWM2QXDQF2FYRANCNFSM4IOS6LTA> .

[hawkinsw]

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 ***@***.***> 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 ***@***.***> 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 > <#63804?email_source=notifications&email_token=ACCP2CQWQAVOC4LCXGCPFYTQF2FYRA5CNFSM4IOS6LTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45A3FA#issuecomment-523898260>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ACCP2CQETZZD3AN7HWM2QXDQF2FYRANCNFSM4IOS6LTA> > . >

[eddyb]

In the original issue report, you wrote:

I share a vector of TokenStreams across multiple invocations of those proc macros using storage allocated with thread_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.

[hawkinsw]

In the original issue report, you wrote:

I share a vector of TokenStreams across multiple invocations of those proc macros using storage allocated with thread_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, the TokenStream 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:

1. 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.

2. 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 TokenStreams using something that eats Drops to show this empirically) but rather the destruction of the global state underlying the implementation of TokenStreams.

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.

[eddyb]

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 sharedTokenStreams 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 Drops, I suspect it doesn't work perfectly.