rbicelli / pfsense-zabbix-template

Zabbix Template for pfSense
Apache License 2.0
246 stars 109 forks source link

Fix broken variable initializations php 8.1 #139

Closed edeckers closed 1 year ago

edeckers commented 1 year ago

The changes in this PR fix these issues:

Comparable to fixes proposed by others, such as https://github.com/rbicelli/pfsense-zabbix-template/issues/132#issuecomment-1446074455. But this PR fixes it at the core, i.e.: the missing initialization of the variables.

So instead of having to check in multiple places for is_countable to avoid NULL comparisons, we can assume the variable is an array, because it was an array from the beginning instead of NULL.

stephanehofman commented 1 year ago

Hello, I have tried your last script version... Still errors PHP ERROR: Type: 1, File: /root/scripts/pfsense_zbx.php, Line: 160, Message: Uncaught TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given in /root/scripts/pfsense_zbx.php:160 Stack trace:

0 /root/scripts/pfsense_zbx.php(1312): pfz_interface_speedtest_value('ipsec1', 'download')

1 {main}

thrown @ 2023-03-14 15:10:53 PHP ERROR: Type: 1, File: /root/scripts/pfsense_zbx.php, Line: 160, Message: Uncaught TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given in /root/scripts/pfsense_zbx.php:160 Stack trace:

0 /root/scripts/pfsense_zbx.php(1312): pfz_interface_speedtest_value('ipsec1', 'ping')

1 {main}

thrown @ 2023-03-14 15:10:56 PHP ERROR: Type: 1, File: /root/scripts/pfsense_zbx.php, Line: 160, Message: Uncaught TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given in /root/scripts/pfsense_zbx.php:160 Stack trace:

0 /root/scripts/pfsense_zbx.php(1312): pfz_interface_speedtest_value('ipsec1', 'upload')

1 {main}

edeckers commented 1 year ago

Thank you @stephanehofman! The last commit in this PR should take care of that.

edeckers commented 1 year ago

The PR is ready to merge as far as I'm concerned: Verified on my own pfSense 23.01 box that the changes from this PR address the initializations issues and do not seem to introduce any new problems.

edeckers commented 1 year ago

Just a heads-up, the changes in b07d9074e98ccbefb1534094caf8b37fdfd404dd are kind of cheating: we silently ignore when the speedtest result file is empty. I'm not sure why these files end up empty every now and then, that would need some further investigation but is out of scope for this PR.

stephanehofman commented 1 year ago

I have tried https://github.com/rbicelli/pfsense-zabbix-template/commit/b07d9074e98ccbefb1534094caf8b37fdfd404dd It seems working without errors now :)

stephanehofman commented 1 year ago

I have tried https://github.com/rbicelli/pfsense-zabbix-template/commit/b07d9074e98ccbefb1534094caf8b37fdfd404dd It seems working without errors now :)

LinuxCuba commented 1 year ago

14.0-CURRENT FreeBSD 14.0-CURRENT #0 plus-RELENG_23_01-n256037-6e914874a5e: Fri Feb 10 20:30:29 UTC 2023 root@freebsd:/var/jenkins/workspace/pfSense-Plus-snapshots-23_01-main/obj/amd64/VDZvZksF/var/jenkins/workspace/pfSense-Plus-snapshots-23_01-main/sources/FreeBS

Crash report details:

PHP Errors: [01-May-2023 04:45:16 Europe/Madrid] PHP Fatal error: Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, null given in /root/scripts/pfsense_zbx.php:978 Stack trace:

0 /root/scripts/pfsense_zbx.php(1024): pfz_dhcp_get('failover')

1 /root/scripts/pfsense_zbx.php(1037): pfz_dhcp_check_failover()

2 /root/scripts/pfsense_zbx.php(1338): pfz_dhcp('failover', NULL)

3 {main}

thrown in /root/scripts/pfsense_zbx.php on line 978

Fix with this change. Check if not null before of the constant initialization.

/* remove duplicate items by mac address */
if (!is_null($leases) && (count($leases) > 0) {
    $leases = pfz_remove_duplicate($leases, "ip");
}

if (!is_null($pools) && (count($pools)) > 0) {
    $pools = pfz_remove_duplicate($pools, "name");
    asort($pools);
}
edeckers commented 1 year ago

Hi @LinuxCuba , thank you for your comment!

Looking at the line number 978 and the code where the error was thrown, you seem to be running the pfsense_zbx.php version from the master branch. Can you try to check out the pfsense_zbx.php from my PR we're now commenting on? I think that fixes your problem as well as some other, related issues.

My fix is to instead of checking for null, to ensure the variables will never be null by initializing them at the beginning of the method: https://github.com/rbicelli/pfsense-zabbix-template/blob/b07d9074e98ccbefb1534094caf8b37fdfd404dd/pfsense_zbx.php#L862

and

https://github.com/rbicelli/pfsense-zabbix-template/blob/b07d9074e98ccbefb1534094caf8b37fdfd404dd/pfsense_zbx.php#L863

If you decide to try it, please let me know if it worked or not, so I can fix the issue if need be.

checkerbomb commented 1 year ago

Not @LinuxCuba, but can concur with other comments above. Installed the PR on three pfSense instances (2x Netgate 5100, 1x Netgate 7100), all running 23.01-RELEASE. Working great now, no errors and Zabbix agent is functioning normally.

edeckers commented 1 year ago

Thanks of checking and reporting @checkerbomb :)

LinuxCuba commented 1 year ago

Thank you so much. I will be testing it now.

Hi @LinuxCuba , thank you for your comment!

Looking at the line number 978 and the code where the error was thrown, you seem to be running the pfsense_zbx.php version from the master branch. Can you try to check out the pfsense_zbx.php from my PR we're now commenting on? I think that fixes your problem as well as some other, related issues.

My fix is to instead of checking for null, to ensure the variables will never be null by initializing them at the beginning of the method:

https://github.com/rbicelli/pfsense-zabbix-template/blob/b07d9074e98ccbefb1534094caf8b37fdfd404dd/pfsense_zbx.php#L862

and

https://github.com/rbicelli/pfsense-zabbix-template/blob/b07d9074e98ccbefb1534094caf8b37fdfd404dd/pfsense_zbx.php#L863

If you decide to try it, please let me know if it worked or not, so I can fix the issue if need be.

Thank you so much. I will be testing it now.