- Don't use `WString::from_str` for `str`s which are available at
compile-time. Use `L!(input).to_owned()` instead. The reason for this
is that `WString::from_str` can cause problems if the input contains
PUA bytes which we use for our custom encoding scheme. In such cases,
`bytes2wcstring` should be used, to avoid problems when decoding the
`WString`. Removing harmless usages of `WString::from_str` allows us
to focus on the potentially dangerous ones which don't convert
`str`'s that are compiled into the binary.
- Make `cstr2wcstring` actually take `CStr` as its input. The former
version was only used in one place, and the conversion to `CStr`
should happen there, where it can be checked that the conversion makes
sense and is safe. The new version is used in
`src/env/environmant.rs`, to avoid `to_bytes()` calls cluttering the
code there.
- Add `osstr2wcstring` function. This function also works for `Path`.
Now, these types can be converted to widestrings with much less
syntactic clutter.
Having the prelude in wchar is not great. The wchar module was empty
except for the prelude, and its prelude included things from wutil.
Having a top-level prelude module in the main crate resolves this. It
allows us to completely remove the wchar module, and a top-level prelude
module makes more sense conceptually. Putting non-wchar things into the
prelude also becomes more sensible, if we ever want to do that.
Closes#12182
The `mkstemp` function opens files without setting `O_CLOEXEC`. We could
manually set this using `fnctl` once the file is opened, but that has
the issue of introducing race conditions. If fish `exec`s in another
thread before the `fnctl` call completes, the file would be left open.
One way of mitigating this is `mkostemp`, but that function is not
available on all systems fish supports, so we can't rely on it.
Instead, build our own tempfile creation logic which uses the `rand`
crate for getting entropy and relies on Rust's stdlib for the rest.
The stdlib functions we use set `O_CLOEXEC` by default.
For directory creation we keep using `mkdtemp`, since there we don't
open anything. We could replace this by extending our custom logic a
bit, which would allow us to drop the `nix` dependency for our
`tempfile` crate, but since the code is simpler as it is now and we need
nix in fish's main crate, there is no need to modify the directory
creation code.
Part of #12030
Issue #11933 stems from Cygwin's flock implementation not being thread
safe. Previous code fixed the worst behavior (deadlock), at least in
some circumstance but still failed the history tests.
This new workaround correctly make flock usage thread safe by surrounding
it with a mutex lock.
The failing test can now pass and is much faster (5s instead of 15 on
my machine)
Upstream bug report: https://cygwin.com/pipermail/cygwin/2025-November/258963.htmlCloses#12068Closes#11933
Use fstat() instead of lseek() to determine history file size.
Pass around the file_id instead of recomputing it.
This saves a few syscalls; no behavior change is expected.
As mentioned in 6896898769 (Add [lints] table to suppress lints
across all our crates, 2024-01-12), we can use workspace lints in
Cargo.toml now that we have MSRV >=1.74 and since we probably don't
support building without cargo.
This implies moving some lints from src/lib.rs to "workspace.lints".
While at it, address some of them insrtead.
Some string handling functions deal with `Vec<u8>` or `&[u8]`, which
have been referred to as `string` or `str` in the function names. This
is confusing, since they don't deal with Rust's `String` type. Use
`bytes` in the function names instead to reduce confusion.
Closes#11969
Update our MSRV to Rust 1.85.
Includes fixes for lints which were previously suppressed due to them
relying on features added after Rust 1.70.
Rust 1.85 prints a warning when using `#[cfg(target_os = "cygwin")]`, so
we work around the one instance where this is a problem for now. This
workaround can be reverted when we update to Rust 1.86 or newer.
Certain old versions of macOS are no longer supported by Rust starting
with Rust 1.74, so this commit raises our macOS version requirement to
10.12.
https://blog.rust-lang.org/2023/09/25/Increasing-Apple-Version-Requirements/https://github.com/fish-shell/fish-shell/pull/11961#discussion_r2442415411Closes#11961
This commit adds `style_edition = "2024"` as a rustfmt config setting.
All other changes are automatically generated by `cargo fmt`.
The 2024 style edition fixes several bugs and changes some defaults.
https://doc.rust-lang.org/edition-guide/rust-2024/rustfmt-style-edition.html
Most of the changes made to our code result from a different sorting
method for `use` statements, improved ability to split long lines, and
contraction of short trailing expressions into single-line expressions.
While our MSRV is still 1.70, we use more recent toolchains for
development, so we can already benefit from the improvements of the new
style edition. Formatting is not require for building fish, so builds
with Rust 1.70 are not affected by this change.
More context can be found at
https://github.com/fish-shell/fish-shell/issues/11630#issuecomment-3406937077Closes#11959
On Cygwin, fsync() on the history and uvar file fails with "permission
denied". Work around this by opening the file in read-write mode
instead of read-only mode.
Closes#11948
Length modifiers are useless. This simplifies the code a bit, results in
more consistency, and allows removing a few PO messages which only
differed in the use of length modifiers.
Closes#11878
There is an unlikely issue if two shells are concurrently rewriting the
history file:
- fish A runs rename("fish_history.DEADBEEF") (rewriting a history file with)
- fish B starts rewriting the history file; since "fish_history.DEADBEEF" no longer exists, it can in theory use that filename
- fish A runs wunlink("fish_history.DEADBEEF"), destroying fish B's work
Fix this by not calling wunlink() iff we successfully rename it.
[ja: add commit message and fix "!do_save" case]
These changes are not intended to change any behavior. They are done to
facilitate closing the tmpfile before renaming, which is required for
correctness on some filesystems (at least btrfs). Using a `ScopeGuard` which
unlinks when the file is closed/dropped does not work in this context, so the
relevant code is wrapped in a function and the tmpfile is unlinked after the
function returns.
The cached information might become outdated. It is important that all fish
processes use the same mutual exclusion logic (either `flock`-based or the
fallback), because the two methods do not provide mutual exclusion from one
another.
Avoiding caching makes the behavior independent on previous system states,
resulting in fish instances performing file operations at the same time to use
the same locking logic.
More detailed discussion in
https://github.com/fish-shell/fish-shell/pull/11492#discussion_r2134543447
Both history files and universal variables files are accessed by multiple
processes, which need a way to synchronize their accesses.
This synchronization logic is mixed in with the logic for reading and updating
the files' contents, which results in messy code, duplicated locking
implementations, and inconsistencies.
Moreover, the existing implementations are flawed which has resulted in file
corruption (e.g. https://github.com/fish-shell/fish-shell/issues/10300).
The new approach separates the synchronization logic from the rest.
There are two approaches to synchronization.
- The primary one is using `flock(2)` to lock the directory containing the file
which requires synchronized access. We do not lock the file holding the data
directly because this file might be replaced, which can result in locking
succeeding when it should block. Locking the directory solves this problem.
To avoid inconsistent file states, changes are first written to a temporary
file, which is then renamed into place while still holding the lock.
- In some situations `flock` locks are unavailable or take a long time. This
mostly applies to remote file systems. If we think that a directory is located
on a remote file system, we do not attempt to use `flock`.
As a fallback, we have a lockless approach, which uses file metadata (device,
inode, size, ctime, mtime) to identify file versions.
We then read from the file, write the updated data to a temporary file,
check if the file id for the path is the same as before we started reading,
and if so we rename the temporary file such that it replaces the old file.
Note that races are possible between the file id check and the rename syscall.
If we detect a file id mismatch, we retry, up to a predetermined number of
attempts.
The operations which should be performed are passed to the functions handling
synchronization as `Fn`s. Because we might have to run these operations
repeatedly when retrying, they should be executable arbitrarily often without
causing side-effects relevant to the program. This requires some changes to
functions accessing these files. In many cases, they have to work with
non-mutable references, which requires that they return the data which should be
updated in the program state, instead of directly assigning to the appropriate
location.
Locking via `O_EXLOCK`, which was used for the universal variable file, is no
longer supported. That version of locking locks the file operated on directly,
instead of its directory. According to the man pages of {Open,Free,Net}BSD
and macOS, these locks have flock semantics. So if flock is available, we can
use it, and if it is not, the `O_EXLOCK` flag does not help.
There is no need for separate implementations for history and uvar file
handling. Having a shared implementation ensures that creating temporary files
is handled consistently and removes clutter from the source files which want to
create temporary files.
