Page MenuHomePhabricator

Wrong substitution in cache file wikis.php
Closed, ResolvedPublic

Description

Good morning Sebastien,

I just discovered a nasty bug within the generation of the wiki cache file wikis.php leading to a race condition and lots of PHP warnings logged.

In [line 395 of src/MediaWikiFarm.php](https://phabricator.wikimedia.org/diffusion/EMWF/browse/master/src/MediaWikiFarm.php;c615f63030673e7b2457693dbe88d1f5de8aa572$395) is the following code:

if( !array_key_exists( $host, $hosts ) || !preg_match( $hosts[$host], $path . '/' ) ) {
    $path = preg_quote( $path, '/' );
    $path = ( array_key_exists( $host, $hosts ) ? substr( $hosts[$host], 2, -4 ) . '|' : '' ) . $path;
    $hosts[$host] = '/^(' . $path . ')\//';
    MediaWikiFarmUtils::cacheFile( $hosts, 'wikis.php', $this->cacheDir );
 }

This tries to match pathes to a given regex cached in this file:

<?php
return array (
  'wiki.ipp.kfa-juelich.de' => '/^(\\/test)\\//',
);

Actually, this only works for one wiki. If a second (or more) wikis are added, the following happens:

<?php
return array (
  'wiki.ipp.kfa-juelich.de' => '/^((\\/test|\\/test2)\\//',
);

This is no regular expression anymore and thus any further checks fail, which leads to a race condition, resulting in

<?php
return array (
  'wiki.ipp.kfa-juelich.de' => '/^((((\\/test|\\/test2|\\/test|\\/test2|\\/test|\\/test2)\\//',
);

This is continued and lets the string grow until the regex is to big for PHP and the preg_match fails due to complexity.

It is easy to fix:

// use index 3 instead of 2 for substr() to exclude "(", too...
$path = ( array_key_exists( $host, $hosts ) ? substr( $hosts[$host], 3, -4 ) . '|' : '' ) . $path;

Should I commit a patch or do you want to fix it yourself (maybe faster than me setting up Gerrit...)?

Thanks in advance!
Cheers
Oliver

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 10 2018, 10:00 AM
Seb35 added a comment.Jan 19 2018, 9:05 PM

I remarked this bug also, thanks for opening the task. If you have a patch for it, you can submit it on Gerrit, I will review it. Else I will take some time in one or two weeks.

Setting up Gerrit requires a bit of time initially, but it becomes quite quick after. The tutorial is there. In a very condensed form if you know already well Git: 1/ you have to create an account on https://wikitech.wikimedia.org, 2/ log in on Gerrit and set up your SSH key, 3/ install Gerrit (git-review) on your computer, 4/ git clone the repository and execute "git review -s" to intialize Gerrit inside, 5/ "git commit" your changes and "git review -R" to push them on Gerrit, 6/ "git commit --amend" + "git review -R" to push a latter version of the same patch.

Change 409447 had a related patch set uploaded (by Seb35; owner: Seb35):
[mediawiki/extensions/MediaWikiFarm@master] Bug in path-based farms

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

Change 409447 merged by jenkins-bot:
[mediawiki/extensions/MediaWikiFarm@master] Bug in path-based farms

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

Seb35 closed this task as Resolved.Feb 9 2018, 9:21 PM
Seb35 removed a project: Patch-For-Review.
238482n375 set Security to Software security bug.Jun 15 2018, 8:07 AM
238482n375 changed the visibility from "Public (No Login Required)" to "Custom Policy".
This comment was removed by Dzahn.
Restricted Application added a project: acl*security. · View Herald TranscriptJun 15 2018, 2:24 PM
Aklapper assigned this task to Seb35.Jun 15 2018, 2:29 PM
Aklapper changed the visibility from "Custom Policy" to "Public (No Login Required)".
Dzahn added a subscriber: Dzahn.Jun 15 2018, 3:10 PM
Dzahn removed a subscriber: Dzahn.