[BUG][CRASH] Occasional crashes when reloading file mounted with SSHFS

kak-0 commented 2 years ago

Version of Kakoune

v2021.11.08-59-g9acd4e62

Reproducer

Kakoune occasionally crashes when reloading a file that's mounted with SSHFS after the file is modified outside of Kakoune (e.g. after running git checkout on the remote machine).

Outcome

Killed by signal SIGBUS.

Backtrace:

#0  0x000055d76d10dc99 parse_file<Kakoune::reload_file_buffer(Kakoune::Buffer&)::<lambda(auto:39&& ...)> > (kak + 0xc1c99)
#1  0x000055d76d10e0e1 Kakoune::reload_file_buffer(Kakoune::Buffer&) (kak + 0xc20e1)
#2  0x000055d76d11184d Kakoune::Client::reload_buffer() (kak + 0xc584d)
#3  0x000055d76d116560 Kakoune::Client::on_buffer_reload_key(Kakoune::Key) (kak + 0xca560)
#4  0x000055d76d1c812e std::function<void (Kakoune::Key, Kakoune::Context&)>::operator()(Kakoune::Key, Kakoune::Context&) const (kak + 0x17c12e)
#5  0x000055d76d1c3347 Kakoune::InputMode::handle_key(Kakoune::Key) (kak + 0x177347)
#6  0x000055d76d113f2c Kakoune::Client::process_pending_inputs() (kak + 0xc7f2c)
#7  0x000055d76d11bcc8 Kakoune::ClientManager::process_pending_inputs() (kak + 0xcfcc8)
#8  0x000055d76d2126fc Kakoune::run_server(Kakoune::StringView, Kakoune::StringView, Kakoune::StringView, Kakoune::Optional<Kakoune::BufferCoord>, Kakoune::ServerFlags, Kakoune::UIType, Kakoune::DebugFlags, Kakoune::ArrayView<Kakoune::StringView const>) (kak + 0x1c66fc)
#9  0x000055d76d0f78d9 main (kak + 0xab8d9)
#10 0x00007fb655299b25 __libc_start_main (libc.so.6 + 0x27b25)
#11 0x000055d76d0f82de _start (kak + 0xac2de)

Expectations

No response

Additional information

I got a SIGBUS signal in parse_file on this line:

        if (*it == '\n')

This line tries to read a memory-mapped file mapped in MappedFile::MappedFile:

    data = (const char*)mmap(nullptr, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);

mmaps(2) says "It is unspecified whether changes made to the file after the mmap() call are visible in the mapped region" when using MAP_PRIVATE. Maybe the crash is due to a race condition where the file contents or size changes while Kakoune is reading it.

This could also be a bug in how SSHFS implements mmap.

In either case, I guess the only workaround is to read the file with read(2) instead of mmap(2).

Does anyone else experience crashes when using SSHFS? Is there another workaround?

krobelus commented 2 years ago

The SIGBUS also happens to kak file -e 'set global autoreload yes' when running while true; do echo $RANDOM > file; done, so I don't think this is specific to SSHFS.

Screwtapello commented 2 years ago

mmap(2) says:

SIGBUS: Attempted access to a page of the buffer that lies beyond the end of the mapped file.

The parse_file() function pointed to is, I think, this one:

https://github.com/mawww/kakoune/blob/0b29fcf32ac756dc54ec5c52db63340c9b3692e9/src/buffer_utils.cc#L119-L145

Note that we get the file size on line 122, and then access the file data on line 137, a classic time-of-check-to-time-of-use (TOCTOU) bug. It's possible (although unlikely) for the file-size to change between those two moments (as in krobelus' test-case) and cause Kakoune to crash.

It's possible that SSHFS makes the problem worse - maybe it caches the expected file-size locally (so that the 'time of check' is not when Kakoune checks, but when the cache was last refreshed), or maybe it's just network latency that pushes "time-of-check" and "time-of-use" far enough apart to cause problems.

Unfortunately, unlike other APIs there's no good way to mmap() the entire file in one step and prevent TOCTOU bugs entirely. Alternatives include:

run Kakoune on the remote host via SSH, minimising Kakoune<->disk latency at the expense of user<->Kakoune latency
run the Kakoune session on the remote host, forward the session socket to the local machine with SSH, and run a local client
add an option to Kakoune to disable mmap(2) and only use read(2)/write(2) for file I/O.

mawww / kakoune