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

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

[Bernhard Held]

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

v2 -> v3

- Network::network2bin: use 'return ();' instead of plain 'return;'

- Network::network_qual: fix identation problem

- proxy.cgi::writepacfile: use 'chomp(@templist);' once before loop

v1 -> v2

- 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 v3 1/4] Network::network_equal: fix check if array is fully defined

[Bernhard Held]

[-- Attachment #1: Type: text/plain, Size: 748 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..070e14d0f 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 v3 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 070e14d0f..7cd56f799 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 v3 3/4] proxy.cgi: strip newline from subnet parameters

[Bernhard Held]

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

The subnets in @templist end with newlines. Theses have to be removed
before printing and comparison with other subnets.
---
 html/cgi-bin/proxy.cgi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
index 6aa14e15a..d565ffbdc 100644
--- a/html/cgi-bin/proxy.cgi
+++ b/html/cgi-bin/proxy.cgi
@@ -3061,6 +3061,8 @@ END
 				close(SUBNETS);
 			}
 
+			chomp(@templist);
+
 			foreach (@templist)
 			{
 				@temp = split(/\//);
@@ -3069,7 +3071,6 @@ END
 					($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 v3 4/4] proxy.cgi: fix subnet comparison for proxy.pac generation

[Bernhard Held]

[-- Attachment #1: Type: text/plain, Size: 1240 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 d565ffbdc..d641c3df9 100644
--- a/html/cgi-bin/proxy.cgi
+++ b/html/cgi-bin/proxy.cgi
@@ -3066,9 +3066,10 @@ END
 			foreach (@templist)
 			{
 				@temp = split(/\//);
-				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

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

[Michael Tremer]

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

Hello,

On Wed, 2018-02-14 at 20:35 +0100, Bernhard Held wrote:
> 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..070e14d0f 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;
>  	}

This is actually quite a good way to probably check for the expected
result.

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

-Michael

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

[Michael Tremer]

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

Hello,

did you check if any other functions need to be changed in order to
reflect this change?

Many functions use the output of this one.

-Michael

On Wed, 2018-02-14 at 20:35 +0100, Bernhard Held wrote:
> '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 070e14d0f..7cd56f799 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;

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

[Michael Tremer]

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

Hi,

On Wed, 2018-02-14 at 20:35 +0100, Bernhard Held wrote:
> 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 d565ffbdc..d641c3df9 100644
> --- a/html/cgi-bin/proxy.cgi
> +++ b/html/cgi-bin/proxy.cgi
> @@ -3066,9 +3066,10 @@ END
>  			foreach (@templist)
>  			{
>  				@temp = split(/\//);
> -				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]\"))";

Strictly, this should be checking if the network in question is either
the GREEN or BLUE network, or if it is a subnet of thereof. This might
be a not so common use-case, but it would make the check more correct.

-Michael

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

[Bernhard Bitsch]

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

Hello,

if I can help, I could look at this also.
Some "cosmetics" (indentions, placing { on the line of 'if', ... ) could make the code more readable. These produce some greater patches without a gain in correctness but in maintainability.

Regards,
Bernhard Bitsch

> Gesendet: Mittwoch, 14. Februar 2018 um 23:12 Uhr
> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> An: "Bernhard Held" <berny156(a)gmx.de>, development(a)lists.ipfire.org
> Betreff: Re: [PATCH v3 2/4] Network::network2bin: return an empty list in case of error
>
> Hello,
> 
> did you check if any other functions need to be changed in order to
> reflect this change?
> 
> Many functions use the output of this one.
> 
> -Michael
> 
> On Wed, 2018-02-14 at 20:35 +0100, Bernhard Held wrote:
> > '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 070e14d0f..7cd56f799 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;
> 

Aw: [PATCH v3 3/4] proxy.cgi: strip newline from subnet parameters

[Bernhard Bitsch]

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

Hi,

newlines have to be stripped from each line.
Therefore the code should read:
  				close(SUBNETS);
 			}
  
  			foreach (@templist)
  			{
			        chomp;
  				@temp = split(/\//);

- Bernhard

> Gesendet: Mittwoch, 14. Februar 2018 um 20:35 Uhr
> Von: "Bernhard Held" <berny156(a)gmx.de>
> An: development(a)lists.ipfire.org
> Betreff: [PATCH v3 3/4] proxy.cgi: strip newline from subnet parameters
>
> The subnets in @templist end with newlines. Theses have to be removed
> before printing and comparison with other subnets.
> ---
>  html/cgi-bin/proxy.cgi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
> index 6aa14e15a..d565ffbdc 100644
> --- a/html/cgi-bin/proxy.cgi
> +++ b/html/cgi-bin/proxy.cgi
> @@ -3061,6 +3061,8 @@ END
>  				close(SUBNETS);
>  			}
>  
> +			chomp(@templist);
> +
>  			foreach (@templist)
>  			{
>  				@temp = split(/\//);
> @@ -3069,7 +3071,6 @@ END
>  					($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
> 
> 

Re: Aw: [PATCH v3 3/4] proxy.cgi: strip newline from subnet parameters

[Bernhard Held]

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

> Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> hat am 15. Februar 2018 um 00:14 geschrieben:
> newlines have to be stripped from each line.
> Therefore the code should read:
>   				close(SUBNETS);
>  			}
>   
>   			foreach (@templist)
>   			{
> 			        chomp;
>   				@temp = split(/\//);

Hm, I see now that I misinterpreted your last comment regarding this patch.

However, v3 is still correct, because if you chomp a list, each element is chomped.

Regards,
Bernhard

> - Bernhard
> 
> > Gesendet: Mittwoch, 14. Februar 2018 um 20:35 Uhr
> > Von: "Bernhard Held" <berny156(a)gmx.de>
> > An: development(a)lists.ipfire.org
> > Betreff: [PATCH v3 3/4] proxy.cgi: strip newline from subnet parameters
> >
> > The subnets in @templist end with newlines. Theses have to be removed
> > before printing and comparison with other subnets.
> > ---
> >  html/cgi-bin/proxy.cgi | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
> > index 6aa14e15a..d565ffbdc 100644
> > --- a/html/cgi-bin/proxy.cgi
> > +++ b/html/cgi-bin/proxy.cgi
> > @@ -3061,6 +3061,8 @@ END
> >  				close(SUBNETS);
> >  			}
> >  
> > +			chomp(@templist);
> > +
> >  			foreach (@templist)
> >  			{
> >  				@temp = split(/\//);
> > @@ -3069,7 +3071,6 @@ END
> >  					($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
> > 
> >

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

[Bernhard Held]

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

> Michael Tremer <michael.tremer(a)ipfire.org> hat am 14. Februar 2018 um 23:14 geschrieben:
> On Wed, 2018-02-14 at 20:35 +0100, Bernhard Held wrote:
> > 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 d565ffbdc..d641c3df9 100644
> > --- a/html/cgi-bin/proxy.cgi
> > +++ b/html/cgi-bin/proxy.cgi
> > @@ -3066,9 +3066,10 @@ END
> >  			foreach (@templist)
> >  			{
> >  				@temp = split(/\//);
> > -				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]\"))";
> 
> Strictly, this should be checking if the network in question is either
> the GREEN or BLUE network, or if it is a subnet of thereof. This might
> be a not so common use-case, but it would make the check more correct.

I'm sorry, that's beyond my mission. I'm just striving to get my 2.5 year old bug report closed and that's it. I'm actually not even running a single IPFire instance, thus I feel unable to invest time in any feature enhancement.

Regards,
Bernhard