Skip to content

Commit

Permalink
mingw: safeguard better against backslashes in file names
Browse files Browse the repository at this point in the history
In 224c7d7 (mingw: only test index entries for backslashes, not tree
entries, 2019-12-31), we relaxed the check for backslashes in tree
entries to check only index entries.

However, the code change was incorrect: it was added to
`add_index_entry_with_check()`, not to `add_index_entry()`, so under
certain circumstances it was possible to side-step the protection.

Besides, the description of that commit purported that all index entries
would be checked when in fact they were only checked when being added to
the index (there are code paths that do not do that, constructing
"transient" index entries).

In any case, it was pointed out in one insightful review at
#2437 (comment)
that it would be a much better idea to teach `verify_path()` to perform
the check for a backslash. This is safer, even if it comes with two
notable drawbacks:

- `verify_path()` cannot say _what_ is wrong with the path, therefore
  the user will no longer be told that there was a backslash in the
  path, only that the path was invalid.

- The `git apply` command also calls the `verify_path()` function, and
  might have been able to handle Windows-style paths (i.e. with
  backslashes instead of forward slashes). This will no longer be
  possible unless the user (temporarily) sets `core.protectNTFS=false`.

Note that `git add <windows-path>` will _still_ work because
`normalize_path_copy_len()` will convert the backslashes to forward
slashes before hitting the code path that creates an index entry.

The clear advantage is that `verify_path()`'s purpose is to check the
validity of the file name, therefore we naturally tap into all the code
paths that need safeguarding, also implicitly into future code paths.

The benefits of that approach outweigh the downsides, so let's move the
check from `add_index_entry_with_check()` to `verify_path()`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
  • Loading branch information
dscho authored and Git for Windows Build Agent committed Jan 9, 2020
1 parent de9616d commit d0dc65b
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 7 deletions.
12 changes: 6 additions & 6 deletions read-cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,7 @@ static int verify_dotfile(const char *rest, unsigned mode)

int verify_path(const char *path, unsigned mode)
{
char c;
char c = 0;

if (has_dos_drive_prefix(path))
return 0;
Expand All @@ -974,6 +974,7 @@ int verify_path(const char *path, unsigned mode)
if (is_dir_sep(c)) {
inside:
if (protect_hfs) {

if (is_hfs_dotgit(path))
return 0;
if (S_ISLNK(mode)) {
Expand All @@ -982,6 +983,10 @@ int verify_path(const char *path, unsigned mode)
}
}
if (protect_ntfs) {
#ifdef GIT_WINDOWS_NATIVE
if (c == '\\')
return 0;
#endif
if (is_ntfs_dotgit(path))
return 0;
if (S_ISLNK(mode)) {
Expand Down Expand Up @@ -1278,11 +1283,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
int new_only = option & ADD_CACHE_NEW_ONLY;

#ifdef GIT_WINDOWS_NATIVE
if (protect_ntfs && strchr(ce->name, '\\'))
return error(_("filename in tree entry contains backslash: '%s'"), ce->name);
#endif

if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
cache_tree_invalidate_path(istate, ce->name);

Expand Down
2 changes: 1 addition & 1 deletion t/t7415-submodule-names.sh
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
hash="$(echo x | git hash-object -w --stdin)" &&
test_must_fail git update-index --add \
--cacheinfo 160000,$rev,d\\a 2>err &&
test_i18ngrep backslash err &&
test_i18ngrep "Invalid path" err &&
git -c core.protectNTFS=false update-index --add \
--cacheinfo 100644,$modules,.gitmodules \
--cacheinfo 160000,$rev,c \
Expand Down

0 comments on commit d0dc65b

Please sign in to comment.