Upgrade code fatal error

phil.davis

I came to one 2.2.3 system a few days ago, it had had some issue reported that there was no DHCP/DNS… and one of our guys connected the console and restarted it. About that time the dashboard had this fatal PHP error reported:

Crash report begins.  Anonymous machine information:

amd64
10.1-RELEASE-p13
FreeBSD 10.1-RELEASE-p13 #0 c77d1b2(releng/10.1)-dirty: Tue Jun 23 17:00:47 CDT 2015     root@pfs22-amd64-builder:/usr/obj.amd64/usr/pfSensesrc/src/sys/pfSense_SMP.10

Crash report details:

PHP Errors:
[17-Jul-2015 03:14:55 Etc/UTC] PHP Fatal error:  Cannot unset string offsets in /etc/inc/upgrade_config.inc on line 291

Filename: /var/crash/minfree
2048

Line 291 is in function upgrade_016_to_017():

	unset($config['shaper']['queue']);

The config.xml of this system has no traffic shaper, only some limiters. The shaper section is only:

	 <shaper></shaper>

And indeed that looks like a string and text array elements cannot be unset:

var_dump($config['shaper']);
unset($config['shaper']['queue']);

string(0) ""

Fatal error: Cannot unset string offsets in /usr/local/www/exec.php(250) : eval()'d code on line 2

I have no idea why that code was even being executed. Somehow the system must have thought it had an ancient config version and tried to upgrade it. Its config version currently shows:

	<version>11.7</version>

In any case, should all that sort of "unset" code be protected by if(isset())? Just in case the referenced thing above it is a string and not an array.

azzido

That's an inherent problem with using arrays for storing and passing around configuration options. It is very hard to write code that always checks if element you are accessing/manipulating actually exists. Sometimes it does not cause any issues, but sometimes it leads to fatal failure.

If you boot dev box with error_reporting set to E_ALL in php.ini file, you will see that PHP will generate a ton (and i mean a ton) of warnings for the code that is accessing/manipulating array elements that do not exists. And that's only booting a box.

I think setting error_reporting to E_ALL for dev snapshots would help catch a lot more bugs, but it would be big effort to modify all existing code that generates PHP warnings. And since pfSense is heading to being rewritten from scratch in different language it is not clear if that would be worth the effort.

It might be worth mentioning that this code does not generate fatal error:


$config = array();
$config['shaper'] = array();

unset($config['shaper']['queue']);

but this code, which is what you encountered, does:


$config = array();
$config['shaper'] = "";

unset($config['shaper']['queue']);

so if there was no <shaper>XML element in the config file you would have been fine.</shaper>

phil.davis

Yes, what you say is correct. And it is all a bit of a pain in PHP, you have to constantly check isset() is_array()…
I have protected all the unset() in /etc/inc/upgrade_config.inc with this pull request:
https://github.com/pfsense/pfsense/pull/1773

I guess it was a bit much for the devs to dump into 2.2.4 just days before release. I can't see anything that would go wrong with it - it can only be better - but it does need to have some upgrades from old configs run against it just to confirm all is well.