From 5a81ad0e8a698cb3e9c6b513b2003b198d5c00a1 Mon Sep 17 00:00:00 2001
From: Brian Wolff <bawolff+wn@gmail.com>
Date: Sun, 27 Oct 2013 17:18:06 -0300
Subject: [PATCH] Throw an error if calling parser recursively

People accidentally (or sometimes intentionally) calling the
parser recursively has been a major source of bugs over the
years. I think its much better to fail suddenly, instead
of having unclear signs like UNIQ's all over the place.

Change-Id: I0e42aa69835c15a5df7aecb0dc5c3dec946bdf6a
---
 RELEASE-NOTES-1.24                            |  1 +
 includes/parser/Parser.php                    | 43 +++++++++++++++++++
 .../includes/parser/ParserMethodsTest.php     | 20 +++++++++
 3 files changed, 64 insertions(+)

diff --git a/RELEASE-NOTES-1.24 b/RELEASE-NOTES-1.24
index 189c5f1fd2..09aeb89086 100644
--- a/RELEASE-NOTES-1.24
+++ b/RELEASE-NOTES-1.24
@@ -30,6 +30,7 @@ production.
   which used to collapse some sidebar elements by default.
 * (bug 890) Links in Special:RecentChanges and Special:Watchlist no longer
   follow redirects to their target pages.
+* Parser now dies early if called recursively, instead of producing subtle bugs.
 
 === Web API changes in 1.24 ===
 * action=parse API now supports prop=modules, which provides the list of
diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php
index 7423006788..35b1b1722c 100644
--- a/includes/parser/Parser.php
+++ b/includes/parser/Parser.php
@@ -211,6 +211,12 @@ class Parser {
 	 */
 	var $mLangLinkLanguages;
 
+	/**
+	 * @var boolean Recursive call protection.
+	 * This variable should be treated as if it were private.
+	 */
+	public $mInParse = false;
+
 	/**
 	 * Constructor
 	 *
@@ -254,6 +260,7 @@ class Parser {
 	 * Allow extensions to clean up when the parser is cloned
 	 */
 	function __clone() {
+		$this->mInParse = false;
 		wfRunHooks( 'ParserCloned', array( $this ) );
 	}
 
@@ -364,6 +371,10 @@ class Parser {
 		wfProfileIn( __METHOD__ );
 		wfProfileIn( $fname );
 
+		if ( $clearState ) {
+			$magicScopeVariable = $this->lock();
+		}
+
 		$this->startParse( $title, $options, self::OT_HTML, $clearState );
 
 		$this->mInputSize = strlen( $text );
@@ -615,6 +626,7 @@ class Parser {
 	 */
 	function preprocess( $text, Title $title = null, ParserOptions $options, $revid = null ) {
 		wfProfileIn( __METHOD__ );
+		$magicScopeVariable = $this->lock();
 		$this->startParse( $title, $options, self::OT_PREPROCESS, true );
 		if ( $revid !== null ) {
 			$this->mRevisionId = $revid;
@@ -662,6 +674,7 @@ class Parser {
 		$text = $msg->params( $params )->plain();
 
 		# Parser (re)initialisation
+		$magicScopeVariable = $this->lock();
 		$this->startParse( $title, $options, self::OT_PLAIN, true );
 
 		$flags = PPFrame::NO_ARGS | PPFrame::NO_TEMPLATES;
@@ -4593,6 +4606,9 @@ class Parser {
 	 * @return string The altered wiki markup
 	 */
 	public function preSaveTransform( $text, Title $title, User $user, ParserOptions $options, $clearState = true ) {
+		if ( $clearState ) {
+			$magicScopeVariable = $this->lock();
+		}
 		$this->startParse( $title, $options, self::OT_WIKI, $clearState );
 		$this->setUser( $user );
 
@@ -4767,6 +4783,7 @@ class Parser {
 	public function cleanSig( $text, $parsing = false ) {
 		if ( !$parsing ) {
 			global $wgTitle;
+			$magicScopeVariable = $this->lock();
 			$this->startParse( $wgTitle, new ParserOptions, self::OT_PREPROCESS, true );
 		}
 
@@ -5627,6 +5644,8 @@ class Parser {
 	 */
 	private function extractSections( $text, $section, $mode, $newText = '' ) {
 		global $wgTitle; # not generally used but removes an ugly failure mode
+
+		$magicScopeVariable = $this->lock();
 		$this->startParse( $wgTitle, new ParserOptions, self::OT_PLAIN, true );
 		$outText = '';
 		$frame = $this->getPreprocessor()->newFrame();
@@ -5972,6 +5991,7 @@ class Parser {
 	 * @return string
 	 */
 	function testSrvus( $text, Title $title, ParserOptions $options, $outputType = self::OT_HTML ) {
+		$magicScopeVariable = $this->lock();
 		$this->startParse( $title, $options, $outputType, true );
 
 		$text = $this->replaceVariables( $text );
@@ -6149,4 +6169,27 @@ class Parser {
 		}
 		return $parsedWidthParam;
 	}
+
+	/**
+	 * Lock the current instance of the parser.
+	 *
+	 * This is meant to stop someone from calling the parser
+	 * recursively and messing up all the strip state.
+	 *
+	 * @throws MWException If parser is in a parse
+	 * @return ScopedCallback The lock will be released once the return value goes out of scope.
+	 */
+	protected function lock() {
+		if ( $this->mInParse ) {
+			throw new MWException( "Parser state cleared while parsing. Did you call Parser::parse recursively?" );
+		}
+		$this->mInParse = true;
+
+		$that = $this;
+		$recursiveCheck = new ScopedCallback( function() use ( $that ) {
+			$that->mInParse = false;
+		} );
+
+		return $recursiveCheck;
+	}
 }
diff --git a/tests/phpunit/includes/parser/ParserMethodsTest.php b/tests/phpunit/includes/parser/ParserMethodsTest.php
index 229eeb44bc..4ea8fc6c19 100644
--- a/tests/phpunit/includes/parser/ParserMethodsTest.php
+++ b/tests/phpunit/includes/parser/ParserMethodsTest.php
@@ -29,6 +29,26 @@ class ParserMethodsTest extends MediaWikiLangTestCase {
 		$this->assertEquals( $expected, $text );
 	}
 
+	/**
+	 * @expectedException MWException
+	 * @expectedExceptionMessage Parser state cleared while parsing. Did you call Parser::parse recursively?
+	 * @covers Parser::lock
+	 */
+	public function testRecursiveParse() {
+		global $wgParser;
+		$title = Title::newFromText( 'foo' );
+		$po = new ParserOptions;
+		$wgParser->setHook( 'recursivecallparser', array( $this, 'helperParserFunc' ) );
+		$wgParser->parse( '<recursivecallparser>baz</recursivecallparser>', $title, $po );
+	}
+
+	public function helperParserFunc( $input, $args, $parser) {
+		$title = Title::newFromText( 'foo' );
+		$po = new ParserOptions;
+		$parser->parse( $input, $title, $po );
+		return 'bar';
+	}
+
 	/**
 	 * @covers Parser::callParserFunction
 	 */
-- 
2.20.1