From c2f407ca4c2dd794a0a2fc504b53147daa9d2cf8 Mon Sep 17 00:00:00 2001
From: Ad Schellevis <ad@opnsense.org>
Date: Tue, 9 May 2023 16:57:48 +0200
Subject: [PATCH] Interfaces: Virtual IPs: Settings - Improve address cleanup
 so modifications to VIPs are less likely to end up with multiple interfaces
 using the same address.

With the previous code it was quite easy to move an address to another interface after which that address was configured on both (new and old).
---
 .../Interfaces/Api/VipSettingsController.php   | 18 +++++++++++++-----
 .../scripts/interfaces/reconfigure_vips.php    | 13 +++++++------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/src/opnsense/mvc/app/controllers/OPNsense/Interfaces/Api/VipSettingsController.php b/src/opnsense/mvc/app/controllers/OPNsense/Interfaces/Api/VipSettingsController.php
index 2bfe55c70..553176c1c 100644
--- a/src/opnsense/mvc/app/controllers/OPNsense/Interfaces/Api/VipSettingsController.php
+++ b/src/opnsense/mvc/app/controllers/OPNsense/Interfaces/Api/VipSettingsController.php
@@ -140,7 +140,14 @@ class VipSettingsController extends ApiMutableModelControllerBase
     {
         $node = $this->getModel()->getNodeByReference('vip.' . $uuid);
         $validations = [];
-        if ($node != null && explode('/', $_POST['vip']['network'])[0] != (string)$node->subnet) {
+        $post_subnet = '';
+        $post_interface = '';
+        if (isset($_POST['vip'])) {
+            $post_subnet = !empty($_POST['vip']['network']) ? explode('/', $_POST['vip']['network'])[0] : '';
+            $post_interface = !empty($_POST['vip']['interface']) ? $_POST['vip']['interface'] : '';
+        }
+
+        if ($node != null && $post_subnet != (string)$node->subnet) {
             $validations = $this->getModel()->whereUsed((string)$node->subnet);
             if (!empty($validations)) {
                 // XXX a bit unpractical, but we can not validate previous values from the model so
@@ -151,10 +158,11 @@ class VipSettingsController extends ApiMutableModelControllerBase
                         'vip.network' => array_slice($validations, 0, 2)
                     ]
                 ];
-            } elseif (!file_exists("/tmp/delete_vip_{$uuid}.todo")) {
-                file_put_contents("/tmp/delete_vip_{$uuid}.todo", (string)$node->subnet);
             }
         }
+        if ($node != null && ($post_subnet != (string)$node->subnet || $post_interface != (string)$node->interface)) {
+            file_put_contents("/tmp/delete_vip_{$uuid}.todo", (string)$node->subnet . "\n", FILE_APPEND);
+        }
 
         return $this->handleFormValidations($this->setBase('vip', 'vip', $uuid, $this->getVipOverlay()));
     }
@@ -186,8 +194,8 @@ class VipSettingsController extends ApiMutableModelControllerBase
             throw new UserException(implode('<br/>', array_slice($validations, 0, 5)), gettext("Item in use by"));
         }
         $response = $this->delBase("vip", $uuid);
-        if (($response['result'] ?? '') == 'deleted' && !file_exists("/tmp/delete_vip_{$uuid}.todo")) {
-            file_put_contents("/tmp/delete_vip_{$uuid}.todo", (string)$node->subnet);
+        if (($response['result'] ?? '') == 'deleted') {
+            file_put_contents("/tmp/delete_vip_{$uuid}.todo", (string)$node->subnet . "\n", FILE_APPEND);
         }
         return $response;
     }
diff --git a/src/opnsense/scripts/interfaces/reconfigure_vips.php b/src/opnsense/scripts/interfaces/reconfigure_vips.php
index 65cc5293c..3a49e14b1 100755
--- a/src/opnsense/scripts/interfaces/reconfigure_vips.php
+++ b/src/opnsense/scripts/interfaces/reconfigure_vips.php
@@ -60,12 +60,13 @@ foreach (legacy_interfaces_details() as $ifname => $ifcnf) {
 
 // remove deleted vips
 foreach (glob("/tmp/delete_vip_*.todo") as $filename) {
-    $address = trim(file_get_contents($filename));
-    if (isset($addresses[$address])) {
-        legacy_interface_deladdress($addresses[$address]['if'], $address, is_ipaddrv6($address) ? 6 : 4);
-    } else {
-        // not found, likely proxy arp
-        $anyproxyarp = true;
+    foreach (array_unique(explode("\n", trim(file_get_contents($filename)))) as $address) {
+        if (isset($addresses[$address])) {
+            legacy_interface_deladdress($addresses[$address]['if'], $address, is_ipaddrv6($address) ? 6 : 4);
+        } else {
+            // not found, likely proxy arp
+            $anyproxyarp = true;
+        }
     }
     unlink($filename);
 }