Stream: t-compiler/wg-rls-2.0

Topic: heap corruption/double free/invalid pointer free bug


Leo Le Bouter (Jan 11 2020 at 02:22, on Zulip):

I know that probably wont be of much help but I just got such an abort message with ra:

free(): invalid next size (fast)

It happened after I removed a dependency from my Cargo.toml

Emil Lauridsen (Jan 11 2020 at 09:04, on Zulip):

We do have some unsafe trickery with regards to how we store syntax trees in Rowan using ThinDST, my initial concern would be that. I'll a attempt to run with a sanitizer and reproduce later. Thanks for the tip

Emil Lauridsen (Jan 11 2020 at 11:40, on Zulip):

I've been unable to reproduce so far, but if you can boil it down to a reproducible test case I'll gladly investigate further.

Leo Le Bouter (Jan 11 2020 at 15:01, on Zulip):

I've been unable to reproduce as well. It seems that the scenario requires a very specific chain of actions for this to trigger.

Do you plan on getting rid of unsafe here? I think this could be very much exploitable.

matklad (Jan 11 2020 at 15:17, on Zulip):

There's only a single place where rust-analyzer fundamentally relies on unsafe -- in the syntax tree implementation.

It is very fancy data structure which requires unsafe to be efficient, and it indeed has quite a few of non-trivail unsafe block.

I believe that the (safe) public interface is sound and implementable, but:

matklad (Jan 11 2020 at 15:23, on Zulip):

In general, rust-analyzer assume non-hostile environment, security-wise. That is, UB is UB and must be fixed, but we generally don't try extremely hard to guarantee the absence of UB (or other security issues).

Leo Le Bouter (Jan 24 2020 at 13:54, on Zulip):

@matklad well third party crates are effectively data from random people on the Internet. Some times I review code and like to have IDE tools to do it, I don't want this code to compromise my machine.

Emil Lauridsen (Jan 24 2020 at 15:01, on Zulip):

@Leo Le Bouter I can assure you it won't compromise your machine any more than a cargo check will.
If your threat model involves RCE from reviewing untrusted crates you should probably be running your entire setup isolated from anything important, preferably on an airgapped machine, but at least in a VM

std::Veetaha (Jan 24 2020 at 15:05, on Zulip):

@Emil Lauridsen , can you please elaborate on how and which sanitizer you run? If such a problem exists in rust-analyzer I guess we would need to integrate sanitizing more closely (and add some comments on that to our dev docs)

Emil Lauridsen (Jan 24 2020 at 15:08, on Zulip):

(Note: this is not me saying the issue isn't important, just that it probably isn't security critical, and not really actionable until somebody has a stacktrace or reproducible example)

Emil Lauridsen (Jan 24 2020 at 15:09, on Zulip):

@std::Veetaha I tried a run with valgrind but no luck replicating so far

std::Veetaha (Jan 24 2020 at 15:11, on Zulip):

Okay, I think I'll create an issue for sanitizing memory-safety issues in rust-analyzer. I know this is not the highest priority right now, but I guess I'll try to tackle that myself once I have time for that.

std::Veetaha (Jan 24 2020 at 15:13, on Zulip):

By the way, speaking about soundness... rowan red-green tree seems to have circular references under the good. I didn't get so much into the details of its unsafe code, but my first concern would arise about &mut references aliasing (though it shouldn't cause ivalid free() in theory)

std::Veetaha (Jan 25 2020 at 11:41, on Zulip):

FYI: @Leo Le Bouter I created an issue, added it to my bookmarks. I hope my proposal is reasonable

Leo Le Bouter (Jan 25 2020 at 23:03, on Zulip):

Leo Le Bouter I can assure you it won't compromise your machine any more than a cargo check will.
If your threat model involves RCE from reviewing untrusted crates you should probably be running your entire setup isolated from anything important, preferably on an airgapped machine, but at least in a VM

I'm as worried about the cargo check but yeah, in the mean time, that's what I do. I would prefer if Rust could make that a little better :-) Isn't it designed for security after all? The #wg-secure-code is working on it as well.

Leo Le Bouter (Jan 25 2020 at 23:04, on Zulip):

@std::Veetaha Could not replicate the issue either. It seems to be caused by a set of things rather than a single thing, which makes it hard to reproduce. The error showed up in my logs and that's it.

Leo Le Bouter (Jan 25 2020 at 23:05, on Zulip):

And thanks!

Leo Le Bouter (Jan 26 2020 at 04:00, on Zulip):

I got the error again, still don't know exactly what caused it, can't reproduce, rewriting my code doesnt seem to work.

error: expected one of `)`, `,`, `::`, or `|`, found `.`
  --> <stdin>:85:54
   |
85 |             e @ Err(std::io::ErrorKind::AlreadyExists.into()) => e.context(CreateSsbDir {
   |                                                      ^ expected one of `)`, `,`, `::`, or `|` here

error: expected one of `)`, `,`, `::`, or `|`, found `.`
  --> <stdin>:85:54
   |
85 |             e @ Err(std::io::ErrorKind::AlreadyExists.into()) => e.context(CreateSsbDir {
   |                                                      ^ expected one of `)`, `,`, `::`, or `|` here

error: expected one of `)`, `,`, `::`, or `|`, found `.`
  --> <stdin>:85:54
   |
85 |             e @ Err(std::io::ErrorKind::AlreadyExists.into()) => e.context(CreateSsbDir {
   |                                                      ^ expected one of `)`, `,`, `::`, or `|` here

free(): invalid next size (fast)
Leo Le Bouter (Jan 26 2020 at 04:01, on Zulip):

I was being a little spammy on Ctrl-Shift-I (Format) -- if that's any relevant.

Leo Le Bouter (Jan 26 2020 at 04:05, on Zulip):

Hmm, I think I can reproduce now. It has to do with where clauses while they're being written.

Leo Le Bouter (Jan 26 2020 at 04:05, on Zulip):
thread '<unnamed>' panicked at 'byte index 3 is not a char boundary; it is inside '\u{3}' (bytes 2..3) of `�u��   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:77
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:61
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1028
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1412
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:65
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:50
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:188
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:205
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:464
  11: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:373
  12: rust_begin_unwind
             at src/libstd/panicking.rs:302
  13: core::panicking::panic_fmt
             at src/libcore/panicking.rs:139
  14: core::str::slice_error_fail
             at src/libcore/str/mod.rs:0
  15: core::str::traits::<impl core::slice::SliceIndex<str> for core::ops::range::Range<usize>>::index::{{closure}}
  16: serde_json::ser::format_escaped_str
  17: serde::ser::SerializeMap::serialize_entry
  18: lsp_server::msg::Message::write
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
free(): invalid next size (fast)
Leo Le Bouter (Jan 26 2020 at 04:05, on Zulip):

Got this as well, now.

Leo Le Bouter (Jan 26 2020 at 04:11, on Zulip):

Also

corrupted double-linked list
Leo Le Bouter (Jan 26 2020 at 04:11, on Zulip):

heap overflow?

Leo Le Bouter (Jan 26 2020 at 04:13, on Zulip):

I think I can reproduce almost reliably now. I'll make a video

Leo Le Bouter (Jan 26 2020 at 04:13, on Zulip):

It only happens in interactivity

Leo Le Bouter (Jan 26 2020 at 04:25, on Zulip):

And when making the video it wont reproduce anymore... urgggh

Leo Le Bouter (Jan 26 2020 at 07:14, on Zulip):

As a hint, it seems to always happen when I write before the first { just after the head of a struct definition.

Leo Le Bouter (Jan 26 2020 at 07:15, on Zulip):

Such as, adding spaces or a newline

Leo Le Bouter (Jan 26 2020 at 09:25, on Zulip):

malloc_consolidate(): invalid chunk size

Leo Le Bouter (Jan 26 2020 at 09:30, on Zulip):

It seems that the overflow is going over the source code buffer:

thread '<unnamed>' panicked at 'byte index 1 is not a char boundary; it is inside '\u{10}' (bytes 0..1) of `��\�stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:77
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:61
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1028
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1412
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:65
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:50
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:188
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:205
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:464
  11: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:373
  12: rust_begin_unwind
             at src/libstd/panicking.rs:302
  13: core::panicking::panic_fmt
             at src/libcore/panicking.rs:139
  14: core::str::slice_error_fail
             at src/libcore/str/mod.rs:0
  15: core::str::traits::<impl core::slice::SliceIndex<str> for core::ops::range::Range<usize>>::index::{{closure}}
  16: serde_json::ser::format_escaped_str
  17: serde::ser::SerializeMap::serialize_entry
  18: lsp_server::msg::Message::write
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
free(): invalid next size (fast)
Leo Le Bouter (Jan 26 2020 at 10:57, on Zulip):

I got a debug trace:

#0  0x00007fff8cebfcf4 in _int_free () from /lib64/libc.so.6
#1  0x000000011461a0b8 in std::sys::unix::alloc::<impl core::alloc::GlobalAlloc for std::alloc::System>::dealloc () at src/libstd/sys/unix/alloc.rs:42
#2  __rdl_dealloc () at src/libstd/alloc.rs:246
#3  0x00000001140023a4 in core::ptr::real_drop_in_place ()
#4  0x0000000113f11d14 in core::ptr::real_drop_in_place ()
#5  0x0000000113f16b3c in core::ptr::real_drop_in_place ()
#6  0x0000000113e64660 in alloc::sync::Arc<T>::drop_slow ()
#7  0x00000001142940e0 in salsa::derived::slot::Slot<DB,Q,MP>::read_upgrade ()
#8  0x00000001142e4188 in salsa::derived::slot::Slot<DB,Q,MP>::read ()
#9  0x0000000113fe150c in <salsa::derived::DerivedStorage<DB,Q,MP> as salsa::plumbing::QueryStorageOps<DB,Q>>::try_fetch ()
#10 0x0000000113e1bc98 in salsa::QueryTable<DB,Q>::get ()
#11 0x00000001140eb9f4 in ra_hir_ty::traits::trait_solve_query ()
#12 0x0000000114055de8 in salsa::runtime::Runtime<DB>::execute_query_implementation ()
#13 0x00000001142c4ae0 in salsa::derived::slot::Slot<DB,Q,MP>::read_upgrade ()
#14 0x000000011422aacc in <salsa::derived::slot::Slot<DB,Q,MP> as salsa::dependency::DatabaseSlot<DB>>::maybe_changed_since ()
#15 0x000000011428a41c in salsa::derived::slot::Slot<DB,Q,MP>::read_upgrade ()
#16 0x00000001142e5408 in salsa::derived::slot::Slot<DB,Q,MP>::read ()
#17 0x0000000113fe7690 in <salsa::derived::DerivedStorage<DB,Q,MP> as salsa::plumbing::QueryStorageOps<DB,Q>>::try_fetch ()
#18 0x0000000113f84fd8 in ra_hir_ty::db::infer ()
#19 0x0000000113e166d0 in ra_hir::code_model::Function::diagnostics ()
#20 0x0000000113e153a0 in ra_hir::code_model::Module::diagnostics ()
#21 0x0000000114060e00 in ra_ide::diagnostics::diagnostics ()
#22 0x000000011408327c in std::panicking::try::do_call ()
#23 0x00000001146244f4 in __rust_maybe_catch_panic () at src/libpanic_unwind/lib.rs:78
#24 0x000000011417979c in ra_db::CheckCanceled::catch_canceled ()
#25 0x0000000113fb4bd0 in ra_ide::Analysis::diagnostics ()
#26 0x0000000113a8779c in ra_lsp_server::main_loop::handlers::publish_diagnostics ()
#27 0x00000001139f1330 in <F as threadpool::FnBox>::call_box ()
#28 0x0000000113c6df1c in std::sys_common::backtrace::__rust_begin_short_backtrace ()
#29 0x00000001146244f4 in __rust_maybe_catch_panic () at src/libpanic_unwind/lib.rs:78
#30 0x0000000113c6e8fc in core::ops::function::FnOnce::call_once{{vtable-shim}} ()
#31 0x0000000114607538 in <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once () at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/boxed.rs:942
#32 0x0000000114623490 in <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once () at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/boxed.rs:942
#33 std::sys_common::thread::start_thread () at src/libstd/sys_common/thread.rs:13
#34 std::sys::unix::thread::Thread::new::thread_start () at src/libstd/sys/unix/thread.rs:79
#35 0x00007fff8d079a88 in start_thread () from /lib64/libpthread.so.0
#36 0x00007fff8cf53998 in clone () from /lib64/libc.so.6
std::Veetaha (Jan 26 2020 at 11:04, on Zulip):

@Leo Le Bouter big thank you for the input, could you please describe the steps you've taken to reproduce this AND get the backtrace (the backtrace part is one of the most interesting to me)?

Leo Le Bouter (Jan 26 2020 at 11:04, on Zulip):

@std::Veetaha Can't say, really. It's not exact what causes it.

Leo Le Bouter (Jan 26 2020 at 11:05, on Zulip):

I can't say more than:

As a hint, it seems to always happen when I write before the first { just after the head of a struct definition.

Leo Le Bouter (Jan 26 2020 at 11:05, on Zulip):

For the backtrace, I attached gdb?

Leo Le Bouter (Jan 26 2020 at 11:06, on Zulip):

And waited until it happened again

Leo Le Bouter (Jan 26 2020 at 11:06, on Zulip):

I'm recompiling ra in debug mode as we speaj

Leo Le Bouter (Jan 26 2020 at 11:06, on Zulip):

speak *

std::Veetaha (Jan 26 2020 at 11:09, on Zulip):

@Leo Le Bouter If you project is open-source, a link to the place where you put { after the head of a struct (whatever this means) would be helpful. Otherwise you could give us an excerpt of the code where the crash is triggered

Leo Le Bouter (Jan 26 2020 at 11:10, on Zulip):

@std::Veetaha It does not seem specific to any particular source code, but https://github.com/leo-lb/ssb-server-rs

std::Veetaha (Jan 26 2020 at 11:11, on Zulip):

I know, yes, it's just to let us reproduce it locally on our machines

Leo Le Bouter (Jan 26 2020 at 11:12, on Zulip):

I don't think you'll be able to reproduce. One example place would be: https://github.com/leo-lb/ssb-packetstream/blob/e6ad8807502a6c37fbc907a9f77eb7eeb06fa3b9/src/stream.rs#L95

std::Veetaha (Jan 26 2020 at 11:13, on Zulip):

We would have to do it or the bug won't be fixed, nevertheless ;) Thanks for this input again!

Leo Le Bouter (Jan 26 2020 at 11:16, on Zulip):

@std::Veetaha In that case, I don't think it will ever get fixed :-/

Leo Le Bouter (Jan 26 2020 at 11:26, on Zulip):

Seems like running ra in debug mode is just not practical at all. So slow!

Leo Le Bouter (Jan 26 2020 at 11:27, on Zulip):

@std::Veetaha If you want to reproduce from that code, try removing the where clause, putting it back character by character, CTRL-S + CTRL-SHIFT-I, and again and again, at some point, the heap will get corrupt

Leo Le Bouter (Jan 26 2020 at 11:28, on Zulip):

Try putting back where: just after the struct and removing, pressing CTRL-SHIFT-I / CTRL-S all while writing it multiple times

std::Veetaha (Jan 26 2020 at 11:28, on Zulip):

@Leo Le Bouter if you have time, may I ask you to formalize what you've done today to reproduce this and put it into an issue on rust-analyzer repo? I could take all that traces you've provided and move them to an issue myself, but I don't have the full context of which concrete commands you used and what steps you take, we need this to be in the form of an algorithm e.g.

  1. I cloned the fresh repo of analyzer (commit hash)
  2. I've built it with <command here>
  3. blah blah
Leo Le Bouter (Jan 26 2020 at 11:28, on Zulip):

where: is invalid syntax but I noticed that's how it happened most

Leo Le Bouter (Jan 26 2020 at 11:30, on Zulip):

I don't have any exact path to reproduction, else I would've done that already, once I have one, I'll be sure to fill an issue

std::Veetaha (Jan 26 2020 at 11:34, on Zulip):

Yes, floating bugs are hard, just mention that the steps you provided are won't necessarily lead to the crash. They would serve just as a context where this issue is more probable to happen

Last update: Sep 18 2020 at 20:15UTC