Wikimedia Bugzilla is closed!

Change 137095 had a related patch set uploaded by Reedy:
Suppress warnings from inet_pton

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

(In reply to Gerrit Notification Bot from comment #1)
> Change 137095 had a related patch set uploaded by Reedy:
> Suppress warnings from inet_pton
> 
> https://gerrit.wikimedia.org/r/137095

I note the code is commented about the emitting of the E_WARNING (code comments would want updating if 137095 was to be merged), but what use are they?

Re: patch 137095, I tend to concur with Aaron that we don't want wf* stuff in IPSet.php (which was placed in libs/ specifically with the goal that it might be generically reusable outside of MediaWiki).

We bounced around on this issue a bit during the initial review process (whether and how to suppress that warning).  There is a way to do the same basic thing directly (and only block E_WARNING to boot).  However, suppressing the warning adds some unfortunate overhead to the otherwise-fast match() method.  I don't remember how much, I'd have to go back and benchmark it again to see.  The other point of view is that it's the responsibility of the caller to sanitize the $ip argument as warranted if they don't want to get E_WARNINGs (in many cases the caller may already know a lot of constraints on $ip that make it easier, or unnecessary, to validate it explicitly).

It's really quite unfortunate that PHP's inet_pton() doesn't adhere to its officially-documented behavior of simply returning false in these cases and not issuing the warning in the first place.

The relevant code in PHP's ext/standard/basic_functions.c is:

	#ifdef HAVE_IPV6
		if (strchr(address, ':')) {
			af = AF_INET6;
		} else
	#endif
		if (!strchr(address, '.')) {
			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unrecognized address %s", address);
			RETURN_FALSE;
		}

		ret = inet_pton(af, address, buffer);

		if (ret <= 0) {
			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unrecognized address %s", address);
			RETURN_FALSE;
		}


We might be able to eliminate most (but not all, obviously) of the warnings by checking for [:.] in PHP.

Change 137095 abandoned by Reedy:
Suppress warnings from inet_pton

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

From Brandon on gerrit:

The non-wf way would be:
                $originalLevel = error_reporting( E_ALL & ~E_WARNING );
                $raw = inet_pton( $ip );
                error_reporting( $originalLevel );
I'd also tend think we'd want to leave the warning in addCidr() (since this is addresses from explicit admin config) and only suppress the noisy ones in match().
Or, as mentioned in the bug comments, could fix it in the caller instead and leave this alone.  It could be that the warnings are coming in via:
IP::isPublic( $ipchain[$i + 1] )
Around line ~1145 of includes/WebRequest.php.  Both that and the:
IP::isConfiguredProxy( $curIP )
just above it make use of IPSet::match under the hood, but $curIP (which is iterating $ipchain ...) has been sanitized before the call, where as $ipchain[$i + 1] has not.
(or it could be that the sanitize/canonicalize calls return null.  we could perhaps bail early with false in match() if the input is a literal null)