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

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;

Bernhard Bitsch Bernhard.Bitsch@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@gmx.de An: development@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

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