config: quote values containing CR character

When reading the config, values that contain a trailing CRLF are
stripped. If the value itself has a trailing CR, the normal LF that
follows results in the CR being unintentionally stripped. This may lead
to unintended behavior due to the config value written being different
when it gets read.

One such issue involves a repository with a submodule path containing a
trailing CR. When the submodule gets initialized, the submodule is
cloned without being checked out and has "core.worktree" set to the
submodule path. The git-checkout(1) that gets spawned later reads the
"core.worktree" config value, but without the trailing CR, and
consequently attempts to checkout to a different path than intended.

If the repository contains a matching path that is a symlink, it is
possible for the submodule repository to be checked out in arbitrary
locations. This is extra bad when the symlink points to the submodule
hooks directory and the submodule repository contains an executable
"post-checkout" hook. Once the submodule repository checkout completes,
the "post-checkout" hook immediately executes.

To prevent mismatched config state due to misinterpreting a trailing CR,
wrap config values containing CR in double quotes when writing the
entry. This ensures a trailing CR is always separated for an LF and thus
prevented from getting stripped.

Note that this problem cannot be addressed by just quoting each CR with
"\r". The reading side of the config interprets only a few backslash
escapes, and "\r" is not among them. This fix is sufficient though
because it only affects the CR at the end of a line and any literal CR
in the interior is already preserved.

Co-authored-by: David Leadbeater <dgl@dgl.cx>
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>

diff --git a/config.c b/config.c
index 9ff6ae1cb903a0690295a7d5bb452ef5f35c12e1..629981451d303bce3547fa396dd68d73ba56b628 100644 (file)
--- a/config.c
+++ b/config.c
@@ -2999,7 +2999,7 @@ static ssize_t write_pair(int fd, const char *key, const char *value,
        if (value[0] == ' ')
                quote = "\"";
        for (i = 0; value[i]; i++)
-               if (value[i] == ';' || value[i] == '#')
+               if (value[i] == ';' || value[i] == '#' || value[i] == '\r')
                        quote = "\"";
        if (i && value[i - 1] == ' ')
                quote = "\"";

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f4e27521344920ca5cf85b20a263487e2d794b52..1010410b7e292693a01421e0d52a98335a622b40 100755 (executable)
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2590,4 +2590,15 @@ test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such
        grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
 '
 
+test_expect_success 'writing value with trailing CR not stripped on read' '
+       test_when_finished "rm -rf cr-test" &&
+
+       printf "bar\r\n" >expect &&
+       git init cr-test &&
+       git -C cr-test config set core.foo $(printf "bar\r") &&
+       git -C cr-test config get core.foo >actual &&
+
+       test_cmp expect actual
+'
+
 test_done

diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh
index 5b845e899bf17caec8e26cff4fb329e8701fb77b..20262855664a9737c92839948a9e7d7f75e7305b 100755 (executable)
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -347,4 +347,37 @@ test_expect_success 'checkout -f --recurse-submodules must not use a nested gitd
        test_path_is_missing nested_checkout/thing2/.git
 '
 
+test_expect_success SYMLINKS,!WINDOWS,!MINGW 'submodule must not checkout into different directory' '
+       test_when_finished "rm -rf sub repo bad-clone" &&
+
+       git init sub &&
+       write_script sub/post-checkout <<-\EOF &&
+       touch "$PWD/foo"
+       EOF
+       git -C sub add post-checkout &&
+       git -C sub commit -m hook &&
+
+       git init repo &&
+       git -C repo -c protocol.file.allow=always submodule add "$PWD/sub" sub &&
+       git -C repo mv sub $(printf "sub\r") &&
+
+       # Ensure config values containing CR are wrapped in quotes.
+       git config unset -f repo/.gitmodules submodule.sub.path &&
+       printf "\tpath = \"sub\r\"\n" >>repo/.gitmodules &&
+
+       git config unset -f repo/.git/modules/sub/config core.worktree &&
+       {
+               printf "[core]\n" &&
+               printf "\tworktree = \"../../../sub\r\"\n"
+       } >>repo/.git/modules/sub/config &&
+
+       ln -s .git/modules/sub/hooks repo/sub &&
+       git -C repo add -A &&
+       git -C repo commit -m submodule &&
+
+       git -c protocol.file.allow=always clone --recurse-submodules repo bad-clone &&
+       ! test -f "$PWD/foo" &&
+       test -f $(printf "bad-clone/sub\r/post-checkout")
+'
+
 test_done