Skip to content

Commit 1f2e05f

Browse files
phillipwoodgitster
authored andcommitted
wildmatch: fix exponential behavior
When dowild() cannot match a '*' or '/**/' wildcard then it must return WM_ABORT_TO_STARSTAR or WM_ABORT_ALL respectively. Failure to observe this results in unnecessary backtracking and the time taken for a failed match increases exponentially with the number of wildcards in the pattern [1]. Unfortunately in some instances dowild() returns WM_NOMATCH for a failed match resulting in long match times for patterns containing multiple wildcards as can be seen in the following benchmark. (Note that the timings in the Benchmark 1 are really measuring the time to execute test-tool rather than the time to match the pattern) Benchmark 1: t/helper/test-tool wildmatch wildmatch aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab "*a" Time (mean ± σ): 22.8 ms ± 1.7 ms [User: 12.1 ms, System: 10.6 ms] Range (min … max): 19.4 ms … 26.9 ms 113 runs Warning: Ignoring non-zero exit code. Benchmark 2: t/helper/test-tool wildmatch wildmatch aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab "*a*a*a*a*a*a*a*a*a" Time (mean ± σ): 5.244 s ± 0.228 s [User: 5.229 s, System: 0.010 s] Range (min … max): 4.969 s … 5.707 s 10 runs Warning: Ignoring non-zero exit code. Summary 't/helper/test-tool wildmatch wildmatch aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab "*a"' ran 230.37 ± 20.04 times faster than 't/helper/test-tool wildmatch wildmatch aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab "*a*a*a*a*a*a*a*a*a"' The security implications are limited as it only affects operations that are potentially DoS vectors. For example by creating a blob containing such a pattern a malicious user can exploit this behavior to use large amounts of CPU time on a remote server by pushing the blob and then creating a new clone with --filter=sparse:oid. However this filter type is usually disabled as it is known to consume large amounts of CPU time even without this bug. The WM_MATCH changed in the first hunk of this patch comes from the original implementation imported from rsync in 5230f60 (Import wildmatch from rsync, 2012-10-15). Compared to the others converted here it is fairly harmless as it only triggers at the end of the pattern and so will only cause a single unnecessary backtrack. The others introduced by 6f1a31f (wildmatch: advance faster in <asterisk> + <literal> patterns, 2013-01-01) and 4698344 (wildmatch: make a special case for "*/" with FNM_PATHNAME, 2013-01-01) are more pernicious and will cause exponential behavior. A new test is added to protect against future regressions. [1] https://research.swtch.com/glob Helped-by: Derrick Stolee <[email protected]> Signed-off-by: Phillip Wood <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 768bb23 commit 1f2e05f

File tree

2 files changed

+17
-4
lines changed

2 files changed

+17
-4
lines changed

t/t3070-wildmatch.sh

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,4 +431,13 @@ match 1 1 1 1 'a' '[B-a]'
431431
match 0 1 0 1 'z' '[Z-y]'
432432
match 1 1 1 1 'Z' '[Z-y]'
433433

434+
test_expect_success 'matching does not exhibit exponential behavior' '
435+
test-tool wildmatch wildmatch \
436+
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab \
437+
"*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a" &
438+
pid=$! &&
439+
sleep 2 &&
440+
! kill $!
441+
'
442+
434443
test_done

wildmatch.c

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
114114
* only if there are no more slash characters. */
115115
if (!match_slash) {
116116
if (strchr((char *)text, '/'))
117-
return WM_NOMATCH;
117+
return WM_ABORT_TO_STARSTAR;
118118
}
119119
return WM_MATCH;
120120
} else if (!match_slash && *p == '/') {
@@ -125,7 +125,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
125125
*/
126126
const char *slash = strchr((char*)text, '/');
127127
if (!slash)
128-
return WM_NOMATCH;
128+
return WM_ABORT_ALL;
129129
text = (const uchar*)slash;
130130
/* the slash is consumed by the top-level for loop */
131131
break;
@@ -153,8 +153,12 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
153153
break;
154154
text++;
155155
}
156-
if (t_ch != p_ch)
157-
return WM_NOMATCH;
156+
if (t_ch != p_ch) {
157+
if (match_slash)
158+
return WM_ABORT_ALL;
159+
else
160+
return WM_ABORT_TO_STARSTAR;
161+
}
158162
}
159163
if ((matched = dowild(p, text, flags)) != WM_NOMATCH) {
160164
if (!match_slash || matched != WM_ABORT_TO_STARSTAR)

0 commit comments

Comments
 (0)