| Message ID | e790bdf714bb4c2a67708c016a97cf4f9e79ac48.1711960552.git.thalia@archibald.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fast-import: tighten parsing of paths | expand |
On Mon, Apr 01, 2024 at 09:02:47AM +0000, Thalia Archibald wrote: > Path parsing in fast-import is inconsistent and many unquoting errors > are suppressed or not checked. > > <path> appears in the grammar in these places: > > filemodify ::= 'M' SP <mode> (<dataref> | 'inline') SP <path> LF > filedelete ::= 'D' SP <path> LF > filecopy ::= 'C' SP <path> SP <path> LF > filerename ::= 'R' SP <path> SP <path> LF > ls ::= 'ls' SP <dataref> SP <path> LF > ls-commit ::= 'ls' SP <path> LF > > and fast-import.c parses them in five different ways: > > 1. For filemodify and filedelete: > Try to unquote <path>. 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 <path>. 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 <path> 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 <path> 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 <path> 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 <path> 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 <path> in filecopy and rename > cannot be an unquoted empty string (this will be addressed subsequently) > and <path> in ls-commit must be quoted to disambiguate it from ls. > > Signed-off-by: Thalia Archibald <thalia@archibald.dev> > --- > builtin/fast-import.c | 102 ++++++++++------- > t/t9300-fast-import.sh | 251 ++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 309 insertions(+), 44 deletions(-) > > diff --git a/builtin/fast-import.c b/builtin/fast-import.c > index 782bda007c..6f9048a037 100644 > --- a/builtin/fast-import.c > +++ b/builtin/fast-import.c > @@ -2258,10 +2258,54 @@ 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) Let's break this overly long line, for example after `**endp,`. > +{ > + 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 = strchr(p, ' '); > + strbuf_add(sb, p, *endp - p); strchr(3P) may return a NULL pointer in case there is no space, which would make us segfault here when dereferencing `*endp`. We should probably add a testcase that would hit this edge case. > + } > +} > + > +/* > + * 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 +2343,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 +2408,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); So this statement right now doesn't make a whole lot of sense because `p` cannot ever be `NULL` -- we'd segfault before that. Once we update `parse_path()` to handle this correctly it will work as expected though. I was briefly wondering though whether we really want `parse_path()` to set `p` to be a NULL pointer. If we didn't, we could retain the previous behaviour here and instead check for `!*p`. Patrick > - 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 +3174,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 +3191,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..0fb5612b07 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 <input > ' > + > ### > ### series R (feature and option) > ### > @@ -2790,7 +2791,7 @@ test_expect_success 'R: blob appears only once' ' > ' > > ### > -### series S > +### series S (mark and path parsing) > ### > # > # Make sure missing spaces and EOLs after mark references > @@ -3060,6 +3061,254 @@ test_expect_success 'S: ls with garbage after sha1 must fail' ' > test_grep "space after tree-ish" err > ' > > +# > +# Path parsing > +# > +# There are two sorts of ways a path can be parsed, depending on whether it is > +# the last field on the line. Additionally, ls without a <dataref> has a special > +# case. Test every occurrence of <path> 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 <<BLOB > + hello world > + BLOB > + > + blob > + mark :402 > + data <<BLOB > + hallo welt > + BLOB > + > + commit refs/heads/S-path-eol > + mark :301 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + initial commit > + COMMIT > + M 100644 :401 hello.c > + > + commit refs/heads/S-path-eol > + mark :302 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + commit filemodify > + COMMIT > + from :301 > + M 100644 :402 '"$path"' > + > + commit refs/heads/S-path-eol > + mark :303 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + commit filedelete > + COMMIT > + from :302 > + D '"$path"' > + > + commit refs/heads/S-path-eol > + mark :304 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + commit filecopy dest > + COMMIT > + from :301 > + C hello.c '"$path"' > + > + commit refs/heads/S-path-eol > + mark :305 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + commit filerename dest > + COMMIT > + from :301 > + R hello.c '"$path"' > + > + ls :305 '"$path"' > + EOF > + > + commit_m=$(grep :302 marks.out | cut -d\ -f2) && > + commit_d=$(grep :303 marks.out | cut -d\ -f2) && > + commit_c=$(grep :304 marks.out | cut -d\ -f2) && > + commit_r=$(grep :305 marks.out | cut -d\ -f2) && > + blob1=$(grep :401 marks.out | cut -d\ -f2) && > + blob2=$(grep :402 marks.out | cut -d\ -f2) && > + > + ( printf "100644 blob $blob2\t'"$unquoted_path"'\n" && > + printf "100644 blob $blob1\thello.c\n" ) | sort >tree_m.exp && I think it is more customary to format as follows: ( printf "100644 blob $blob2\t'"$unquoted_path"'\n" && printf "100644 blob $blob1\thello.c\n" ) | sort >tree_m.exp && Same for other statements further down. Also, there is no need to do `'"$unuoted_path"'` here. You should be able to refer to `$unquoted_path` just fine even without unquoting again because we use eval to execute the code block. In fact, it can even be harmful as it is known to break shells under some circumstances. See also 7c4449eb31 (t/README: document how to loop around test cases, 2024-03-22), which I think should apply in your case, too. Patrick > + 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 <<BLOB > + hello world > + BLOB > + > + commit refs/heads/S-path-space > + mark :301 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + initial commit > + COMMIT > + M 100644 :401 '"$path"' > + > + commit refs/heads/S-path-space > + mark :302 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + commit filecopy source > + COMMIT > + from :301 > + C '"$path"' hello2.c > + > + commit refs/heads/S-path-space > + mark :303 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + commit filerename source > + COMMIT > + from :301 > + R '"$path"' hello2.c > + > + EOF > + > + commit_c=$(grep :302 marks.out | cut -d\ -f2) && > + commit_r=$(grep :303 marks.out | cut -d\ -f2) && > + blob=$(grep :401 marks.out | cut -d\ -f2) && > + > + ( printf "100644 blob $blob\t'"$unquoted_path"'\n" && > + printf "100644 blob $blob\thello2.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 $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 <path> 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 <<BLOB > + hello world > + BLOB > + > + commit refs/heads/S-path-fail > + mark :2 > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + commit setup > + COMMIT > + M 100644 :1 hello.c > + > + commit refs/heads/S-path-fail > + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE > + data <<COMMIT > + commit with bad path > + COMMIT > + from :2 > + '"$prefix$path$suffix"' > + EOF > + > + test_grep '"'$err_grep'"' err > + ' > +} > + > +test_path_base_fail () { > + local change="$1" prefix="$2" field="$3" suffix="$4" > + test_path_fail "$change" 'unclosed " in '"$field" "$prefix" '"hello.c' "$suffix" "Invalid $field" > + test_path_fail "$change" "invalid escape in quoted $field" "$prefix" '"hello\xff"' "$suffix" "Invalid $field" > +} > +test_path_eol_quoted_fail () { > + local change="$1" prefix="$2" field="$3" suffix="$4" > + test_path_base_fail "$change" "$prefix" "$field" "$suffix" > + test_path_fail "$change" "garbage after quoted $field" "$prefix" '"hello.c"x' "$suffix" "Garbage after $field" > + test_path_fail "$change" "space after quoted $field" "$prefix" '"hello.c" ' "$suffix" "Garbage after $field" > +} > +test_path_eol_fail () { > + local change="$1" prefix="$2" field="$3" suffix="$4" > + test_path_eol_quoted_fail "$change" "$prefix" "$field" "$suffix" > +} > +test_path_space_fail () { > + local change="$1" prefix="$2" field="$3" suffix="$4" > + test_path_base_fail "$change" "$prefix" "$field" "$suffix" > + test_path_fail "$change" "missing space after quoted $field" "$prefix" '"hello.c"x' "$suffix" "Missing space after $field" > +} > + > +test_path_eol_fail filemodify 'M 100644 :1 ' path '' > +test_path_eol_fail filedelete 'D ' path '' > +test_path_space_fail filecopy 'C ' source ' world.c' > +test_path_eol_fail filecopy 'C hello.c ' dest '' > +test_path_space_fail filerename 'R ' source ' world.c' > +test_path_eol_fail filerename 'R hello.c ' dest '' > +test_path_eol_fail 'ls (in commit)' 'ls :2 ' path '' > + > +# When 'ls' has no <dataref>, the <path> must be quoted. > +test_path_eol_quoted_fail 'ls (without dataref in commit)' 'ls ' path '' > + > ### > ### series T (ls) > ### > -- > 2.44.0 >
On Tue, Apr 9, 2024 at 11:30 PM Patrick Steinhardt <ps@pks.im> wrote: > > + if (include_spaces) > > + *endp = p + strlen(p); > > + else > > + *endp = strchr(p, ' '); > > + strbuf_add(sb, p, *endp - p); > > strchr(3P) may return a NULL pointer in case there is no space, which > would make us segfault here when dereferencing `*endp`. We should > probably add a testcase that would hit this edge case. Note that you can do: *endp = p + strcspn(p, " "); (though `strcspn` is a fundamentally harder operation since it takes a string argument). Everything depends on whether you want to test for an explicit "there was no space at all" case of course; performance considerations are secondary. Chris
On Apr 10, 2024, at 01:18, Chris Torek <chris.torek@gmail.com> wrote: > On Tue, Apr 9, 2024 at 11:30 PM Patrick Steinhardt <ps@pks.im> wrote: >>> + if (include_spaces) >>> + *endp = p + strlen(p); >>> + else >>> + *endp = strchr(p, ' '); >>> + strbuf_add(sb, p, *endp - p); >> >> strchr(3P) may return a NULL pointer in case there is no space, which >> would make us segfault here when dereferencing `*endp`. We should >> probably add a testcase that would hit this edge case. > > Note that you can do: > > *endp = p + strcspn(p, " "); > > (though `strcspn` is a fundamentally harder operation since it > takes a string argument). Everything depends on whether you > want to test for an explicit "there was no space at all" case of > course; performance considerations are secondary. I thought strchr returned a pointer to the terminating NUL byte if the needle was not found. Turns out it does return NULL in that case, as you say. strchrnul does what I want here and I’ve replaced it with that. I’ve added a test covering this case. Thalia
On Wed, Apr 10, 2024 at 1:47 AM Thalia Archibald <thalia@archibald.dev> wrote:
> strchrnul does what I want here and I’ve replaced it with that.
`strchrnul` is a GNU extension (found on a lot of systems, but not
part of C90 or C99).
Chris
(Sorry for re-sending) On Apr 9, 2024, at 23:27, Patrick Steinhardt <ps@pks.im> wrote: > On Mon, Apr 01, 2024 at 09:02:47AM +0000, Thalia Archibald wrote: >> >> - if (!*endp) >> + if (!p) >> die("Missing dest: %s", command_buf.buf); > > So this statement right now doesn't make a whole lot of sense because > `p` cannot ever be `NULL` -- we'd segfault before that. Once we update > `parse_path()` to handle this correctly it will work as expected though. > > I was briefly wondering though whether we really want `parse_path()` to > set `p` to be a NULL pointer. If we didn't, we could retain the previous > behaviour here and instead check for `!*p`. Good catch. There should be a deref there. This mistake was because I originally planned to not allow unquoted empty strings and had factored that condition into parse_path. After your round 1 feedback, I changed my mind after reanalysis. The condition you see here is supposed to match the behavior for before and is removed in patch 3/8. There was no test before my series exercising this branch and my test for it is added in 3/8, so it wasn't caught in this intermediate version. >> + ( printf "100644 blob $blob2\t'"$unquoted_path"'\n" && >> + printf "100644 blob $blob1\thello.c\n" ) | sort >tree_m.exp && > > Also, there is no need to do `'"$unuoted_path"'` here. You should be > able to refer to `$unquoted_path` just fine even without unquoting again > because we use eval to execute the code block. In fact, it can even be > harmful as it is known to break shells under some circumstances. See > also 7c4449eb31 (t/README: document how to loop around test cases, > 2024-03-22), which I think should apply in your case, too. I agree it makes it less finicky. The one upside to string splicing is that when a test fails, the substitutions are visible in the dump of the shell script. I found that useful while debugging. The titles can uniquely identify the $prefix/$path/$suffix values when looking in the source, since they're all 1-to-1. Considering the downsides, I've switched to plain substitutions. Thalia
On Apr 10, 2024, at 01:51, Chris Torek <chris.torek@gmail.com> wrote: > On Wed, Apr 10, 2024 at 1:47 AM Thalia Archibald <thalia@archibald.dev> wrote: >> strchrnul does what I want here and I’ve replaced it with that. > > `strchrnul` is a GNU extension (found on a lot of systems, but not > part of C90 or C99). I can’t speak to Git standards, but it seems broadly used in Git, including three times already in fast-import: $ rg --count-matches --sort=path strchrnul add-patch.c:1 advice.c:1 apply.c:2 archive.c:1 attr.c:1 builtin/am.c:1 builtin/fast-export.c:5 builtin/fast-import.c:4 builtin/stash.c:1 cache-tree.c:2 commit.c:5 compat/mingw.c:1 compat/terminal.c:1 config.c:1 diff.c:1 fmt-merge-msg.c:2 fsck.c:2 git-compat-util.h:3 git.c:1 gpg-interface.c:8 graph.c:1 help.c:1 http.c:2 ident.c:2 log-tree.c:1 mailmap.c:1 match-trees.c:1 notes.c:1 object-file.c:2 parse-options.c:2 path.c:1 pretty.c:2 ref-filter.c:5 refs/debug.c:1 remote-curl.c:1 remote.c:1 run-command.c:1 scalar.c:1 sequencer.c:5 strbuf.c:1 trailer.c:1 transport-helper.c:1 utf8.c:1 ws.c:1 wt-status.c:1 Thalia
On Apr 10, 2024, at 02:14, Thalia Archibald <thalia@archibald.dev> wrote: > On Apr 10, 2024, at 01:51, Chris Torek <chris.torek@gmail.com> wrote: >> On Wed, Apr 10, 2024 at 1:47 AM Thalia Archibald <thalia@archibald.dev> wrote: >>> strchrnul does what I want here and I’ve replaced it with that. >> >> `strchrnul` is a GNU extension (found on a lot of systems, but not >> part of C90 or C99). > > I can’t speak to Git standards, but it seems broadly used in Git, including > three times already in fast-import. … and that would be because it is supplied when unavailable: git-compat-util.h #ifndef HAVE_STRCHRNUL #define strchrnul gitstrchrnul static inline char *gitstrchrnul(const char *s, int c) { while (*s && *s != c) s++; return (char *)s; } #endif Thalia
On Wed, Apr 10, 2024 at 09:14:16AM +0000, Thalia Archibald wrote: > On Apr 10, 2024, at 01:51, Chris Torek <chris.torek@gmail.com> wrote: > > On Wed, Apr 10, 2024 at 1:47 AM Thalia Archibald <thalia@archibald.dev> wrote: > >> strchrnul does what I want here and I’ve replaced it with that. > > > > `strchrnul` is a GNU extension (found on a lot of systems, but not > > part of C90 or C99). > > I can’t speak to Git standards, but it seems broadly used in Git, including > three times already in fast-import: It's fine to use `strchrnul()` in Git. In case libc doesn't provide it we have a fallback implementation in "git-compat-util.h". Patrick
diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 782bda007c..6f9048a037 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -2258,10 +2258,54 @@ 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 = strchr(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 +2343,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 +2408,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 +3174,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 +3191,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..0fb5612b07 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 <input ' + ### ### series R (feature and option) ### @@ -2790,7 +2791,7 @@ test_expect_success 'R: blob appears only once' ' ' ### -### series S +### series S (mark and path parsing) ### # # Make sure missing spaces and EOLs after mark references @@ -3060,6 +3061,254 @@ test_expect_success 'S: ls with garbage after sha1 must fail' ' test_grep "space after tree-ish" err ' +# +# Path parsing +# +# There are two sorts of ways a path can be parsed, depending on whether it is +# the last field on the line. Additionally, ls without a <dataref> has a special +# case. Test every occurrence of <path> 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 <<BLOB + hello world + BLOB + + blob + mark :402 + data <<BLOB + hallo welt + BLOB + + commit refs/heads/S-path-eol + mark :301 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + initial commit + COMMIT + M 100644 :401 hello.c + + commit refs/heads/S-path-eol + mark :302 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + commit filemodify + COMMIT + from :301 + M 100644 :402 '"$path"' + + commit refs/heads/S-path-eol + mark :303 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + commit filedelete + COMMIT + from :302 + D '"$path"' + + commit refs/heads/S-path-eol + mark :304 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + commit filecopy dest + COMMIT + from :301 + C hello.c '"$path"' + + commit refs/heads/S-path-eol + mark :305 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + commit filerename dest + COMMIT + from :301 + R hello.c '"$path"' + + ls :305 '"$path"' + EOF + + commit_m=$(grep :302 marks.out | cut -d\ -f2) && + commit_d=$(grep :303 marks.out | cut -d\ -f2) && + commit_c=$(grep :304 marks.out | cut -d\ -f2) && + commit_r=$(grep :305 marks.out | cut -d\ -f2) && + blob1=$(grep :401 marks.out | cut -d\ -f2) && + blob2=$(grep :402 marks.out | cut -d\ -f2) && + + ( printf "100644 blob $blob2\t'"$unquoted_path"'\n" && + printf "100644 blob $blob1\thello.c\n" ) | sort >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 <<BLOB + hello world + BLOB + + commit refs/heads/S-path-space + mark :301 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + initial commit + COMMIT + M 100644 :401 '"$path"' + + commit refs/heads/S-path-space + mark :302 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + commit filecopy source + COMMIT + from :301 + C '"$path"' hello2.c + + commit refs/heads/S-path-space + mark :303 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + commit filerename source + COMMIT + from :301 + R '"$path"' hello2.c + + EOF + + commit_c=$(grep :302 marks.out | cut -d\ -f2) && + commit_r=$(grep :303 marks.out | cut -d\ -f2) && + blob=$(grep :401 marks.out | cut -d\ -f2) && + + ( printf "100644 blob $blob\t'"$unquoted_path"'\n" && + printf "100644 blob $blob\thello2.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 $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 <path> 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 <<BLOB + hello world + BLOB + + commit refs/heads/S-path-fail + mark :2 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + commit setup + COMMIT + M 100644 :1 hello.c + + commit refs/heads/S-path-fail + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + commit with bad path + COMMIT + from :2 + '"$prefix$path$suffix"' + EOF + + test_grep '"'$err_grep'"' err + ' +} + +test_path_base_fail () { + local change="$1" prefix="$2" field="$3" suffix="$4" + test_path_fail "$change" 'unclosed " in '"$field" "$prefix" '"hello.c' "$suffix" "Invalid $field" + test_path_fail "$change" "invalid escape in quoted $field" "$prefix" '"hello\xff"' "$suffix" "Invalid $field" +} +test_path_eol_quoted_fail () { + local change="$1" prefix="$2" field="$3" suffix="$4" + test_path_base_fail "$change" "$prefix" "$field" "$suffix" + test_path_fail "$change" "garbage after quoted $field" "$prefix" '"hello.c"x' "$suffix" "Garbage after $field" + test_path_fail "$change" "space after quoted $field" "$prefix" '"hello.c" ' "$suffix" "Garbage after $field" +} +test_path_eol_fail () { + local change="$1" prefix="$2" field="$3" suffix="$4" + test_path_eol_quoted_fail "$change" "$prefix" "$field" "$suffix" +} +test_path_space_fail () { + local change="$1" prefix="$2" field="$3" suffix="$4" + test_path_base_fail "$change" "$prefix" "$field" "$suffix" + test_path_fail "$change" "missing space after quoted $field" "$prefix" '"hello.c"x' "$suffix" "Missing space after $field" +} + +test_path_eol_fail filemodify 'M 100644 :1 ' path '' +test_path_eol_fail filedelete 'D ' path '' +test_path_space_fail filecopy 'C ' source ' world.c' +test_path_eol_fail filecopy 'C hello.c ' dest '' +test_path_space_fail filerename 'R ' source ' world.c' +test_path_eol_fail filerename 'R hello.c ' dest '' +test_path_eol_fail 'ls (in commit)' 'ls :2 ' path '' + +# When 'ls' has no <dataref>, the <path> must be quoted. +test_path_eol_quoted_fail 'ls (without dataref in commit)' 'ls ' path '' + ### ### series T (ls) ###
Path parsing in fast-import is inconsistent and many unquoting errors are suppressed or not checked. <path> appears in the grammar in these places: filemodify ::= 'M' SP <mode> (<dataref> | 'inline') SP <path> LF filedelete ::= 'D' SP <path> LF filecopy ::= 'C' SP <path> SP <path> LF filerename ::= 'R' SP <path> SP <path> LF ls ::= 'ls' SP <dataref> SP <path> LF ls-commit ::= 'ls' SP <path> LF and fast-import.c parses them in five different ways: 1. For filemodify and filedelete: Try to unquote <path>. 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 <path>. 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 <path> 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 <path> 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 <path> 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 <path> 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 <path> in filecopy and rename cannot be an unquoted empty string (this will be addressed subsequently) and <path> in ls-commit must be quoted to disambiguate it from ls. Signed-off-by: Thalia Archibald <thalia@archibald.dev> --- builtin/fast-import.c | 102 ++++++++++------- t/t9300-fast-import.sh | 251 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 309 insertions(+), 44 deletions(-)