False Positive: Function argument(s) returned by "func_get_args" might have been modified

[reedy]

This seems to be rather a lot of false positives returned by this...

File: /home/reedy/mediawiki/img_auth.php
> Line 192: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

https://phabricator.wikimedia.org/source/mediawiki/browse/master/img_auth.php;1b0c9f6098c31d6bf16a00d37a8aa5cd493270e1$192

The only statements before this is global $wgImgAuthDetails; -- this isn't any modification

We've seen a lot of these false positives in many functions when scanning MediaWiki and extensions deployed on Wikimedia sites

I can give you some more examples if you want to see if it's possible to improve the detection

[sstalle]

You are right, it just checks if a certain statement/call occurs before func_get_args in a function, so it's very imprecise. I have some ideas on how to improve this, and having real world examples would be very helpful to validate these ideas.

[reedy]

Cool. I'll go through the ones I've been given for MW/MW extensions, and try and get a list of "easy" to detect false positives, through to the more complex ones, but are still false +ve

Of course, a workaround for this, code wise, is just to stash what's returned by func_get_args into a variable as the first line in a function. Not great, but could be useful sometimes

Will get back to you in a bit

Thanks!

[reedy]

https://github.com/wikimedia/mediawiki

So from mediawiki core I reckon these below should be identifiable as false positives. Some are more complex than others, but some are trivially simple

Where variables that are manipulated aren't already named parameters in the function definition... Or noting related is touched

Hope these are useful to find some test cases :)

File: /home/reedy/mediawiki/img_auth.php
> Line 192: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/img_auth.php#L192


File: /home/reedy/mediawiki/tests/phpunit/maintenance/MaintenanceTest.php
> Line 82: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/tests/phpunit/maintenance/MaintenanceTest.php#L82


File: /home/reedy/mediawiki/tests/phpunit/includes/api/query/ApiQueryTestBase.php
> Line 43: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/tests/phpunit/includes/api/query/ApiQueryTestBase.php#L43


File: /home/reedy/mediawiki/includes/htmlform/HTMLForm.php
> Line 958: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();


    https://github.com/wikimedia/mediawiki/blob/master/includes/htmlform/HTMLForm.php#L958

File: /home/reedy/mediawiki/includes/api/ApiAuthManagerHelper.php
> Line 387: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/api/ApiAuthManagerHelper.php#L387


File: /home/reedy/mediawiki/includes/parser/CoreParserFunctions.php
> Line 98: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/parser/CoreParserFunctions.php#L98


File: /home/reedy/mediawiki/includes/filerepo/ForeignDBRepo.php
> Line 134: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/filerepo/ForeignDBRepo.php#L134


File: /home/reedy/mediawiki/includes/filerepo/ForeignDBViaLBRepo.php
> Line 93: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/filerepo/ForeignDBViaLBRepo.php#L93


File: /home/reedy/mediawiki/includes/libs/rdbms/database/DBConnRef.php
> Line 163: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/libs/rdbms/database/DBConnRef.php#L163


File: /home/reedy/mediawiki/includes/libs/rdbms/database/Database.php
> Line 3206: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/libs/rdbms/database/Database.php#L3206


File: /home/reedy/mediawiki/includes/libs/objectcache/MultiWriteBagOStuff.php
> Line 182: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

	https://github.com/wikimedia/mediawiki/blob/master/includes/libs/objectcache/MultiWriteBagOStuff.php#L182


File: /home/reedy/mediawiki/includes/media/MediaTransformOutput.php
> Line 307: [Warning] Function argument(s) returned by "func_get_arg" might have been modified
    func_get_arg(5);
> Line 309: [Warning] Function argument(s) returned by "func_get_arg" might have been modified
    func_get_arg(4);

    https://github.com/wikimedia/mediawiki/blob/master/includes/media/MediaTransformOutput.php#L307
    https://github.com/wikimedia/mediawiki/blob/master/includes/media/MediaTransformOutput.php#L309


File: /home/reedy/mediawiki/includes/specialpage/SpecialPage.php
> Line 749: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/specialpage/SpecialPage.php#L749


File: /home/reedy/mediawiki/includes/GlobalFunctions.php
> Line 1420: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();
> Line 3003: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();
> Line 3041: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/GlobalFunctions.php#L1420
    https://github.com/wikimedia/mediawiki/blob/master/includes/GlobalFunctions.php#L3003
    https://github.com/wikimedia/mediawiki/blob/master/includes/GlobalFunctions.php#L3041


File: /home/reedy/mediawiki/includes/exception/MWExceptionHandler.php
> Line 156: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/exception/MWExceptionHandler.php#L156


File: /home/reedy/mediawiki/includes/skins/BaseTemplate.php
> Line 36: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/skins/BaseTemplate.php#L36


File: /home/reedy/mediawiki/includes/SiteConfiguration.php
> Line 587: [Warning] Function argument(s) returned by "func_get_arg" might have been modified
    func_get_arg($i);

    https://github.com/wikimedia/mediawiki/blob/master/includes/SiteConfiguration.php#L587


File: /home/reedy/mediawiki/includes/specials/SpecialEditWatchlist.php
> Line 103: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/specials/SpecialEditWatchlist.php#L103


File: /home/reedy/mediawiki/includes/specials/SpecialImport.php
> Line 580: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

    https://github.com/wikimedia/mediawiki/blob/master/includes/specials/SpecialImport.php#L580

[sstalle]

Thank you for the examples. I think I've fixed most of the issues in 7bd1d16, except for

File: /home/reedy/mediawiki/includes/GlobalFunctions.php
> Line 1420: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

Fixing that requires reflecting on user-defined classes without loading them, which php7cc currently doesn't do. I'll think about integrating a library like roave/better-reflection for that, but it won't happen, most likely, as it will have an adverse performance impact.

[reedy]

Sweet. I think (not tested yet) you've fixed most of the SNR problems here, and a couple of "false positives" because of lack of knowledge of the actual code is ok.

In this case, we could easily get rid of the warning by swapping the code around

function wfMessage( $key /*...*/ ) {
	$message = new Message( $key );

	// We call Message::params() to reduce code duplication
	$params = func_get_args();
	array_shift( $params );
	if ( $params ) {
		call_user_func_array( [ $message, 'params' ], $params );
	}

	return $message;
}

to

function wfMessage( $key /*...*/ ) {
	$params = func_get_args();
	$message = new Message( $key );

	// We call Message::params() to reduce code duplication
	array_shift( $params );
	if ( $params ) {
		call_user_func_array( [ $message, 'params' ], $params );
	}

	return $message;
}

Which would clear the warning too.

Thanks! :)

[reedy]

I don't know if you've tested it again against MediaWiki/some extensions... But I will do so and report back :)