fast-import: tighten path unquoting
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Thalia Archibald committed
Apr 14, 2024 at 01:11 UTC
0df86b66899f9d6f1c09cceb4743c8cef733836a
2 files changed
+322
-44
builtin/fast-import.c
+65
-43
@@ -2258,10 +2258,60 @@ static uintmax_t parse_mark_ref_space(const char **p)
2258
return mark;
2259
}
2260
2261
+/*
2262
+ * Parse the path string into the strbuf. The path can either be quoted with
2263
+ * escape sequences or unquoted without escape sequences. Unquoted strings may
2264
+ * contain spaces only if `is_last_field` is nonzero; otherwise, it stops
2265
+ * parsing at the first space.
2266
+ */
2267
+static void parse_path(struct strbuf *sb, const char *p, const char **endp,
2268
+ int is_last_field, const char *field)
2269
+{
2270
+ if (*p == '"') {
2271
+ if (unquote_c_style(sb, p, endp))
2272
+ die("Invalid %s: %s", field, command_buf.buf);
2273
+ } else {
2274
+ /*
2275
+ * Unless we are parsing the last field of a line,
2276
+ * SP is the end of this field.
2277
+ */
2278
+ *endp = is_last_field
2279
+ ? p + strlen(p)
2280
+ : strchrnul(p, ' ');
2281
+ strbuf_add(sb, p, *endp - p);
2282
+ }
2283
+}
2284
+
2285
+/*
2286
+ * Parse the path string into the strbuf, and complain if this is not the end of
2287
+ * the string. Unquoted strings may contain spaces.
2288
+ */
2289
+static void parse_path_eol(struct strbuf *sb, const char *p, const char *field)
2290
+{
2291
+ const char *end;
2292
+
2293
+ parse_path(sb, p, &end, 1, field);
2294
+ if (*end)
2295
+ die("Garbage after %s: %s", field, command_buf.buf);
2296
+}
2297
+
2298
+/*
2299
+ * Parse the path string into the strbuf, and ensure it is followed by a space.
2300
+ * Unquoted strings may not contain spaces. Update *endp to point to the first
2301
+ * character after the space.
2302
+ */
2303
+static void parse_path_space(struct strbuf *sb, const char *p,
2304
+ const char **endp, const char *field)
2305
+{
2306
+ parse_path(sb, p, endp, 0, field);
2307
+ if (**endp != ' ')
2308
+ die("Missing space after %s: %s", field, command_buf.buf);
2309
+ (*endp)++;
2310
+}
2311
+
2312
static void file_change_m(const char *p, struct branch *b)
2313
{
2314
static struct strbuf uq = STRBUF_INIT;
2264
- const char *endp;
2315
struct object_entry *oe;
2316
struct object_id oid;
2317
uint16_t mode, inline_data = 0;
@@ -2299,11 +2349,8 @@ static void file_change_m(const char *p, struct branch *b)
2349
}
2350
2351
strbuf_reset(&uq);
2302
- if (!unquote_c_style(&uq, p, &endp)) {
2303
- if (*endp)
2304
- die("Garbage after path in: %s", command_buf.buf);
2305
- p = uq.buf;
2306
- }
2352
+ parse_path_eol(&uq, p, "path");
2353
+ p = uq.buf;
2354
2355
/* Git does not track empty, non-toplevel directories. */
2356
if (S_ISDIR(mode) && is_empty_tree_oid(&oid) && *p) {
@@ -2367,48 +2414,29 @@ static void file_change_m(const char *p, struct branch *b)
2414
static void file_change_d(const char *p, struct branch *b)
2415
{
2416
static struct strbuf uq = STRBUF_INIT;
2370
- const char *endp;
2417
2418
strbuf_reset(&uq);
2373
- if (!unquote_c_style(&uq, p, &endp)) {
2374
- if (*endp)
2375
- die("Garbage after path in: %s", command_buf.buf);
2376
- p = uq.buf;
2377
- }
2419
+ parse_path_eol(&uq, p, "path");
2420
+ p = uq.buf;
2421
tree_content_remove(&b->branch_tree, p, NULL, 1);
2422
}
2423
2381
-static void file_change_cr(const char *s, struct branch *b, int rename)
2424
+static void file_change_cr(const char *p, struct branch *b, int rename)
2425
{
2383
- const char *d;
2426
+ const char *s, *d;
2427
static struct strbuf s_uq = STRBUF_INIT;
2428
static struct strbuf d_uq = STRBUF_INIT;
2386
- const char *endp;
2429
struct tree_entry leaf;
2430
2431
strbuf_reset(&s_uq);
2390
- if (!unquote_c_style(&s_uq, s, &endp)) {
2391
- if (*endp != ' ')
2392
- die("Missing space after source: %s", command_buf.buf);
2393
- } else {
2394
- endp = strchr(s, ' ');
2395
- if (!endp)
2396
- die("Missing space after source: %s", command_buf.buf);
2397
- strbuf_add(&s_uq, s, endp - s);
2398
- }
2432
+ parse_path_space(&s_uq, p, &p, "source");
2433
s = s_uq.buf;
2434
2401
- endp++;
2402
- if (!*endp)
2435
+ if (!*p)
2436
die("Missing dest: %s", command_buf.buf);
2404
-
2405
- d = endp;
2437
strbuf_reset(&d_uq);
2407
- if (!unquote_c_style(&d_uq, d, &endp)) {
2408
- if (*endp)
2409
- die("Garbage after dest in: %s", command_buf.buf);
2410
- d = d_uq.buf;
2411
- }
2438
+ parse_path_eol(&d_uq, p, "dest");
2439
+ d = d_uq.buf;
2440
2441
memset(&leaf, 0, sizeof(leaf));
2442
if (rename)
@@ -3152,6 +3180,7 @@ static void print_ls(int mode, const unsigned char *hash, const char *path)
3180
3181
static void parse_ls(const char *p, struct branch *b)
3182
{
3183
+ static struct strbuf uq = STRBUF_INIT;
3184
struct tree_entry *root = NULL;
3185
struct tree_entry leaf = {NULL};
3186
@@ -3168,16 +3197,9 @@ static void parse_ls(const char *p, struct branch *b)
3197
root->versions[1].mode = S_IFDIR;
3198
load_tree(root);
3199
}
3171
- if (*p == '"') {
3172
- static struct strbuf uq = STRBUF_INIT;
3173
- const char *endp;
3174
- strbuf_reset(&uq);
3175
- if (unquote_c_style(&uq, p, &endp))
3176
- die("Invalid path: %s", command_buf.buf);
3177
- if (*endp)
3178
- die("Garbage after path in: %s", command_buf.buf);
3179
- p = uq.buf;
3180
- }
3200
+ strbuf_reset(&uq);
3201
+ parse_path_eol(&uq, p, "path");
3202
+ p = uq.buf;
3203
tree_content_get(root, p, &leaf, 1);
3204
/*
3205
* A directory in preparation would have a sha1 of zero
t/t9300-fast-import.sh
+257
-1
@@ -2142,6 +2142,7 @@ test_expect_success 'Q: deny note on empty branch' '
2142
EOF
2143
test_must_fail git fast-import <input
2144
'
2145
+
2146
###
2147
### series R (feature and option)
2148
###
@@ -2790,7 +2791,7 @@ test_expect_success 'R: blob appears only once' '
2791
'
2792
2793
###
2793
-### series S
2794
+### series S (mark and path parsing)
2795
###
2796
#
2797
# Make sure missing spaces and EOLs after mark references
@@ -3060,6 +3061,261 @@ test_expect_success 'S: ls with garbage after sha1 must fail' '
3061
test_grep "space after tree-ish" err
3062
'
3063
3064
+#
3065
+# Path parsing
3066
+#
3067
+# There are two sorts of ways a path can be parsed, depending on whether it is
3068
+# the last field on the line. Additionally, ls without a <dataref> has a special
3069
+# case. Test every occurrence of <path> in the grammar against every error case.
3070
+#
3071
+
3072
+#
3073
+# Valid paths at the end of a line: filemodify, filedelete, filecopy (dest),
3074
+# filerename (dest), and ls.
3075
+#
3076
+# commit :301 from root -- modify hello.c (for setup)
3077
+# commit :302 from :301 -- modify $path
3078
+# commit :303 from :302 -- delete $path
3079
+# commit :304 from :301 -- copy hello.c $path
3080
+# commit :305 from :301 -- rename hello.c $path
3081
+# ls :305 $path
3082
+#
3083
+test_path_eol_success () {
3084
+ local test="$1" path="$2" unquoted_path="$3"
3085
+ test_expect_success "S: paths at EOL with $test must work" '
3086
+ test_when_finished "git branch -D S-path-eol" &&
3087
+
3088
+ git fast-import --export-marks=marks.out <<-EOF >out 2>err &&
3089
+ blob
3090
+ mark :401
3091
+ data <<BLOB
3092
+ hello world
3093
+ BLOB
3094
+
3095
+ blob
3096
+ mark :402
3097
+ data <<BLOB
3098
+ hallo welt
3099
+ BLOB
3100
+
3101
+ commit refs/heads/S-path-eol
3102
+ mark :301
3103
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3104
+ data <<COMMIT
3105
+ initial commit
3106
+ COMMIT
3107
+ M 100644 :401 hello.c
3108
+
3109
+ commit refs/heads/S-path-eol
3110
+ mark :302
3111
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3112
+ data <<COMMIT
3113
+ commit filemodify
3114
+ COMMIT
3115
+ from :301
3116
+ M 100644 :402 $path
3117
+
3118
+ commit refs/heads/S-path-eol
3119
+ mark :303
3120
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3121
+ data <<COMMIT
3122
+ commit filedelete
3123
+ COMMIT
3124
+ from :302
3125
+ D $path
3126
+
3127
+ commit refs/heads/S-path-eol
3128
+ mark :304
3129
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3130
+ data <<COMMIT
3131
+ commit filecopy dest
3132
+ COMMIT
3133
+ from :301
3134
+ C hello.c $path
3135
+
3136
+ commit refs/heads/S-path-eol
3137
+ mark :305
3138
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3139
+ data <<COMMIT
3140
+ commit filerename dest
3141
+ COMMIT
3142
+ from :301
3143
+ R hello.c $path
3144
+
3145
+ ls :305 $path
3146
+ EOF
3147
+
3148
+ commit_m=$(grep :302 marks.out | cut -d\ -f2) &&
3149
+ commit_d=$(grep :303 marks.out | cut -d\ -f2) &&
3150
+ commit_c=$(grep :304 marks.out | cut -d\ -f2) &&
3151
+ commit_r=$(grep :305 marks.out | cut -d\ -f2) &&
3152
+ blob1=$(grep :401 marks.out | cut -d\ -f2) &&
3153
+ blob2=$(grep :402 marks.out | cut -d\ -f2) &&
3154
+
3155
+ (
3156
+ printf "100644 blob $blob2\t$unquoted_path\n" &&
3157
+ printf "100644 blob $blob1\thello.c\n"
3158
+ ) | sort >tree_m.exp &&
3159
+ git ls-tree $commit_m | sort >tree_m.out &&
3160
+ test_cmp tree_m.exp tree_m.out &&
3161
+
3162
+ printf "100644 blob $blob1\thello.c\n" >tree_d.exp &&
3163
+ git ls-tree $commit_d >tree_d.out &&
3164
+ test_cmp tree_d.exp tree_d.out &&
3165
+
3166
+ (
3167
+ printf "100644 blob $blob1\t$unquoted_path\n" &&
3168
+ printf "100644 blob $blob1\thello.c\n"
3169
+ ) | sort >tree_c.exp &&
3170
+ git ls-tree $commit_c | sort >tree_c.out &&
3171
+ test_cmp tree_c.exp tree_c.out &&
3172
+
3173
+ printf "100644 blob $blob1\t$unquoted_path\n" >tree_r.exp &&
3174
+ git ls-tree $commit_r >tree_r.out &&
3175
+ test_cmp tree_r.exp tree_r.out &&
3176
+
3177
+ test_cmp out tree_r.exp
3178
+ '
3179
+}
3180
+
3181
+test_path_eol_success 'quoted spaces' '" hello world.c "' ' hello world.c '
3182
+test_path_eol_success 'unquoted spaces' ' hello world.c ' ' hello world.c '
3183
+
3184
+#
3185
+# Valid paths before a space: filecopy (source) and filerename (source).
3186
+#
3187
+# commit :301 from root -- modify $path (for setup)
3188
+# commit :302 from :301 -- copy $path hello2.c
3189
+# commit :303 from :301 -- rename $path hello2.c
3190
+#
3191
+test_path_space_success () {
3192
+ local test="$1" path="$2" unquoted_path="$3"
3193
+ test_expect_success "S: paths before space with $test must work" '
3194
+ test_when_finished "git branch -D S-path-space" &&
3195
+
3196
+ git fast-import --export-marks=marks.out <<-EOF 2>err &&
3197
+ blob
3198
+ mark :401
3199
+ data <<BLOB
3200
+ hello world
3201
+ BLOB
3202
+
3203
+ commit refs/heads/S-path-space
3204
+ mark :301
3205
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3206
+ data <<COMMIT
3207
+ initial commit
3208
+ COMMIT
3209
+ M 100644 :401 $path
3210
+
3211
+ commit refs/heads/S-path-space
3212
+ mark :302
3213
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3214
+ data <<COMMIT
3215
+ commit filecopy source
3216
+ COMMIT
3217
+ from :301
3218
+ C $path hello2.c
3219
+
3220
+ commit refs/heads/S-path-space
3221
+ mark :303
3222
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3223
+ data <<COMMIT
3224
+ commit filerename source
3225
+ COMMIT
3226
+ from :301
3227
+ R $path hello2.c
3228
+
3229
+ EOF
3230
+
3231
+ commit_c=$(grep :302 marks.out | cut -d\ -f2) &&
3232
+ commit_r=$(grep :303 marks.out | cut -d\ -f2) &&
3233
+ blob=$(grep :401 marks.out | cut -d\ -f2) &&
3234
+
3235
+ (
3236
+ printf "100644 blob $blob\t$unquoted_path\n" &&
3237
+ printf "100644 blob $blob\thello2.c\n"
3238
+ ) | sort >tree_c.exp &&
3239
+ git ls-tree $commit_c | sort >tree_c.out &&
3240
+ test_cmp tree_c.exp tree_c.out &&
3241
+
3242
+ printf "100644 blob $blob\thello2.c\n" >tree_r.exp &&
3243
+ git ls-tree $commit_r >tree_r.out &&
3244
+ test_cmp tree_r.exp tree_r.out
3245
+ '
3246
+}
3247
+
3248
+test_path_space_success 'quoted spaces' '" hello world.c "' ' hello world.c '
3249
+test_path_space_success 'no unquoted spaces' 'hello_world.c' 'hello_world.c'
3250
+
3251
+#
3252
+# Test a single commit change with an invalid path. Run it with all occurrences
3253
+# of <path> in the grammar against all error kinds.
3254
+#
3255
+test_path_fail () {
3256
+ local change="$1" what="$2" prefix="$3" path="$4" suffix="$5" err_grep="$6"
3257
+ test_expect_success "S: $change with $what must fail" '
3258
+ test_must_fail git fast-import <<-EOF 2>err &&
3259
+ blob
3260
+ mark :1
3261
+ data <<BLOB
3262
+ hello world
3263
+ BLOB
3264
+
3265
+ commit refs/heads/S-path-fail
3266
+ mark :2
3267
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3268
+ data <<COMMIT
3269
+ commit setup
3270
+ COMMIT
3271
+ M 100644 :1 hello.c
3272
+
3273
+ commit refs/heads/S-path-fail
3274
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
3275
+ data <<COMMIT
3276
+ commit with bad path
3277
+ COMMIT
3278
+ from :2
3279
+ $prefix$path$suffix
3280
+ EOF
3281
+
3282
+ test_grep "$err_grep" err
3283
+ '
3284
+}
3285
+
3286
+test_path_base_fail () {
3287
+ local change="$1" prefix="$2" field="$3" suffix="$4"
3288
+ test_path_fail "$change" 'unclosed " in '"$field" "$prefix" '"hello.c' "$suffix" "Invalid $field"
3289
+ test_path_fail "$change" "invalid escape in quoted $field" "$prefix" '"hello\xff"' "$suffix" "Invalid $field"
3290
+}
3291
+test_path_eol_quoted_fail () {
3292
+ local change="$1" prefix="$2" field="$3"
3293
+ test_path_base_fail "$change" "$prefix" "$field" ''
3294
+ test_path_fail "$change" "garbage after quoted $field" "$prefix" '"hello.c"' 'x' "Garbage after $field"
3295
+ test_path_fail "$change" "space after quoted $field" "$prefix" '"hello.c"' ' ' "Garbage after $field"
3296
+}
3297
+test_path_eol_fail () {
3298
+ local change="$1" prefix="$2" field="$3"
3299
+ test_path_eol_quoted_fail "$change" "$prefix" "$field"
3300
+}
3301
+test_path_space_fail () {
3302
+ local change="$1" prefix="$2" field="$3"
3303
+ test_path_base_fail "$change" "$prefix" "$field" ' world.c'
3304
+ test_path_fail "$change" "missing space after quoted $field" "$prefix" '"hello.c"' 'x world.c' "Missing space after $field"
3305
+ test_path_fail "$change" "missing space after unquoted $field" "$prefix" 'hello.c' '' "Missing space after $field"
3306
+}
3307
+
3308
+test_path_eol_fail filemodify 'M 100644 :1 ' path
3309
+test_path_eol_fail filedelete 'D ' path
3310
+test_path_space_fail filecopy 'C ' source
3311
+test_path_eol_fail filecopy 'C hello.c ' dest
3312
+test_path_space_fail filerename 'R ' source
3313
+test_path_eol_fail filerename 'R hello.c ' dest
3314
+test_path_eol_fail 'ls (in commit)' 'ls :2 ' path
3315
+
3316
+# When 'ls' has no <dataref>, the <path> must be quoted.
3317
+test_path_eol_quoted_fail 'ls (without dataref in commit)' 'ls ' path
3318
+
3319
###
3320
### series T (ls)
3321
###