Re: [PATCH v2] Fix network-functions net membership check

Hi Tim,

This is a large patch with many independent changes which would have been easier to review in multiple smaller patches…

On 24 Dec 2019, at 11:15, Tim FitzGeorge ipfr@tfitzgeorge.me.uk wrote:

Replace textual comparison of two number with arithmetic in ip_address_in_Network().

Changes since V1:

• Correctly copied change from production system
• Fixed test of non-existent variables in network2bin
• Enhanced testsuite to report which test failed

Fixes: 12263 Signed-off-by: Tim FitzGeorge ipfr@tfitzgeorge.me.uk

---

config/cfgroot/network-functions.pl | 38 ++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl index 8649d0502..b911e214f 100644 --- a/config/cfgroot/network-functions.pl +++ b/config/cfgroot/network-functions.pl @@ -111,11 +111,11 @@ sub network_equal { my @bin1 = &network2bin($network1); my @bin2 = &network2bin($network2);

• if (!defined $bin1 || !defined $bin2) {

• if (!defined $bin1[0] || !defined $bin2[0]) { return undef; }

What is this part supposed to do?

&network2bin() might return “undef” and therefore you cannot check for $bin1[0] straight away.

This has been introduced in:

https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=3f3974b711391b6bbc05...

• if ($bin1[0] eq $bin2[0] && $bin1[1] eq $bin2[1]) {

• if ($bin1[0] == $bin2[0] && $bin1[1] == $bin2[1]) { return 1; }

I agree with this one.

@@ -295,7 +295,7 @@ sub ip_address_in_network($$) { # Find end address my $broadcast_bin = $network_bin ^ (~$netmask_bin % 2 ** 32);

• return (($address_bin ge $network_bin) && ($address_bin le $broadcast_bin));

• return (($address_bin >= $network_bin) && ($address_bin <= $broadcast_bin));

Likewise.

}

sub setup_upstream_proxy() { @@ -449,14 +449,15 @@ sub get_mac_by_name($) { # Remove the next line to enable the testsuite __END__

-sub assert($) { +sub assert($$) {

• my $tst = shift; my $ret = shift;

Indentation is off here. Not a major thing, but a thing nevertheless.

if ($ret) { return; }

• print "ASSERTION ERROR";

• print "ASSERTION ERROR - $tst\n"; exit(1);

}

@@ -464,10 +465,10 @@ sub testsuite() { my $result;

my $address1 = &ip2bin("8.8.8.8");

• assert($address1 == 134744072);

• assert('ip2bin("8.8.8.8")', $address1 == 134744072);

  my $address2 = &bin2ip($address1);

• assert($address2 eq "8.8.8.8");

• assert("bin2ip($address1)", $address2 eq "8.8.8.8");

  # Check if valid IP addresses are correctly recognised. foreach my $address ("1.2.3.4", "192.168.180.1", "127.0.0.1") {

@@ -486,34 +487,37 @@ sub testsuite() { }

$result = &check_ip_address_and_netmask("192.168.180.0/255.255.255.0");

• assert($result);

• assert('check_ip_address_and_netmask("192.168.180.0/255.255.255.0")', $result);

  $result = &convert_netmask2prefix("255.255.254.0");

• assert($result == 23);

• assert('convert_netmask2prefix("255.255.254.0")', $result == 23);

  $result = &convert_prefix2netmask(8);

• assert($result eq "255.0.0.0");

• assert('convert_prefix2netmask(8)', $result eq "255.0.0.0");

  $result = &find_next_ip_address("1.2.3.4", 2);

• assert($result eq "1.2.3.6");

• assert('find_next_ip_address("1.2.3.4", 2)', $result eq "1.2.3.6");

  $result = &network_equal("192.168.0.0/24", "192.168.0.0/255.255.255.0");

• assert($result);

• assert('network_equal("192.168.0.0/24", "192.168.0.0/255.255.255.0")', $result);

  $result = &network_equal("192.168.0.0/24", "192.168.0.0/25");

• assert(!$result);

• assert('network_equal("192.168.0.0/24", "192.168.0.0/25")', !$result);

  $result = &network_equal("192.168.0.0/24", "192.168.0.128/25");

• assert(!$result);

• assert('network_equal("192.168.0.0/24", "192.168.0.128/25")', !$result);

  $result = &network_equal("192.168.0.1/24", "192.168.0.XXX/24");

• assert(!$result);

• assert('network_equal("192.168.0.1/24", "192.168.0.XXX/24")', !$result);

  $result = &ip_address_in_network("10.0.1.4", "10.0.0.0/8");

• assert($result);

• assert('ip_address_in_network("10.0.1.4", "10.0.0.0/8"', $result);

  $result = &ip_address_in_network("192.168.30.11", "192.168.30.0/255.255.255.0");

• assert($result);

• assert('ip_address_in_network("192.168.30.11", "192.168.30.0/255.255.255.0")', $result);

• $result = &ip_address_in_network("192.168.30.11", "0.0.0.0/8");

• assert('ip_address_in_network("192.168.30.11", "0.0.0.0/8")', !$result);

  print "Testsuite completed successfully!\n";

-- 2.16.4