Uncaught Error: Attempt to assign property "abspath" on bool

WP fail2ban Bugs and Patches
1

Hi there!

I’m running the latest versions of both WP and wp-fail2ban in a system running Ubuntu 20.04.2 LTS, nginx/1.19.6, PHP 8.0.2, and Fail2Ban v0.11.1.

Early this morning, wp-fail2ban threw the above-mentioned fatal error. Here’s the stack trace, redacted for privacy issues:

PHP message: PHP Fatal error: Uncaught Error: Attempt to assign property "abspath" on bool in [redacted WP_PATH]/wp-content/plugins/wp-fail2ban/vendor/freemius/wordpress-sdk/start.php:86

Stack trace:
#0 [redacted WP_PATH]/wp-content/plugins/wp-fail2ban/freemius.php(38): require_once()
#1 [redacted WP_PATH]/wp-content/plugins/wp-fail2ban/freemius.php(65): org\lecklider\charles\wordpress\wp_fail2ban\wf_fs()
#2 [redacted WP_PATH]/wp-content/plugins/wp-fail2ban/wp-fail2ban.php(46): require_once('...')
#3 [redacted WP_PATH]/wp-settings.php(319): include_once('...')
#4 [redacted WP_PATH]/wp-admin/setup-config.php(33): require('...')
#5 {main}
thrown in [redacted WP_PATH]/wp-content/plugins/wp-fail2ban/vendor/freemius/wordpress-sdk/start.php on line 86" while reading response header from upstream, client: 173.201.196.93, server: my.server.tld request: "GET /wp-admin/setup-config.php HTTP/1.1", upstream: "fastcgi://unix:[redacted]/sock:", host: "my.server.tld"

The request made by 173.201.196.93 was:

173.201.196.93 - - [20/Feb/2021:04:27:20 +0000] "GET /wp-admin/setup-config.php HTTP/1.1" 500 1851 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36"

which is obviously a ‘forbidden’ request. The origin seems to be a site hosted somewhere by GoDaddy, and the IP address was promptly blocked (as it should), although I’m not quite sure which of my many, many security measures blocked it :wink:

Anyway, this was a ‘fatal error’ which didn’t really impact the access to the web site (as far as I can see). But allegedly this is something coming from one of the imported libraries so maybe it’s worthy of taking a look to understand why that error appears.

It’s a non-critical issue :slight_smile:

(note: minor edit for further redaction which I missed)

3

Here’s an attempt to ‘fix’ the above issue — Invis might have to contact the guys at Freemius to change their libraries.

The ‘problem’ above comes from the fact that PHP 8.0 is much stricter with types — what used to be perfectly valid expressions in the PHP 5.X days and became ‘warnings’ under PHP 7.X, have now (finally) become errors, some of which are termed fatal. There is a good reason for this: the stronger the typing, the more efficient the compilation; and PHP 8.0 reaps the benefits of better performance mostly thanks to that. The flip side of the coin is that PHP programmers have to stop being so lazy and not ‘assume’ so many things about their variables :slight_smile: (I know, I know, I also like PHP a lot because of its very lazy, laissez-faire attitude towards variable types…).

Opening wp-content/plugins/wp-fail2ban/vendor/freemius/wordpress-sdk/start.php I can see that, around line 69, the code leading to the culprit on line 86:

    if ( ! isset( $fs_active_plugins ) ) {
        // Load all Freemius powered active plugins.                                            
        $fs_active_plugins = get_option( 'fs_active_plugins', new stdClass() );

        if ( ! isset( $fs_active_plugins->plugins ) && ! empty( $fs_active_plugins ) ) {
            $fs_active_plugins->plugins = array();
        }
    }

    if ( empty( $fs_active_plugins->abspath ) ) {
	/**                                                                                     
         * Store the WP install absolute path reference to identify environment change          
         * while replicating the storage.                                                       
         *                                                                                      
         * @author Vova Feldman (@svovaf)                                                       
         * @since  1.2.1.7                                                                      
         */
        $fs_active_plugins->abspath = ABSPATH;
    } else {

The issue here is that if $fs_active_plugins gets initialised at some point with the boolean value false (it’s a global variable, I can’t see where it is set), this code won’t work under PHP 8.0. In that scenario, the initial if ( ! isset( $fs_active_plugins ) ) { will not fail (because $fs_active_plugins will be set to false) — thus, it will fail to actually initialise $fs_active_plugins — therefore empty( $fs_active_plugins->abspath ) will return true (because empty($var) is essentially !isset($var) || $var == false — see manual ), but once the code reaches line 86, it calls a member function ‘abspath’ from a boolean, and, under PHP 8.0, this is simply impossible — thus the fatal error being thrown.

I’d suggest to change line 69 to if ( empty( $fs_active_plugins ) ) { — that ought to catch both an unitialised $fs_active_plugins as well as $fs_active_plugins set to null or false.

Note that such a tiny change would also work under PHP 7 and lower, nothing would be ‘broken’. But it’s better to leave the Freemius developers to do all sorts of tests with that code; also note that the Freemius SDK is at 2.4.2 as of writing (you seem to be still using 2.4.1.0), but, as far as I can see, this particular file wasn’t changed (but, as said, since it relies on a global variable, I didn’t check the Freemius code to see where it gets set).

4

Lot’s to unpack there - I’ll try to take it bit by bit.

  1. IMO no-one should be running ClassicPress or WordPress in production on PHP8 yet. As part of the ClassicPress project I’m well aware of the amount of testing WP have done with v8 and IMO it’s nowhere near ready - the unit test code coverage is way too low to have confidence things will work as they should.

  2. IMO PHP8 isn’t ready for production yet.The changes are so deep and wide that I’m not going near it for production until at least 8.1; if they do a Microsoft and push an early version bump just to signal readiness I’ll be waiting until 8.2. Rinse, repeat as desired :slight_smile:

  3. I’ve not yet tested WPf2b with PHP8. It’s on the todo list sometime after getting v4.3.2.1 released, and as I’ve got good code coverage with unit tests it shouldn’t be too much of a chore.

  4. v4.3.0.9 was released just after Freemius v2.4.1; Freemius v2.4.2 was released this week so I’ll probably do v4.3.0.10 next week (there are a couple of very minor updates). I’ve tried not to do a release just for the sake of updating the Freemius library, but I can see there might be a need for that in future to deal with PHP8.

And now for the bug itself :slight_smile:

This particular bug perfectly illustrates what I’m talking about in my first point. There’s nothing actually wrong with the code that’s failing - it’s some side-effect somewhere else that’ll need a debugger to track down. There will be many, many cases of that in ClassicPress and WordPress that will take years to uncover fully - and then there are all the plugins.

Pre 7.4 empty() was much underused - there simply weren’t many times you cared about the difference between empty/false/null. With the stricter checks around countability in 7.4 it suddenly became useful, and with PHP8 I can see it becoming commonplace.

For me, PHP8 is repeating the mistakes Python made with v3. I’m all for stricter typing - I came from a C/C++ background before Perl, Python, and PHP - but it should have been something to enable like strict_types, not forced for everything. It’s going to be many years before all the legacy PHP libraries out there are truly safe to use with PHP8, and I expect a lot will never make it. It’s not impossible that PHP will split over this - they’re certainly going to have to maintain 7.x past 2030.

This case is actually pretty harmless as it falls over loudly, but because the behaviour of PHP has changed so fundamentally with v8 there will be many more cases where nothing blows up but the code silently does something different. Without good unit tests these will be almost impossible to detect and fix, and generally, people don’t have good unit tests.

I’ll talk to Freemius about their plans for PHP8 as this is almost certainly not the only issue you’ll run into, but for now I strongly recommend against using PHP8 with ClassicPress or WordPress.