When using 32-bit OS, automation errors can be seen due to subnet mask calculations

[panlinux]

Describe the bug
Function automation_get_valid_mask() fails in in https://github.com/Cacti/cacti/blob/develop/lib/api_automation.php#L3029 when converting a binary string to integer on a 32bit architecture like armhf.

This results in errors like these in the logs:

02/27/2020 03:15:08 - ERROR PHP WARNING: long2ip() expects parameter 1 to be int, float given in file: /usr/share/cacti/site/lib/api_automation.php  on line: 3006
02/27/2020 03:15:08 - CMDPHP PHP ERROR WARNING Backtrace:  (/poller_automation.php[272]:automation_primeIPAddressTable(), /lib/api_automation.php[3179]:automation_calculate_total_ips(), /lib/api_automation.php[3128]:automation_get_network_info(), /lib/api_automation.php[3034]:automation_get_valid_mask(), /lib/api_automation.php[3006]:long2ip(), CactiErrorHandler())
02/27/2020 03:15:08 - ERROR PHP WARNING: long2ip() expects parameter 1 to be int, float given in file: /usr/share/cacti/site/lib/api_automation.php  on line: 3006
02/27/2020 03:15:08 - CMDPHP PHP ERROR WARNING Backtrace:  (/poller_automation.php[272]:automation_primeIPAddressTable(), /lib/api_automation.php[3182]:automation_calculate_start(), /lib/api_automation.php[3116]:automation_get_network_info(), /lib/api_automation.php[3034]:automation_get_valid_mask(), /lib/api_automation.php[3006]:long2ip(), CactiErrorHandler())

To Reproduce
The error is showing up in the DEP8 Ubuntu tests at http://autopkgtest.ubuntu.com/packages/cacti. One can see they are failing for armhf: http://autopkgtest.ubuntu.com/packages/cacti/focal/armhf

A simple way to reproduce it is to call the automation_get_valid_mask() function with any argument between 0 and 33. For example, 24, on an amd64 machine returns this (with some debugging of mine):

$ php ./automation_get_valid_mask.php 24

bindec(11111111111111111111111100000000):
4294967040
Array
(
    [cidr] => 24
    [subnet] => 255.255.255.0
    [count] => 255
)

On armhf, however:

$ php ./automation_get_valid_mask.php 24

bindec(11111111111111111111111100000000):
4294967040PHP Warning:  long2ip() expects parameter 1 to be int, float given in /home/ubuntu/automation_get_valid_mask.php on line 13

Array
(
    [cidr] => 24
    [subnet] => 
    [count] => 255
)

There are warnings about issues with 32bit architectures on https://www.php.net/manual/en/function.bindec.php and https://www.php.net/manual/en/function.long2ip.php

Expected behavior
On armhf 32bit, automation_get_valid_mask() should work properly. For example, return "255.255.255.0" for "subnet" when called with a range == 24.

Cacti server

• OS: Ubuntu 20.04 Focal Fossa (development release)
• php 7.3.15
• architecture: armhf (32bits)

Misc
This is the script I used. I copied the function from lib/api_automation.php and added some debugging:

<?php
function automation_get_valid_mask($range) {
    $cidr = false;
    if (is_numeric($range)) {
        if ($range > 0 && $range < 33) {
            $cidr = $range;
	    $ones = str_repeat('1',$range);
	    $zeroes = str_repeat('0',32-$range);
	    echo "\nbindec(" . $ones . $zeroes . "):\n";
	    print_r(bindec($ones . $zeroes));
            $mask = array(
                'cidr' => $cidr,
                'subnet' => long2ip(bindec($ones . $zeroes)));
        } else {
            $mask = false;
        }
    } else {
        $mask = automation_get_valid_subnet_cidr($range);
    }

    if ($mask !== false) {
        $mask['count'] = bindec(str_repeat('0',$mask['cidr']) . str_repeat('1',32-$mask['cidr']));
        if ($mask['count'] == 0) {
            $mask['count'] = 1;
        }
    }
    return $mask;
}

$bits = strval($argv[1]);
$mask = automation_get_valid_mask($bits);
echo "\n";
print_r($mask);
?>

[TheWitness]

Can you do a pull request against the 1.2.x branch?

[panlinux]

I wonder if this is enough to fix it? I'm wary of casting variable types and its behavior between different architectures:

diff --git a/lib/api_automation.php b/lib/api_automation.php
index a1373af08..c71b08705 100644
--- a/lib/api_automation.php
+++ b/lib/api_automation.php
@@ -3003,7 +3003,7 @@ function automation_get_valid_mask($range) {
 			$cidr = $range;
 			$mask = array(
 				'cidr' => $cidr,
-				'subnet' => long2ip(bindec(str_repeat('1',$range) . str_repeat('0',32-$range))));
+				'subnet' => long2ip((int)bindec(str_repeat('1',$range) . str_repeat('0',32-$range))));
 		} else {
 			$mask = false;
 		}

[panlinux]

With the casting change you mean?

[panlinux]

Maybe this is better:

diff --git a/lib/api_automation.php b/lib/api_automation.php
index a1373af08..b9fffbeeb 100644
--- a/lib/api_automation.php
+++ b/lib/api_automation.php
@@ -3003,7 +3003,7 @@ function automation_get_valid_mask($range) {
 			$cidr = $range;
 			$mask = array(
 				'cidr' => $cidr,
-				'subnet' => long2ip(bindec(str_repeat('1',$range) . str_repeat('0',32-$range))));
+				'subnet' => long2ip((2**$range-1) << (32-$range)))
 		} else {
 			$mask = false;
 		}

[netniV]

Has this been tested against PHP 5.4 or 5.5? I suspect not as I just noticed that Travis has moaned about the use of 2**

[panlinux]

No, I didn't test against those versions of php. Is there no ** operator there, or is it an issue with parenthesis?

[netniV]

I was not sure, I briefly saw that Travis didn't like it, so I looked at the php docs:

$a ** $b | Exponentiation | Result of raising $a to the $b'th power. Introduced in PHP 5.6.

So, we can only use that if PHP 5.4 or later, which unfortunately mean we either roll back the change or make an if statement and accept that under 5.4/5.5, there is a flaw in the logic on little endian systems making it a known issue.

[panlinux]

I'll see if I can come up with something tomorrow

…

On Thu, Mar 5, 2020, 20:39 Mark Brugnoli-Vinten ***@***.***> wrote: I was not sure, I briefly saw that Travis didn't like it, so I looked at the php docs: $a ** $b | Exponentiation | Result of raising $a to the $b'th power. Introduced in PHP 5.6. So, we can only use that if PHP 5.4 or later, which unfortunately mean we either roll back the change or make an if statement and accept that under 5.4/5.5, there is a flaw in the logic on little endian systems making it a known issue. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3310?email_source=notifications&email_token=ABHBKGWOQMSKHBPTOKK77L3RGAZ33A5CNFSM4K7YCCEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN7HGBQ#issuecomment-595489542>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHBKGTSI4YHK2MWQNF26ULRGAZ33ANCNFSM4K7YCCEA> .

[netniV]

That would be good thanks as unfortunately I don't have a system to test that against. Took me a while to come up with my original code but yours was a lot simpler :)

Could we reverse the bit order depending on the endian using my original code for older PHP versions?

[panlinux]

How far back in terms of PHP do you need to go?

[panlinux]

If you prefer to leave your old code with "1" and "0" strings, then maybe the (int) casting change from #3310 (comment) would be best, but we would have to test on the old php versions as well.

I did test the ** fix for endianess, and no changes were required, I'm guessing long2ip() knows about these things. At least in php7.3.

The oldest php I can get is the one from Ubuntu Precise: 5.3.10.

[panlinux]

This worked on precise's php 5.3.10:

'subnet' => long2ip((pow(2,$range)-1) << (32-$range)));

Let me try on other architectures now.

[panlinux]

The oldest php I can get on big-endian (s390x in this case) is php 7.0.33, from ubuntu xenial, same from debian stretch. The script keeps working fine there, with the pow() modification. Would you like a PR for this change?

[netniV]

Sure if you already have it that would be great :) Otherwise I can just apply the change over the weekend.

[cigamit]

The pow() function is working well in x86_64 on PHP5.4. So, I guess this is resolved then?

[netniV]

Yes, I believe so. Be nice when PHP 5 is no longer a worry.