Refine how finalize_entry sets executable bits
#1764
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This implements a function for tests to safely read the current process umask without the usual race condition of doing so, at the expense of using subprocesses to do it. This just calls a shell and runs `umask` from it (which is expected to be a builtin and, on many systems, is available as a builtin but not an executable). Even though this is safe, including thread-safe, it is unlikely to be suitable for use outside of tests, because of its use of `expect` and assertions when there are errors, combined with the possibly slow speed of using subprocesses. Given that this is effecitvely running a tiny shell script to do the work, why is it not instead a fixture script that is named in a `.gitignore` file so that it is not tracked? The reason is that the outcomes of running such fixture scripts are still saved across separate test runs, but it is useful to be able to run the tests with differnt umasks, e.g. `(umask 077; cargo nextest run ...)`. The immediate purpose is in forthcoming tests that, when checkout sets +x on an existing file, it doesn't set excessive permissions. The fix to pass such a test is not currently planned to use the umask explicitly. But the tests will use it, at least to detect when they cannot really verify the code under test on the grounds that they are running with an excessively permissive umask that doesn't allow behavior that only occurs with a generally reasonable umask to be observed.
This expands the `overwriting_files_and_lone_directories_works` test function to check not only that executable permissions have been added at least for the owner, but that write permissions are not added anywhere they would be excessive. The write permissions bits of the current test runner process umask is used to gauge this. This means we are expecting different permissions in different environments. `(umask ...; cargo nextest run ...)` can be used to try it with different umasks if desired, but this should not typically be necessary. Possible under-restrictive umasks could make the test pass even if the underlying bug is not fixed; this is avoided by also testing that the umask is sufficient to facilitate the test. (This is really why the test accesses the umask in the first place: in environments where files would automatically be created with completely unrestricted permissions, the expected behavior of the code under test may be to do that, but running the tests in such an environment is insufficient to check if the bug is fixed.)
This fixes a bug that occurred when a regular file tracked as executable was checked out in a non-exclusive checkout on a Unix-like system (where the destination is a filesystem that supports Unix-style executable permissions). This occurs when `destination_is_initially_empty: true` is not explicitly set. Whether the file existed before or not, it is created without attempting to set its permissions to include executable bits, then the executable bits are added afterwards. On Unix-like systems, the file was given unrestricted 0777 permissions and was thus world writable. This happened regardless of the process umask or any other contextual factors. Although `0o777` is given as the mode both when permissions are set on creation (using `OpenOptionsExt::mode` and `OpenOptions::open`, which delegates to `open`), and when they are set after creation (using `PermissionsExt::set_mode` and `std::fs::set_permissions`, which delegates to `chmod`), the cases do not treat their mode arguments equivalently. - The first situation worked correctly because `open` automatically respects the current process umask. (The system takes care of this, as it is part of the semantics of creating a new file and specifying desired permissions.) So 777 was really expressing the idea of maximal permissions of those considered safe under the current configuration, including executable permissions. - But the second situation did not work correctly, because `chmod` calls try to set the exact permissions specified (and usually succeed). Unlike `open`, with `chmod` there is no implicit use of the umask. Various fixes are possible. The fix implemented here hews to the existing design as much as possible while avoiding setting any write permissions (though existing write permissions are preserved) and also avoiding setting executable permissions for whoever does not have read permissions. We: 1. Unset the setuid, setgid, and sticky bits, in the rare case that any of them are set. Keeping them could be unsafe or have unexpected effects when set for a file that may conceptually hold different data or serve a different purpose (since this is a checkout). For the setuid and setgid bits, it would be unsafe to keep them when adding new executable permissions. The intent of setuid and setgit bits is ambiguous in this situation, since they may have been meant only to apply to an earlier version of the file, especially since users may expect the file to be deleted and a new file of the same name to be created, rather than to confer extra abilities when executable bits are added in the future. Unsetting them makes adding executable bits where read bits are already present (which we will do) a reasonably safe operation. In the case of the setgid bit, another reason to remove it is that, on some systems, the setgid bit in the absence of any executable bits has the different meaning of enabling mandatory file locking. If the setgid bit was set for this purpose, then the effect of setting the EGID and potentialy elevating the privileges of the user who runs it is surely not intended. 2. Check which read bits (for owner, group, and other) are already set on the file. We do this only by looking at the mode. For example, ACLs do not participate. 3. Set executable bits corresponding to the preexisting read bits. That is, for each of the owner, group, and others, if it can read (according to the file mode), set it to be able to execute as well. In some cases, this may have a different effect from what would happen if the file were created anew with the desired permissions specified by a broad mode (e.g. 777) and subject to the umask. This is because it is possible to have a umask that limits read and execute permissions differently. Also, the file may have had its permissions modified in some other way since creation. The idea here is to keep the idea behind the way it worked before, but avoid adding any write permissions or giving permissions to users who don't already have any. This fixes the bug where executable files were sometimes checked out with unrestricted, world-writable permissions. However, this is not necessarily the approach that will be kept long-term. This does not attempt to avoid effects that are fundamental to the reuse of an existing file (versus the creation of a new one). In particular, this currently assumes that observing changes due to a checkout through other hard links to a file (whose link count is higher than 1) is an intended or otherwise acceptable effect of using multiple hard links. Another aspect of the current approach that is preserved so far but that may eventually change is that some operations are done through an open file object while others are done using the path, and there may be unusual situations, perhaps involving long-running process smudge filters and separate concurrent modification of the working tree, where they diverge. However, the specific scenario of path coming to refer to something that is no longer a regular file will be covered in a subsequent commit.
This does not make a difference in typical cases, and anytime it matters something has probably gone unexpectedly, but this narrows the window of time during which a race condition could occur where a regular file has been replaced with something else at the same path (e.g. a directory) by some other process. An example of why it's valuable to avoid this is that if the entry is a directory then executable permissions have a different meaning and adding them could increase access unintentionally. Likewise, when we set executable permissions we remove setuid, setgid, and sticky bits, which also have different meanings for directories; in particular, on a directory the sticky bit restricts deletion. (This also automatically avoids some problems in the case of `finalize_entry` being called with a path to set executable that was never a regular file in the first place. That should not happen, though, and allowing that is not a goal of this change.)
Trimming Unicode whitespace is not equivalent to trimming ASCII whitespace, but in this case I think they are equally okay to do. If we get Unicode whitespace that is not ASCII whitespace from running `umask`, in principle it's possible this is due to it being a misinterpretation of something not Unicode. But if `umask` gives us anything besides whitespace and a single nonempty sequence of octal digits, then (a) that's very strange, and (b) it should fail and indicate its failure with a nonzero exit status, which we check.
Byron
approved these changes
Jan 13, 2025
|
I will try to release this in the coming hours, hoping that I can land a feature first. |
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Jan 13, 2025
There were two places in GitoxideLabs#1764 where I had an unconditional import that causes a warning on Windows about how it is unused. 4d5e656 fixed one. This fixes the other: warning: unused import: `Permissions` --> gix-worktree-state\src\checkout\entry.rs:3:23 | 3 | fs::{OpenOptions, Permissions}, | ^^^^^^^^^^^
|
Thanks! I just did a few follow-up things:
Update: This has been assigned RUSTSEC-2025-0001. (I've added it to the PR description.) |
This was referenced Jan 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The fixed the vulnerability RUSTSEC-2025-0001 (GHSA-fqmf-w4xh-33rh, CVE-2025-22620).
I'll update this with a link to the global GHSA (i.e. the entry in the GitHub Advisory Database) once it is available. I'll also try to update this with more information, but the main source of information will remain the advisories. The original PR description follows.
This adjusts how executable bits that are added subsequent to file creation, as occurs when
destination_is_initially_empty: trueis not included in checkout options, are computed and added. The idea is to produce an effect more similar to what users expect and may wish to rely on, as well as to make things somewhat closer to what Git does in this area while still keeping the fundamental design the same.Further changes, which may affect the design more or in other ways end up being trickier, may still be worthwhile; I do not claim that the specific approach here will ultimately be the best one.