v2 with following changes:
Network::network_qual: Improved check if array is fully defined. Honestly, the testsuite of the unpatched file still fails on my IPFire for an unknown reason. The patches fixe the issue for me using perl v5.12.3 as well as v5.26.1.
Network::network2bin: 'return undef;' doesn't return an empty list causing lots of headaches. Popular sites strongly recommend to use 'return;' instead of 'return undef;'. Maybe 'return undef;' should be avoided everywhere in IPFire.
Have fun, Bernhard
Correctly check length of list. Credit to Bernhard Bitsch for pointing in the right direction. --- config/cfgroot/network-functions.pl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl index 2902aabb0..93bb646a6 100644 --- a/config/cfgroot/network-functions.pl +++ b/config/cfgroot/network-functions.pl @@ -111,8 +111,9 @@ sub network_equal { my @bin1 = &network2bin($network1); my @bin2 = &network2bin($network2);
- if (!defined $bin1 || !defined $bin2) { - return undef; + unless (scalar @bin1 == 2 && + scalar @bin2 == 2) { + return; }
if ($bin1[0] eq $bin2[0] && $bin1[1] eq $bin2[1]) {
'return undef;' will always return a single value 'undef' even in list context.
sub foo { return undef } if ( my @x = foo() ) { print "oops, we think we got a result"; } --- config/cfgroot/network-functions.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl index 93bb646a6..41d36a194 100644 --- a/config/cfgroot/network-functions.pl +++ b/config/cfgroot/network-functions.pl @@ -139,7 +139,7 @@ sub network2bin($) { my $netmask_bin = &ip2bin($netmask);
if (!defined $address_bin || !defined $netmask_bin) { - return undef; + return; }
my $network_start = $address_bin & $netmask_bin;
return (); (the empty array) makes the code a bit more readable.
Gesendet: Montag, 12. Februar 2018 um 23:19 Uhr Von: "Bernhard Held" berny156@gmx.de An: development@lists.ipfire.org Betreff: [PATCH v2 2/4] Network::network2bin: return an empty list in case of error
'return undef;' will always return a single value 'undef' even in list context.
sub foo { return undef } if ( my @x = foo() ) { print "oops, we think we got a result"; }
config/cfgroot/network-functions.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl index 93bb646a6..41d36a194 100644 --- a/config/cfgroot/network-functions.pl +++ b/config/cfgroot/network-functions.pl @@ -139,7 +139,7 @@ sub network2bin($) { my $netmask_bin = &ip2bin($netmask);
if (!defined $address_bin || !defined $netmask_bin) {
return undef;
return;}
my $network_start = $address_bin & $netmask_bin;
-- 2.16.1
$temp[1] might end with a newline; this is unavoidable when specifying serveral subnets. Thus, 'chomp $temp[1];' has to be moved before the comparisons with the green and blue subnets. Otherwise the comparison might always be wrong due to the newline. --- html/cgi-bin/proxy.cgi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi index 6aa14e15a..ea3b41126 100644 --- a/html/cgi-bin/proxy.cgi +++ b/html/cgi-bin/proxy.cgi @@ -3064,12 +3064,12 @@ END foreach (@templist) { @temp = split(///); + chomp $temp[1]; if ( ($temp[0] ne $netsettings{'GREEN_NETADDRESS'}) && ($temp[1] ne $netsettings{'GREEN_NETMASK'}) && ($temp[0] ne $netsettings{'BLUE_NETADDRESS'}) && ($temp[1] ne $netsettings{'BLUE_NETMASK'}) ) { - chomp $temp[1]; print FILE " ||\n (isInNet(myIpAddress(), "$temp[0]", "$temp[1]"))"; } }
The logic of subnet comparison is broken. E.g. if the blue netmask is 255.255.255.0, it's impossible to add a VPN subnet with the same netmask. The fix simplifies the logic by using Network::network_equal. --- html/cgi-bin/proxy.cgi | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi index ea3b41126..df436595b 100644 --- a/html/cgi-bin/proxy.cgi +++ b/html/cgi-bin/proxy.cgi @@ -3065,9 +3065,10 @@ END { @temp = split(///); chomp $temp[1]; - if ( - ($temp[0] ne $netsettings{'GREEN_NETADDRESS'}) && ($temp[1] ne $netsettings{'GREEN_NETMASK'}) && - ($temp[0] ne $netsettings{'BLUE_NETADDRESS'}) && ($temp[1] ne $netsettings{'BLUE_NETMASK'}) + unless ( + # GREEN or BLUE networks are already added to "DIRECT". Check if given network is different from these. + &Network::network_equal("$temp[0]/$temp[1]", "$netsettings{'GREEN_NETADDRESS'}/$netsettings{'GREEN_NETMASK'}") || + &Network::network_equal("$temp[0]/$temp[1]", "$netsettings{'BLUE_NETADDRESS'}/$netsettings{'BLUE_NETMASK'}") ) { print FILE " ||\n (isInNet(myIpAddress(), "$temp[0]", "$temp[1]"))";
-
Bernhard Bitsch -
Bernhard Held