IPSet::__construct() in gets into infinite loop when called from curl on a CI host
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Tgr
	Feb 10 2016, 7:16 PM

Description

IP::isPublic() calls

new IPSet( array(
	'10.0.0.0/8', # RFC 1918 (private)
	'172.16.0.0/12', # RFC 1918 (private)
	'192.168.0.0/16', # RFC 1918 (private)
	'0.0.0.0/8', # this network
	'127.0.0.0/8', # loopback
	'fc00::/7', # RFC 4193 (local)
	'0:0:0:0:0:0:0:1', # loopback
	'169.254.0.0/16', # link-local
	'fe80::/10', # link-local
) );

which ends up in an infinite loop when Jenkins calls various things via curl for the qunit tests:
https://integration.wikimedia.org/ci/job/mediawiki-extensions-qunit/30149/console

P2585 infinite recursion on https://gerrit.wikimedia.org/r/#/c/268972/

1
2	( ! ) Fatal error: Maximum function nesting level of '100' reached, aborting! in /mnt/jenkins-workspace/workspace/mediawiki-extensions-qunit/src/vendor/wikimedia/ip-set/src/IPSet.php on line 219
3	Call Stack
4	#Time Function Location
5	1 {main}( )../index.php:0
6	2 require( '/mnt/jenkins-workspace/workspace/mediawiki-extensions-qunit/src/includes/WebStart.php' )../index.php:40
7	3 require_once( '/mnt/jenkins-workspace/workspace/mediawiki-extensions-qunit/src/includes/Setup.php' )../WebStart.php:152
8	4 MediaWiki\Session\SessionManager->checkIpLimits( )../Setup.php:814
9	5 IP::isPublic( )../SessionManager.php:1075
10	6 IPSet\IPSet->__construct( )../IP.php:391
11	7 IPSet\IPSet::recOptimize( )../IPSet.php:104
12	8 IPSet\IPSet::recOptimize( )../IPSet.php:220
13	9 IPSet\IPSet::recOptimize( )../IPSet.php:220
14	10 IPSet\IPSet::recOptimize( )../IPSet.php:220
15	11 IPSet\IPSet::recOptimize( )../IPSet.php:220
16	12 IPSet\IPSet::recOptimize( )../IPSet.php:220
17	13 IPSet\IPSet::recOptimize( )../IPSet.php:220
18	14 IPSet\IPSet::recOptimize( )../IPSet.php:220
19	15 IPSet\IPSet::recOptimize( )../IPSet.php:220
20	16 IPSet\IPSet::recOptimize( )../IPSet.php:220
21	17 IPSet\IPSet::recOptimize( )../IPSet.php:220
22	18 IPSet\IPSet::recOptimize( )../IPSet.php:220
23	19 IPSet\IPSet::recOptimize( )../IPSet.php:220
24	20 IPSet\IPSet::recOptimize( )../IPSet.php:220
25	21 IPSet\IPSet::recOptimize( )../IPSet.php:220
26	22 IPSet\IPSet::recOptimize( )../IPSet.php:220
27	23 IPSet\IPSet::recOptimize( )../IPSet.php:220
28	24 IPSet\IPSet::recOptimize( )../IPSet.php:220
29	25 IPSet\IPSet::recOptimize( )../IPSet.php:220
30	26 IPSet\IPSet::recOptimize( )../IPSet.php:220
31	27 IPSet\IPSet::recOptimize( )../IPSet.php:220
32	~ ~
33	~ ~
34	NORMAL  +  unix  13%   4:15
35	378 substitutions on 31 lines

The same call works fine in other contexts on the same machine, and via curl on my local machine. IPSet does not have configuration or any other kind of environmental dependencies.

Details

	Subject	Repo	Branch	Lines +/-
	Optimize and compress the tries as nodes are added	IPSet	master	+49 -109
	Use IP::isPublic() in SessionManager::checkIpLimits()	mediawiki/core	master	+1 -1

Customize query in gerrit

Related Objects
Search...

Status	Subtype	Assigned	Task
Resolved		• Deskana	T75616 Tracking: API/backend issues blocking Wikipedia app development
Resolved		Anomie	T32788 Allow triggering of user password reset email via the API
Open		None	T90925 General authentication improvements for MediaWiki
Resolved		Anomie	T48179 Allow a challenge stage during authentication
Open		None	T5709 Refactoring to make external authentication and identity systems easier
Resolved		Tgr	T43201 UserLoadFromSession considered evil
Resolved		Anomie	T67493 Session is started by EditAction (problem for extensions using UserLoadFromSession hook)
Open	Feature	None	T55156 Provide option to force a login session to end within a certain time
Open		None	T89459 Modernize MediaWiki authentication system (AuthManager)
Resolved		Anomie	T123451 Deploy SessionManager and bot passwords
Declined		None	T125455 Track ip addresses associated with a session and log when anomalous
Resolved		PleaseStand	T126495 IPSet::__construct() in gets into infinite loop when called from curl on a CI host
Resolved		Legoktm	T128167 Tag versions of IPSet in git
Resolved		Legoktm	T128169 Bump MediaWiki wikimedia/ip-set version to include fix for xdebug.max_nesting_level = 100

Event Timeline

Tgr created this task.Feb 10 2016, 7:16 PM

Tgr raised the priority of this task from to Needs Triage.

Tgr updated the task description. (Show Details)

Tgr added projects: Jenkins, mediawiki-libs-IPUtils.

Tgr subscribed.

Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptFeb 10 2016, 7:16 PM

Tgr added a parent task: T125455: Track ip addresses associated with a session and log when anomalous.Feb 10 2016, 7:17 PM

Change 269900 had a related patch set uploaded (by Gergő Tisza):
Use IP::isPublic() in SessionManager::checkIpLimits()

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

gerritbot added a project: Patch-For-Review.Feb 11 2016, 3:55 AM

Debug info in https://integration.wikimedia.org/ci/job/mediawiki-extensions-qunit/30404/artifact/log/mw-debug-www.log.
Same log on vagrant (where it works correctly):

ci.log6 MBDownload

Looking at the logs it seems that the bit tree used to store the address ranges is legitimately >100 levels deep so probably the recursion limits are somehow lower for CI web requests.

+ @BBlack who wrote the IPSet matching code.

No idea how the code work but an IPv6 is 128 bits long. Looking at the log output from Vagrant the array indeed has ~128 levels.

On both Precise and Trusty instances the Zend PHP has xdebug.max_nesting_level => 100 => 100. Beeem test fail.

So something in your code triggers a nasty nested array which apparently is not covered by IPSet tests, or IPSet would have failed tests running on CI with xdebug.max_nesting_level=100.

Would be nice to gather more evidence. The parameter being passed to IP::isPublic() would help maybe. It should really be 127.0.0.1 though.

Yes, the code requires >100 levels of recursion.

https://github.com/wikimedia/IPSet/blob/master/tests/IPSetTest.php#L35 is where the unit test workaround is at, it probably needs updating (as do the settings for runtime, wherever IPSet is used).

In T126495#2018694, @hashar wrote:

So something in your code triggers a nasty nested array which apparently is not covered by IPSet tests, or IPSet would have failed tests running on CI with xdebug.max_nesting_level=100.

I note that phpunit.php also sets xdebug.max_nesting_level higher for the phpunit tests, and the phpunit tests in Gergő's patch are passing. The failing tests here are qunit, not phpunit.

Presumably the existing qunit tests just aren't sending any requests with an X-Forwarded-For header (which seems to be the only other thing using IP::isPublic()) or hitting anything else that constructs an IPSet with IPv6 addresses. Gergő's addition here calls IP::isPublic() on every request, so every qunit test is going to hit it.

Would be nice to gather more evidence. The parameter being passed to IP::isPublic() would help maybe. It should really be 127.0.0.1 though.

Since it's happening in the IPSet constructor (which doesn't get passed the $ip) rather than the call to match(), that's unlikely to matter.

So we need the local webhost used by QUnit to have max_nesting_level.

The apache vhost is setup via modules/contint/manifests/localvhost.pp. So we can bump the xdebug value either:

A) directly in the Apache vhost by editing modules/contint/templates/apache/localvhost.erb

B) Add a new file under /etc/php5/apache2/conf.d/

hashar edited projects, added Continuous-Integration-Infrastructure; removed Jenkins.Feb 11 2016, 4:30 PM

hashar set Security to None.

Maybe large IPSet rules should be precompiled? 100+ levels of recursion sounds expensive...

It's only ~128 levels, and it's not that expensive really. And any pre-compilation of course still happens under the same interpreter with the same constraint that the recursion-level setting needs appropriate adjustment.

More to the point, though, the constructor is effectively the pre-compilation step. The intent is you construct an IPSet object infrequently (e.g. when a process starts, or on a configuration change), and then just invoke the object's match method for heavy runtime traffic.

meta-note: it would be best if IPSet's documentation made a note that it requires the interpreter/engine config change for recursion limits...

Circling back around to this and looking again: of course it's possible that a better answer here is to refactor recOptimize and recCompress to use loops in place of recursion. In theory, recursive things can always be rewritten as loops that maintain some kind of state-stack variable, e.g. an array stack that grows by one entry for each virtual level of recursion in the loop.

The upside is it could remove the constraint that using this module requires setting a special interpreter/engine config variable in the common case. The primary potential downside is that the code might get more-unreadable (and it's already pretty bad). The other potential fallout is of course the perf impact of that refactor is an unknown, but I'd expect perf improvement, and in the constructor case perf isn't as important as it is in match()

In T126495#2019792, @BBlack wrote:

Circling back around to this and looking again: of course it's possible that a better answer here is to refactor recOptimize and recCompress to use loops in place of recursion. In theory, recursive things can always be rewritten as loops that maintain some kind of state-stack variable, e.g. an array stack that grows by one entry for each virtual level of recursion in the loop.

The upside is it could remove the constraint that using this module requires setting a special interpreter/engine config variable in the common case. The primary potential downside is that the code might get more-unreadable (and it's already pretty bad). The other potential fallout is of course the perf impact of that refactor is an unknown, but I'd expect perf improvement, and in the constructor case perf isn't as important as it is in match()

Soon after IPSet was added to core, I worked on a patch that avoids the recursion, though never uploaded it to Gerrit. Actually, I had multiple versions of the patch that took different approaches. I'll see if I can update one of them for the current version of IPSet.

Change 270301 had a related patch set uploaded (by PleaseStand):
Optimize and compress the tries as nodes are added

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

Change 269900 abandoned by Gergő Tisza:
Use IP::isPublic() in SessionManager::checkIpLimits()

Reason:
checkIpLimits was removed.

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

Change 270301 merged by jenkins-bot:
Optimize and compress the tries as nodes are added

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

My understand is the patch @PleaseStand provided on https://gerrit.wikimedia.org/r/#/c/270301/ and approved by @BBlack make it so the level of recursion has been dramatically lowered and that would make it work for folks having the default xdebug setting of 100 levels. That is frankly quite awesome. Thank you all !

I would like a new version of IPSet to be cut and MediaWiki core to included before 1.27. I am filling sub tasks.

hashar created subtask T128167: Tag versions of IPSet in git.Feb 26 2016, 9:50 AM