Change 650565 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Avoid 'finally' clause in AbuseFilterParser::parseDetailed

https://gerrit.wikimedia.org/r/650565

In T270514#6702331, @Daimona wrote:

I am not very sure, but I think the only way for this error to happen is that the request times out during the parse() call, then the 'finally' block is executed, and $res is unset.

If that's the case, then I don't think removing finally clause will fix it, since you'll still use the unset $res to call ParserStatus::__construct()

The root cause of the issue, I believe, is that AbuseFilterParser::parse() can return bool or null, but this is not documented.

/**
 * @param string $code
 * @return bool
 */
public function parse( $code ) {
    return $this->intEval( $code )->toBool();
}

It's documented to return bool only, but that's not quite accurate.

If we follow through AFPData::toBool(), we can see that it's ultimately returning a prop which is documented to be almost everything

/**
 * @var mixed|null|AFPData[] The actual data contained in this object
 * @private Use $this->getData()
 */
public $data;

Then we have this ( called from the parse() call above)

public function intEval( $code ) {
...
 $result = new AFPData( AFPData::DEMPTY, ); // <-- Creating AFPData without second argument will initialize  $data to null
 $this->doLevelEntry( $result );
 if ( $result->getType() === AFPData::DUNDEFINED ) {
	$result = new AFPData( AFPData::DBOOL, false ); 
 $this->statsd->timing( 'abusefilter_oldparser_full', microtime( true ) - $startTime );
 return $result;
 }

In T270514#6703166, @Ammarpad wrote:

In T270514#6702331, @Daimona wrote:

I am not very sure, but I think the only way for this error to happen is that the request times out during the parse() call, then the 'finally' block is executed, and $res is unset.

If that's the case, then I don't think removing finally clause will fix it, since you'll still use the unset $res to call ParserStatus::__construct()

This is wrong. 'finally' blocks are guaranteed to be executed out of the ordinary flow, and the 'finally' block here is executed even if the time limit is reached while parsing. If we remove the 'finally' block, and the time limit is reached while parsing, the exception will bubble up without reaching the return line. Also, the 'finally' is not necessary right now, because every "acceptable" exception is covered by the 'catch' clause, and any unwanted exception should bubble up.

I can now actually come up with a working example:

function throwRuntime() {
    throw new \RuntimeException;
}

function main() {
    try {
        $res = throwRuntime();
    } catch ( \LogicException $e ) {
        $res = false;
    } finally {
        return $res;
    }
}

a();

What you'll get is

Warning: Undefined variable $res in /in/qbmaF on line 13

And this is probably what's happening here, except that the exception being thrown is WMFTimeoutException (or whatever it's called), and the catch clause looks for AFPException. Removing the finally block will cause the exception to bubble up as expected.

The root cause of the issue, I believe, is that AbuseFilterParser::parse() can return bool or null, but this is not documented.

/**
 * @param string $code
 * @return bool
 */
public function parse( $code ) {
    return $this->intEval( $code )->toBool();
}

This is also incorrect, toBool() casts the internal type to bool, as the name suggests.

If we follow through AFPData::toBool(), we can see that it's ultimately returning a prop which is documented to be almost everything

/**
 * @var mixed|null|AFPData[] The actual data contained in this object
 * @private Use $this->getData()
 */
public $data;

That property can hold any data type because AFPData are generic value objects used to represent a value within the AF tokenizer/parser/evaluator. If you carefully analyze what AFPData::castTypes does, you'll note that if the target type is bool, then it will return a boolean (same for other types).

Then we have this ( called from the parse() call above)

public function intEval( $code ) {
...
 $result = new AFPData( AFPData::DEMPTY, ); // <-- Creating AFPData without second argument will initialize  $data to null
 $this->doLevelEntry( $result );
 if ( $result->getType() === AFPData::DUNDEFINED ) {
	$result = new AFPData( AFPData::DBOOL, false ); 
 $this->statsd->timing( 'abusefilter_oldparser_full', microtime( true ) - $startTime );
 return $result;
 }

Correct, but it doesn't prove your point. That AFPData is passed through the parser levels, which will replace the $result with the actual result. Even if this would result in a DNULL/DEMPTY being returned, the toBool call will cast it to a boolean.

Change 650565 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Avoid 'finally' clause in AbuseFilterParser::parseDetailed

https://gerrit.wikimedia.org/r/650565