Readd complete URL parsing fix from 3.18.7 release
Open, NormalPublic

Description

Noticed this when phpunit tests for mediawiki/core for hhvm on jessie started failing:

18:26:28 1) LinkFilterTest::testMakeLikeArrayWithValidPatterns with data set #39 ('', 'http://xx23124:__ffdfdef__@ww...45/dir', 'http://name:pass@www.test.com...]=4rtg')
18:26:28 LinkFilter::makeLikeArray('http://xx23124:__ffdfdef__@www.test.com:12345/dir', '') returned false on a valid pattern
18:26:28 Failed asserting that false is true.
18:26:28 
18:26:28 /home/jenkins/workspace/mediawiki-phpunit-hhvm-jessie/src/tests/phpunit/includes/LinkFilterTest.php:185
18:26:28 /home/jenkins/workspace/mediawiki-phpunit-hhvm-jessie/src/tests/phpunit/MediaWikiTestCase.php:421
18:26:28 /home/jenkins/workspace/mediawiki-phpunit-hhvm-jessie/src/maintenance/doMaintenance.php:94
18:26:28 
18:26:28 FAILURES!

Unit tests fail because parse_url("http://xx23124:__ffdfdef__@www.test.com:12345/dir") now returns false.

@Addshore pointed out https://github.com/facebook/hhvm/commit/80855dc1f2fe4d9de6bf4a4207ba88fbf7933b94

Reproducible with 3.18.5+dfsg-1+wmf3+deb9u1:

hhvm -d hhvm.jit=0 -v Eval.Jit=false tests/phpunit/phpunit.php --filter LinkFilterTest::testMakeLikeArrayWithValidPatterns
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 16 2018, 7:07 PM
Ottomata triaged this task as Normal priority.Jan 16 2018, 7:39 PM
Anomie added a subscriber: Anomie.EditedJan 16 2018, 9:40 PM

It looks like someone upstream misread RFC 3986 near zend-url.cpp line 167 in that change.

From the RFC (section 3.2.1 and appendix A):

userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )
unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"
sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
              / "*" / "+" / "," / ";" / "="

They got sub-delims and the extra : right (assuming they filter out " and # earlier), but they overlooked that unreserved has additional characters beyond what isalnum() checks.

hashar updated the task description. (Show Details)Jan 17 2018, 1:09 PM

Mentioned in SAL (#wikimedia-releng) [2018-01-17T13:10:59Z] <hashar> nodepool: updating snapshot to get hhvm +wmf4 for T185024 : nodepool image-update wmflabs-eqiad snapshot-ci-jessie

I've built/uploaded new HHVM packages for jessie (stretch following soon) which disable the broken patch and also reported this upstream at https://github.com/facebook/hhvm/issues/8104

Stretch packages have also been uploaded in the mean time.

Krinkle moved this task from Inbox to PHPUnit on the MediaWiki-Core-Tests board.
Krinkle raised the priority of this task from Normal to High.
Krinkle added a subscriber: Krinkle.

This is breaking the HHVM build on Travis.

https://travis-ci.org/wikimedia/mediawiki/jobs/329697385

There was 1 failure:
1) LinkFilterTest::testMakeLikeArrayWithValidPatterns with data set #39 ('', 'http://xx23124:__ffdfdef__@ww...45/dir', 'http://name:pass@www.test.com...]=4rtg')
LinkFilter::makeLikeArray('http://xx23124:__ffdfdef__@www.test.com:12345/dir', '') returned false on a valid pattern
Failed asserting that false is true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/LinkFilterTest.php:185
Krinkle raised the priority of this task from High to Unbreak Now!.Jan 23 2018, 1:36 AM
Restricted Application added subscribers: Liuxinyu970226, Jay8g, TerraCodes. · View Herald TranscriptJan 23 2018, 1:36 AM

Let's skip the test on broken HHVM versions? :/

For the HHVM builds on apt.wikimedia.org this has been fixed in 3.18.5+dfsg-1+wmf4 (jessie) and 3.18.5+dfsg-1+wmf4+deb9u1 (stretch). Does Travis use the deb packages provided by Facebook?

fred added a subscriber: fred.Jan 25 2018, 5:52 PM

https://gist.github.com/fredemmott/39c9abef4571f1e337d339fd8355da60 should resolve this, and will hopefully land to HHVM master, 3.21, and 3.24 in the next few days.

fred added a comment.Jan 25 2018, 5:58 PM

Also:

  • From Facebook's perspective, HHVM 3.18 is unsupported as of 2018-01-16; that said, I'm likely to make an exception for the a fix for this issue given how recently we introduced the regression, if backporting it isn't too involved.
  • Travis uses our packages, not the DFSG ones
  • We're not ready to announce this yet (want a little more testing first), but as of last week, we started packaging for Trusty again, and Travis now has 3.21 and 3.24 available: https://travis-ci.org/fredemmott/travistest/builds/333382730

Also:

  • From Facebook's perspective, HHVM 3.18 is unsupported as of 2018-01-16; that said, I'm likely to make an exception for the a fix for this issue given how recently we introduced the regression, if backporting it isn't too involved.

From my point of view there's no need for an exceptional 3.18 release, we roll our packages anyway and will backport your patch internally.

fred added a comment.EditedJan 25 2018, 6:24 PM

If it's practical, I'm going to do it even if it's not necessary for Wikipedia - breaking things 4 days before the end of support and not fixing them isn't ideal, and we have other reports of this issue.

A revised fix has been released (along with 3.18.8), I'll roll that into our packages: https://hhvm.com/blog/2018/01/30/hhvm-3.24.1.html

greg added a subscriber: greg.Feb 23 2018, 4:17 AM

A revised fix has been released (along with 3.18.8), I'll roll that into our packages: https://hhvm.com/blog/2018/01/30/hhvm-3.24.1.html

status? Just pinging since this is an UBN! without update for almost a month.

@greg Travis uses the debs packaged by Facebook and new releases have been made on the 30th: https://hhvm.com/blog/2018/01/30/hhvm-3.24.1.html

So I assume this is fixed, but better doublecheck current test runs before closing.

The Travis CI jobs for HHVM (3.18, 3.21, and 3.24) are all passing. This task can be closed once we're sure the upstream fix has also been backported/deployed on Wikimedia servers.

MoritzMuehlenhoff renamed this task from HHVM 3.18.5+dfsg-1+wmf3 changes parse_url causing unit tests to fail to Readd complete URL parsing fix from 3.18.7 release.Feb 26 2018, 11:19 AM
MoritzMuehlenhoff lowered the priority of this task from Unbreak Now! to Normal.