Distinguish preg_match() engine failure from "no match" in all dispatchers - #305
Open
dualfroz wants to merge 1 commit into
Open
Distinguish preg_match() engine failure from "no match" in all dispatchers#305dualfroz wants to merge 1 commit into
dualfroz wants to merge 1 commit into
Conversation
dualfroz
force-pushed
the
dualfroz/fix-preg-error-not-notfound
branch
from
September 5, 2026 22:57
af8bfe5 to
1f51800
Compare
…ting them as no-match All dispatcher strategies checked preg_match() results with `!== 1`, which conflates a genuine no-match (0) with a PCRE engine failure (false), e.g. PREG_BACKTRACK_LIMIT_ERROR from a catastrophically backtracking route pattern. Because dynamic routes for the same method are combined into shared regex chunks, one such route could make the preg_match() call for the whole chunk fail, silently turning an unrelated, cheap route in that chunk into a spurious NOT_FOUND. Add a shared matchRoute() helper on RegexBasedAbstract that calls preg_match() and throws a RuntimeException (including preg_last_error_msg()) when it returns false, while preserving the existing 0/1 no-match/match behavior. Update GroupCountBased, GroupPosBased, CharCountBased and MarkBased to use it. Fixes nikic#167
dualfroz
force-pushed
the
dualfroz/fix-preg-error-not-notfound
branch
from
September 5, 2026 23:25
1f51800 to
8bb83fe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (#167)
#167
All dispatcher strategies (
GroupCountBased,GroupPosBased,CharCountBased,MarkBased) callpreg_match()against a route's compiled regex and checkonly
!== 1to decide whether the route matched:preg_match()returnsfalse(not0) when the underlying PCRE enginefails - for example
PREG_BACKTRACK_LIMIT_ERRORwhen a catastrophicallybacktracking route pattern exceeds
pcre.backtrack_limit. The!== 1checktreats
falseexactly like0("no match"), so the engine error issilently swallowed and dispatch continues to the next route as if this one
simply didn't match.
Because
DataGenerator\RegexBasedAbstractcombines multiple dynamic routesfor the same HTTP method into shared alternation regex chunks (to keep the
number of
preg_match()calls low), a single catastrophically backtrackingroute can poison the whole chunk: when
preg_match()fails on the combinedregex, an unrelated, cheap route in the same chunk that would otherwise
have matched the request URI is spuriously treated as not found, and the
request can incorrectly fall through to a 404 (
NOT_FOUND).Root cause
src/Dispatcher/GroupCountBased.php:18src/Dispatcher/GroupPosBased.php:18src/Dispatcher/CharCountBased.php:21src/Dispatcher/MarkBased.php:17Each of these lines called
preg_match(...) !== 1directly, conflating"no match" (
0) with "engine failure" (false).Fix
Added a shared, error-checked helper to the common base class
src/Dispatcher/RegexBasedAbstract.php:All four dispatcher strategies now call
$this->matchRoute(...)instead ofpreg_match(...)directly. Normal behavior is unchanged:0(no match) and1(match) are returned exactly as before. Afalsereturn (enginefailure) now throws a
RuntimeExceptiondescribing the failing regex andpreg_last_error_msg(), instead of being silently treated as "no match".Empirical reproduction on unpatched HEAD
Using the exact route shape from issue #167 (a catastrophically backtracking
placeholder pattern combined, in the same regex chunk, with a cheap,
unrelated
/{p:a+}route), dispatching a 30-character URI that the cheaproute should match:
preg_last_error()confirms the PCRE engine failed(
PREG_BACKTRACK_LIMIT_ERROR), yet the dispatcher returnedNotMatchedfor a URI that the cheap, unrelated
/{p:a+}route matches on its own.This reproduces with the library's default
pcre.backtrack_limit(1,000,000) at 30 characters; a lowered limit (
ini_set('pcre.backtrack_limit', '1000'))was additionally used in the regression test to make the failure
deterministic and independent of the host's PCRE build/limits.
Regression test
Added
regexEngineFailureIsNotSilentlyTreatedAsNotFound()totest/Dispatcher/DispatcherTestCase.php, the shared base class extended byall four dispatcher test classes (
GroupCountBasedTest,GroupPosBasedTest,CharCountBasedTest,MarkBasedTest), so the fix is verified against allfour strategies.