From df2fb88bf6036b0aaeb10c63e4f0ed4e8fc827d2 Mon Sep 17 00:00:00 2001 From: Ad Schellevis Date: Thu, 2 Jun 2022 18:59:49 +0200 Subject: [PATCH] Firewall: Aliases - performance improvement for large numbers of (port type) aliases. o cache getservbyname() results when validating a port in isPort() and use the same static object in is_port() for legacy code o move isAlias() to use getByName() in the alias model so we can add a simple caching mechanism there To invalidate the cache for isAlias() one could either hook a new instance of the model using attachAliasObject() or attach an empty one attachAliasObject(null). --- src/etc/inc/util.inc | 10 +--- .../app/library/OPNsense/Firewall/Util.php | 50 ++++++++++++++----- .../app/models/OPNsense/Firewall/Alias.php | 38 ++++++++++++-- 3 files changed, 73 insertions(+), 25 deletions(-) diff --git a/src/etc/inc/util.inc b/src/etc/inc/util.inc index 335f6a799..98230e63b 100644 --- a/src/etc/inc/util.inc +++ b/src/etc/inc/util.inc @@ -702,15 +702,7 @@ function is_macaddr($macaddr, $partial = false) /* returns true if $port is a valid TCP/UDP port */ function is_port($port) { - if (getservbyname($port, "tcp") || getservbyname($port, "udp")) { - return true; - } - if (!ctype_digit($port)) { - return false; - } elseif ((intval($port) < 1) || (intval($port) > 65535)) { - return false; - } - return true; + return OPNsense\Firewall\Util::isPort($port, false); } /* returns true if $portrange is a valid TCP/UDP portrange (":") */ diff --git a/src/opnsense/mvc/app/library/OPNsense/Firewall/Util.php b/src/opnsense/mvc/app/library/OPNsense/Firewall/Util.php index a9efd7364..372474fb1 100644 --- a/src/opnsense/mvc/app/library/OPNsense/Firewall/Util.php +++ b/src/opnsense/mvc/app/library/OPNsense/Firewall/Util.php @@ -45,7 +45,12 @@ class Util /** * @var null|array cached alias descriptions */ - private static $aliasDescriptions = array(); + private static $aliasDescriptions = []; + + /** + * @var array cached getservbyname results + */ + private static $servbynames = []; /** * is provided address an ip address. @@ -107,6 +112,9 @@ class Util public static function attachAliasObject($alias) { self::$aliasObject = $alias; + if ($alias != null) { + $alias->flushCache(); + } } /** @@ -121,18 +129,18 @@ class Util if (self::$aliasObject == null) { // Cache the alias object to avoid object creation overhead. self::$aliasObject = new Alias(); + self::$aliasObject->flushCache(); } if (!empty($name)) { - foreach (self::$aliasObject->aliasIterator() as $alias) { - if ($alias['name'] == $name) { - if ($valid) { - // check validity for port type aliases - if (preg_match("/port/i", $alias['type']) && empty($alias['content'])) { - return false; - } + $alias = self::$aliasObject->getByName($name); + if ($alias != null) { + if ($valid) { + // check validity for port type aliases + if (preg_match("/port/i", (string)$alias->type) && empty((string)$alias->content)) { + return false; } - return true; } + return true; } } return false; @@ -209,6 +217,23 @@ class Util return $result; } + /** + * cached version of getservbyname() + * @param string $service service name + * @param string $protocol protocol name + * @return boolean + */ + private static function getservbyname($service, $protocol) + { + if (!isset(self::$servbynames[$protocol])){ + self::$servbynames[$protocol] = []; + } + if (!isset(self::$servbynames[$protocol][$service])) { + self::$servbynames[$protocol][$service] = getservbyname($service, $protocol); + } + return self::$servbynames[$protocol][$service]; + } + /** * check if name exists in alias config section * @param string $number port number or range @@ -219,10 +244,9 @@ class Util { $tmp = explode(':', $number); foreach ($tmp as $port) { - if ( - !getservbyname($port, "tcp") && !getservbyname($port, "udp") - && (filter_var($port, FILTER_VALIDATE_INT, array( - "options" => array("min_range" => 1, "max_range" => 65535))) === false || !ctype_digit($port)) + if ((filter_var($port, FILTER_VALIDATE_INT, array( + "options" => array("min_range" => 1, "max_range" => 65535))) === false || !ctype_digit($port)) && + !self::getservbyname($port, "tcp") && !self::getservbyname($port, "udp") ) { return false; } diff --git a/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php b/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php index 504bde296..6e3c1a135 100644 --- a/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php +++ b/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php @@ -45,6 +45,11 @@ class Alias extends BaseModel */ private $aliasIteratorCache = []; + /** + * alias name cache + */ + private $aliasReferenceCache = []; + /** * return locations where aliases can be used inside the configuration * @return array alias source map @@ -107,6 +112,15 @@ class Alias extends BaseModel } } + /** + * flush cached objects and references + */ + public function flushCache() + { + $this->aliasIteratorCache = []; + $this->aliasReferenceCache = []; + } + /** * Return all places an alias seems to be used * @param string $name alias name @@ -189,13 +203,31 @@ class Alias extends BaseModel } } - public function getByName($name) + public function getByName($name, $flush = false) { - foreach ($this->aliases->alias->iterateItems() as $alias) { - if ((string)$alias->name == $name) { + if ($flush) { + // Flush false negative results (searched earlier, didn't exist at the time) as positive matches will + // always be resolved. + $this->aliasReferenceCache = []; + } + // cache alias uuid references, but always validate existence before return. + if (isset($this->aliasReferenceCache[$name])) { + $uuid = $this->aliasReferenceCache[$name]; + if ($uuid === false) { + return null; + } + $alias = $this->getNodeByReference("aliases.alias.".$uuid); + if ($alias != null && (string)$alias->name == $name) { return $alias; } } + foreach ($this->aliases->alias->iterateItems() as $uuid => $alias) { + if ((string)$alias->name == $name) { + $this->aliasReferenceCache[$name] = $uuid; + return $alias; + } + } + $this->aliasReferenceCache[$name] = false; return null; } }