* [PATCH v2] Fix network-functions net membership check
@ 2019-12-24 10:15 Tim FitzGeorge
2019-12-24 10:21 ` Michael Tremer
0 siblings, 1 reply; 2+ messages in thread
From: Tim FitzGeorge @ 2019-12-24 10:15 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 4092 bytes --]
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(a)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;
}
- if ($bin1[0] eq $bin2[0] && $bin1[1] eq $bin2[1]) {
+ if ($bin1[0] == $bin2[0] && $bin1[1] == $bin2[1]) {
return 1;
}
@@ -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));
}
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;
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
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH v2] Fix network-functions net membership check
2019-12-24 10:15 [PATCH v2] Fix network-functions net membership check Tim FitzGeorge
@ 2019-12-24 10:21 ` Michael Tremer
0 siblings, 0 replies; 2+ messages in thread
From: Michael Tremer @ 2019-12-24 10:21 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 4882 bytes --]
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(a)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(a)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=3f3974b711391b6bbc05e5435398c1b3ee26c6e8
> - 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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-12-24 10:21 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-24 10:15 [PATCH v2] Fix network-functions net membership check Tim FitzGeorge
2019-12-24 10:21 ` Michael Tremer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox