[PATCH v2 0/4] proxy.cgi fixes for bugzilla #10852

[PATCH v2 0/4] proxy.cgi fixes for bugzilla #10852

[Bernhard Held]

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

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

[PATCH v2 1/4] Network::network_equal: fix check if array is fully defined

[Bernhard Held]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

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]) {
-- 
2.16.1

[PATCH v2 2/4] Network::network2bin: return an empty list in case of error

[Bernhard Held]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

'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

[PATCH v2 3/4] proxy.cgi: strip newline from subnet parameter

[Bernhard Held]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

$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]\"))";
 				}
 			}
-- 
2.16.1

[PATCH v2 4/4] proxy.cgi: fix subnet comparison for proxy.pac generation

[Bernhard Held]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

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]\"))";
-- 
2.16.1

Aw: [PATCH v2 2/4] Network::network2bin: return an empty list in case of error

[Bernhard Bitsch]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

return (); 
(the empty array) makes the code a bit more readable.

> Gesendet: Montag, 12. Februar 2018 um 23:19 Uhr
> Von: "Bernhard Held" <berny156(a)gmx.de>
> An: development(a)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
> 
>