From 0d1bc5760e53eb88078db0f45e32d13134c46b26 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Mon, 17 Feb 2025 14:17:19 +1100 Subject: language: Simplify MessageParser::getParser() calling convention Instead of getParser(), which is documented as requiring the caller to manage relevant state, add acquireParser() and releaseParser(). Add test. Change-Id: I112a0b6a1ff2e1dfaed7fe8e011872235d126f46 --- includes/language/MessageParser.php | 93 ++++++++++++++-------- .../includes/language/MessageParserTest.php | 28 ++++++- 2 files changed, 83 insertions(+), 38 deletions(-) diff --git a/includes/language/MessageParser.php b/includes/language/MessageParser.php index 82bd682fd96b..8f595fbe8f23 100644 --- a/includes/language/MessageParser.php +++ b/includes/language/MessageParser.php @@ -21,6 +21,9 @@ use Psr\Log\LoggerInterface; * @since 1.44 */ class MessageParser { + private const DEPTH_EXCEEDED_MESSAGE = + 'Message parse depth limit exceeded'; + private ParserFactory $parserFactory; private OutputTransformPipeline $outputPipeline; private LanguageFactory $langFactory; @@ -29,8 +32,9 @@ class MessageParser { /** @var ParserOptions|null Lazy-initialised */ private ?ParserOptions $parserOptions = null; - /** @var Parser[] Lazy-created via self::getParser() */ + /** @var Parser[] Cached Parser objects */ private array $parsers = []; + /** @var int Index into $this->parsers for the active Parser */ private int $curParser = -1; /** @@ -109,37 +113,18 @@ class MessageParser { } $page ??= $this->getPlaceholderTitle(); + $parser = $this->acquireParser(); + if ( !$parser ) { + return self::DEPTH_EXCEEDED_MESSAGE; + } try { - $this->curParser++; - $parser = $this->getParser(); - if ( !$parser ) { - return 'Message transform depth limit exceeded'; - } - $message = $parser->transformMsg( $message, $popts, $page ); + return $parser->transformMsg( $message, $popts, $page ); } finally { - $this->curParser--; - } - if ( $oldUserLang ) { - $popts->setUserLang( $oldUserLang ); - } - - return $message; - } - - /** - * You should increment $this->curParser before calling this method and decrement it after - * to support recursive calls to message parsing. - */ - private function getParser(): ?Parser { - if ( $this->curParser >= self::MAX_PARSER_DEPTH ) { - $this->logger->debug( __METHOD__ . ": Refusing to create a new parser with index {$this->curParser}" ); - return null; - } - if ( !isset( $this->parsers[ $this->curParser ] ) ) { - $this->logger->debug( __METHOD__ . ": Creating a new parser with index {$this->curParser}" ); - $this->parsers[ $this->curParser ] = $this->parserFactory->create(); + $this->releaseParser( $parser ); + if ( $oldUserLang ) { + $popts->setUserLang( $oldUserLang ); + } } - return $this->parsers[ $this->curParser ]; } /** @@ -198,15 +183,14 @@ class MessageParser { $page ??= $this->getPlaceholderTitle(); + $parser = $this->acquireParser(); + if ( !$parser ) { + return new ParserOutput( self::DEPTH_EXCEEDED_MESSAGE ); + } try { - $this->curParser++; - $parser = $this->getParser(); - if ( !$parser ) { - return new ParserOutput( 'Message parse depth limit exceeded' ); - } return $parser->parse( $text, $page, $popts, $lineStart ); } finally { - $this->curParser--; + $this->releaseParser( $parser ); } } @@ -217,4 +201,43 @@ class MessageParser { WikiAwareEntity::LOCAL ); } + + /** + * Attempt to get a free parser from the cache. If none exists, create one, + * up to a limit of MAX_PARSER_DEPTH. If the limit is exceeded, return null. + * + * If a parser is returned, it must be released with releaseParser(). + * + * @return Parser|null + */ + private function acquireParser(): ?Parser { + $index = $this->curParser + 1; + if ( $index >= self::MAX_PARSER_DEPTH ) { + $this->logger->debug( __METHOD__ . ": Refusing to create a new parser with index {$index}" ); + return null; + } + $parser = $this->parsers[ $index ] ?? null; + if ( !$parser ) { + $this->logger->debug( __METHOD__ . ": Creating a new parser with index {$index}" ); + $parser = $this->parserFactory->create(); + } + $this->parsers[ $index ] = $parser; + $this->curParser = $index; + return $parser; + } + + /** + * Release a parser previously acquired by acquireParser(). + * + * @param Parser $parser + */ + private function releaseParser( Parser $parser ) { + if ( $this->parsers[$this->curParser] !== $parser ) { + throw new \LogicException( 'releaseParser called with the wrong ' . + "parser instance: #{$this->curParser} = " . + gettype( $this->parsers[$this->curParser] ) ); + } + $this->curParser--; + } + } diff --git a/tests/phpunit/includes/language/MessageParserTest.php b/tests/phpunit/includes/language/MessageParserTest.php index eeb980598e0d..41436ca6ff67 100644 --- a/tests/phpunit/includes/language/MessageParserTest.php +++ b/tests/phpunit/includes/language/MessageParserTest.php @@ -12,7 +12,7 @@ class MessageParserTest extends MediaWikiIntegrationTestCase { public function testNestedMessageParse() { $msgOuter = ( new RawMessage( '[[Link|{{#language:}}]]' ) ) ->inLanguage( 'outer' ) - ->page( new PageIdentityValue( 1, NS_MAIN, 'Link', PageIdentityValue::LOCAL ) ); + ->page( $this->makePage( 'Link' ) ); // T372891: Allow nested message parsing // Any hook from Linker or LinkRenderer will do for this test, but this one is the simplest @@ -24,6 +24,28 @@ class MessageParserTest extends MediaWikiIntegrationTestCase { $this->assertEquals( 'outerinner', $msgOuter->parse() ); } + public function testNestedLimit() { + $text = '[[Link|label]]'; + $messageParser = $this->getServiceContainer()->getMessageParser(); + $this->setTemporaryHook( + 'SelfLinkBegin', + function ( $nt, &$html, &$trail, &$prefix, &$ret ) use ( $messageParser ) { + $html .= $messageParser + ->parse( '[[Link|label]]', $this->makePage( 'Link' ), true, true, 'en' ) + ->getContentHolderText(); + } ); + $result = $messageParser + ->parse( $text, $this->makePage( 'Link' ), true, true, 'en' ) + ->getContentHolderText(); + $this->assertStringContainsString( 'Message parse depth limit exceeded', $result ); + + // Check that the MessageParser is still functional after hitting the limit + $result = $messageParser + ->parse( 'test', null, true, true, 'en' ) + ->getContentHolderText(); + $this->assertSame( 'test', Parser::stripOuterParagraph( $result ) ); + } + public static function provideTransform() { return [ [ @@ -62,7 +84,7 @@ class MessageParserTest extends MediaWikiIntegrationTestCase { $result = $messageParser->transform( $input, $options['interface'] ?? true, - $options['lang'] ?? null, + $options['lang'] ?? 'en', $this->makePage( $options['page'] ?? null ) ); $this->assertSame( $expected, $result ); @@ -112,7 +134,7 @@ class MessageParserTest extends MediaWikiIntegrationTestCase { $this->makePage( $options['page'] ?? null ), $options['lineStart'] ?? true, $options['interface'] ?? true, - $options['lang'] ?? null + $options['lang'] ?? 'en' ); $result = Parser::stripOuterParagraph( $parserOutput->getContentHolderText() ); $this->assertSame( $expected, $result ); -- cgit v1.2.3