From patchwork Wed Apr 10 09:55:25 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thalia Archibald X-Patchwork-Id: 13624000 Received: from mail-4323.proton.ch (mail-4323.proton.ch [185.70.43.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DDB1F13D28F for ; Wed, 10 Apr 2024 09:55:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.70.43.23 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712742941; cv=none; b=FVem1J8QnaflTHzFkOIflmRRtYa1U7yppRGgwhHwn4G3wbjC/P49GGFcAdfFPRhmohRuBAb1LCrm4E6b8MkFc71yzLwsUitm/VAubQi+y8TzNOGE2+eHOdxM0b6cEUKrotA3+2gyi3ceI3BQSy1+m0OsObgKUZMc7O/E1bDuyBg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712742941; c=relaxed/simple; bh=acg6uErMN+6ZvUrywvbmTO9QG4lfIFJFCdqgvY98wpM=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=gCQG53GwpUaDTiQ60wgFqPvr8CUew5Pt07M0qYJiOGHG2mLht3gDrAYZaAIwv4LUJyx7NGnNnQ7U0/ThgiQrIIawai7OdNDbJIBpJiBpfmfFUQ+unA9P5InUJwbqvgITBKyA6c0ZwGKR0vLaR/y/p4BlJ67mJq05SU1AeadrM7w= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=archibald.dev; spf=pass smtp.mailfrom=archibald.dev; dkim=pass (2048-bit key) header.d=archibald.dev header.i=@archibald.dev header.b=pICivzSm; arc=none smtp.client-ip=185.70.43.23 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=archibald.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=archibald.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=archibald.dev header.i=@archibald.dev header.b="pICivzSm" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=archibald.dev; s=protonmail3; t=1712742936; x=1713002136; bh=tiGSRuofedbARXrF5142zIgTsD3QEIWWMPyAPNGtgnw=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=pICivzSmsqIHW6Mi8g5p2TtI1qvNxA0/og0cjF6imWlLXm6+8bjz8RfdUuRjYzSHw PsIdQ0pxvt3ieVFcSNN0vdvpUjR3S214HyLBC/KE4o5p9LSURVWu5OAdmVB0+pRh6m asL7G4JmzDMbchOWJIiF1nGrxULgtaDvKjmodScamH0VeUkcO8MJS6OS+sjA29y9OJ c31gJ7RXk0nrB7uh0DfbGIZNvq4I5s3jEsY3TRu3Bd9E+M6gHqbPE0GWNjWxXyjYud ax94DdA7d+MOX+/BLSqJE5mhLRQLarpJlZADsUUt4vnWqXCJUNyJSp2tBNpQ9iNo6B BpDygX7/KWY3g== Date: Wed, 10 Apr 2024 09:55:25 +0000 To: git@vger.kernel.org From: Thalia Archibald Cc: Patrick Steinhardt , Chris Torek , Elijah Newren , Thalia Archibald Subject: [PATCH v3 1/8] fast-import: tighten path unquoting Message-ID: In-Reply-To: References: <20240322000304.76810-1-thalia@archibald.dev> Feedback-ID: 63908566:user:proton Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Path parsing in fast-import is inconsistent and many unquoting errors are suppressed or not checked. appears in the grammar in these places: filemodify ::= 'M' SP ( | 'inline') SP LF filedelete ::= 'D' SP LF filecopy ::= 'C' SP SP LF filerename ::= 'R' SP SP LF ls ::= 'ls' SP SP LF ls-commit ::= 'ls' SP LF and fast-import.c parses them in five different ways: 1. For filemodify and filedelete: Try to unquote . If it unquotes without errors, use the unquoted version; otherwise, treat it as literal bytes to the end of the line (including any number of SP). 2. For filecopy (source) and filerename (source): Try to unquote . If it unquotes without errors, use the unquoted version; otherwise, treat it as literal bytes up to, but not including, the next SP. 3. For filecopy (dest) and filerename (dest): Like 1., but an unquoted empty string is forbidden. 4. For ls: If starts with `"`, unquote it and report parse errors; otherwise, treat it as literal bytes to the end of the line (including any number of SP). 5. For ls-commit: Unquote and report parse errors. (It must start with `"` to disambiguate from ls.) In the first three, any errors from trying to unquote a string are suppressed, so a quoted string that contains invalid escapes would be interpreted as literal bytes. For example, `"\xff"` would fail to unquote (because hex escapes are not supported), and it would instead be interpreted as the byte sequence '"', '\\', 'x', 'f', 'f', '"', which is certainly not intended. Some front-ends erroneously use their language's standard quoting routine instead of matching Git's, which could silently introduce escapes that would be incorrectly parsed due to this and lead to data corruption. The documentation states “To use a source path that contains SP the path must be quoted.”, so it is expected that some implementations depend on spaces being allowed in paths in the final position. Thus we have two documented ways to parse paths, so simplify the implementation to that. Now we have: 1. `parse_path_eol` for filemodify, filedelete, filecopy (dest), filerename (dest), ls, and ls-commit: If starts with `"`, unquote it and report parse errors; otherwise, treat it as literal bytes to the end of the line (including any number of SP). 2. `parse_path_space` for filecopy (source) and filerename (source): If starts with `"`, unquote it and report parse errors; otherwise, treat it as literal bytes up to, but not including, the next SP. It must be followed by SP. There remain two special cases: The dest in filecopy and rename cannot be an unquoted empty string (this will be addressed subsequently) and in ls-commit must be quoted to disambiguate it from ls. Signed-off-by: Thalia Archibald --- builtin/fast-import.c | 104 ++++++++++------- t/t9300-fast-import.sh | 258 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 318 insertions(+), 44 deletions(-) diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 782bda007c..ce9231afe6 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -2258,10 +2258,56 @@ static uintmax_t parse_mark_ref_space(const char **p) return mark; } +/* + * Parse the path string into the strbuf. It may be quoted with escape sequences + * or unquoted without escape sequences. When unquoted, it may only contain a + * space if `include_spaces` is nonzero. + */ +static void parse_path(struct strbuf *sb, const char *p, const char **endp, + int include_spaces, const char *field) +{ + if (*p == '"') { + if (unquote_c_style(sb, p, endp)) + die("Invalid %s: %s", field, command_buf.buf); + } else { + if (include_spaces) + *endp = p + strlen(p); + else + *endp = strchrnul(p, ' '); + strbuf_add(sb, p, *endp - p); + } +} + +/* + * Parse the path string into the strbuf, and complain if this is not the end of + * the string. It may contain spaces even when unquoted. + */ +static void parse_path_eol(struct strbuf *sb, const char *p, const char *field) +{ + const char *end; + + parse_path(sb, p, &end, 1, field); + if (*end) + die("Garbage after %s: %s", field, command_buf.buf); +} + +/* + * Parse the path string into the strbuf, and ensure it is followed by a space. + * It may not contain spaces when unquoted. Update *endp to point to the first + * character after the space. + */ +static void parse_path_space(struct strbuf *sb, const char *p, + const char **endp, const char *field) +{ + parse_path(sb, p, endp, 0, field); + if (**endp != ' ') + die("Missing space after %s: %s", field, command_buf.buf); + (*endp)++; +} + static void file_change_m(const char *p, struct branch *b) { static struct strbuf uq = STRBUF_INIT; - const char *endp; struct object_entry *oe; struct object_id oid; uint16_t mode, inline_data = 0; @@ -2299,11 +2345,8 @@ static void file_change_m(const char *p, struct branch *b) } strbuf_reset(&uq); - if (!unquote_c_style(&uq, p, &endp)) { - if (*endp) - die("Garbage after path in: %s", command_buf.buf); - p = uq.buf; - } + parse_path_eol(&uq, p, "path"); + p = uq.buf; /* Git does not track empty, non-toplevel directories. */ if (S_ISDIR(mode) && is_empty_tree_oid(&oid) && *p) { @@ -2367,48 +2410,29 @@ static void file_change_m(const char *p, struct branch *b) static void file_change_d(const char *p, struct branch *b) { static struct strbuf uq = STRBUF_INIT; - const char *endp; strbuf_reset(&uq); - if (!unquote_c_style(&uq, p, &endp)) { - if (*endp) - die("Garbage after path in: %s", command_buf.buf); - p = uq.buf; - } + parse_path_eol(&uq, p, "path"); + p = uq.buf; tree_content_remove(&b->branch_tree, p, NULL, 1); } -static void file_change_cr(const char *s, struct branch *b, int rename) +static void file_change_cr(const char *p, struct branch *b, int rename) { - const char *d; + const char *s, *d; static struct strbuf s_uq = STRBUF_INIT; static struct strbuf d_uq = STRBUF_INIT; - const char *endp; struct tree_entry leaf; strbuf_reset(&s_uq); - if (!unquote_c_style(&s_uq, s, &endp)) { - if (*endp != ' ') - die("Missing space after source: %s", command_buf.buf); - } else { - endp = strchr(s, ' '); - if (!endp) - die("Missing space after source: %s", command_buf.buf); - strbuf_add(&s_uq, s, endp - s); - } + parse_path_space(&s_uq, p, &p, "source"); s = s_uq.buf; - endp++; - if (!*endp) + if (!*p) die("Missing dest: %s", command_buf.buf); - - d = endp; strbuf_reset(&d_uq); - if (!unquote_c_style(&d_uq, d, &endp)) { - if (*endp) - die("Garbage after dest in: %s", command_buf.buf); - d = d_uq.buf; - } + parse_path_eol(&d_uq, p, "dest"); + d = d_uq.buf; memset(&leaf, 0, sizeof(leaf)); if (rename) @@ -3152,6 +3176,7 @@ static void print_ls(int mode, const unsigned char *hash, const char *path) static void parse_ls(const char *p, struct branch *b) { + static struct strbuf uq = STRBUF_INIT; struct tree_entry *root = NULL; struct tree_entry leaf = {NULL}; @@ -3168,16 +3193,9 @@ static void parse_ls(const char *p, struct branch *b) root->versions[1].mode = S_IFDIR; load_tree(root); } - if (*p == '"') { - static struct strbuf uq = STRBUF_INIT; - const char *endp; - strbuf_reset(&uq); - if (unquote_c_style(&uq, p, &endp)) - die("Invalid path: %s", command_buf.buf); - if (*endp) - die("Garbage after path in: %s", command_buf.buf); - p = uq.buf; - } + strbuf_reset(&uq); + parse_path_eol(&uq, p, "path"); + p = uq.buf; tree_content_get(root, p, &leaf, 1); /* * A directory in preparation would have a sha1 of zero diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 60e30fed3c..de2f1304e8 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2142,6 +2142,7 @@ test_expect_success 'Q: deny note on empty branch' ' EOF test_must_fail git fast-import has a special +# case. Test every occurrence of in the grammar against every error case. +# + +# +# Valid paths at the end of a line: filemodify, filedelete, filecopy (dest), +# filerename (dest), and ls. +# +# commit :301 from root -- modify hello.c (for setup) +# commit :302 from :301 -- modify $path +# commit :303 from :302 -- delete $path +# commit :304 from :301 -- copy hello.c $path +# commit :305 from :301 -- rename hello.c $path +# ls :305 $path +# +test_path_eol_success () { + local test="$1" path="$2" unquoted_path="$3" + test_expect_success "S: paths at EOL with $test must work" ' + test_when_finished "git branch -D S-path-eol" && + + git fast-import --export-marks=marks.out <<-EOF >out 2>err && + blob + mark :401 + data < $GIT_COMMITTER_DATE + data < $GIT_COMMITTER_DATE + data < $GIT_COMMITTER_DATE + data < $GIT_COMMITTER_DATE + data < $GIT_COMMITTER_DATE + data <tree_m.exp && + git ls-tree $commit_m | sort >tree_m.out && + test_cmp tree_m.exp tree_m.out && + + printf "100644 blob $blob1\thello.c\n" >tree_d.exp && + git ls-tree $commit_d >tree_d.out && + test_cmp tree_d.exp tree_d.out && + + ( + printf "100644 blob $blob1\t$unquoted_path\n" && + printf "100644 blob $blob1\thello.c\n" + ) | sort >tree_c.exp && + git ls-tree $commit_c | sort >tree_c.out && + test_cmp tree_c.exp tree_c.out && + + printf "100644 blob $blob1\t$unquoted_path\n" >tree_r.exp && + git ls-tree $commit_r >tree_r.out && + test_cmp tree_r.exp tree_r.out && + + test_cmp out tree_r.exp + ' +} + +test_path_eol_success 'quoted spaces' '" hello world.c "' ' hello world.c ' +test_path_eol_success 'unquoted spaces' ' hello world.c ' ' hello world.c ' + +# +# Valid paths before a space: filecopy (source) and filerename (source). +# +# commit :301 from root -- modify $path (for setup) +# commit :302 from :301 -- copy $path hello2.c +# commit :303 from :301 -- rename $path hello2.c +# +test_path_space_success () { + local test="$1" path="$2" unquoted_path="$3" + test_expect_success "S: paths before space with $test must work" ' + test_when_finished "git branch -D S-path-space" && + + git fast-import --export-marks=marks.out <<-EOF 2>err && + blob + mark :401 + data < $GIT_COMMITTER_DATE + data < $GIT_COMMITTER_DATE + data < $GIT_COMMITTER_DATE + data <tree_c.exp && + git ls-tree $commit_c | sort >tree_c.out && + test_cmp tree_c.exp tree_c.out && + + printf "100644 blob $blob\thello2.c\n" >tree_r.exp && + git ls-tree $commit_r >tree_r.out && + test_cmp tree_r.exp tree_r.out + ' +} + +test_path_space_success 'quoted spaces' '" hello world.c "' ' hello world.c ' +test_path_space_success 'no unquoted spaces' 'hello_world.c' 'hello_world.c' + +# +# Test a single commit change with an invalid path. Run it with all occurrences +# of in the grammar against all error kinds. +# +test_path_fail () { + local change="$1" what="$2" prefix="$3" path="$4" suffix="$5" err_grep="$6" + test_expect_success "S: $change with $what must fail" ' + test_must_fail git fast-import <<-EOF 2>err && + blob + mark :1 + data < $GIT_COMMITTER_DATE + data < $GIT_COMMITTER_DATE + data <, the must be quoted. +test_path_eol_quoted_fail 'ls (without dataref in commit)' 'ls ' path + ### ### series T (ls) ###