[PATCH 0/6] alfred: interfaces: random fixes

[PATCH 0/6] alfred: interfaces: random fixes

[Sven Eckelmann]

Similar to the batctl fixes, I have now compiled a list of potential fixes
for alfred. Instead of posting everything at once, I will be posted in
smaller chunks. They are just various (mostly minor) problems which should
be tackled at some point.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Sven Eckelmann (6):
      alfred: fix off-by-one in client interface name length checks
      alfred: update stored interface list on interface change
      alfred: don't drop the mesh interface name on OOM
      alfred: keep the running interfaces when reconfiguration fails
      alfred: Report failure when the client refuses to change interfaces
      alfred: don't check the "none" on network interface change

 client.c    | 31 ++++++++++++++++++++-----------
 netsock.c   | 41 +++++++++++++++++++++++++++--------------
 unix_sock.c | 22 +++++++++++++++++++---
 3 files changed, 66 insertions(+), 28 deletions(-)
---
base-commit: 462a0cf40615f62e0912c36d72a75accbb26f5ee
change-id: 20260731-bugfixes-interfaces-ca479c0fa5b5

Best regards,
--  
Sven Eckelmann <sven@narfation.org>

[PATCH 1/6] alfred: fix off-by-one in client interface name length checks

[Sven Eckelmann]

check_interface(), alfred_client_change_interface() and
alfred_client_change_bat_iface() rejected names only when they were
strictly longer than the destination buffer. A name whose length equals the
buffer size passed the check, but the subsequent strncpy() then filled the
buffer without a terminator and the forced '\0' at the last position
silently dropped the final character.

Reject names which don't fit including their terminating \0 byte.

Fixes: babd772a36e1 ("alfred: support for changing interfaces")
Fixes: b96cc742ef3e ("alfred: introduce 'change batman-adv interface' IPC call")
Fixes: 67ae5f57eedd ("alfred: Add support for multiple interfaces per master")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 client.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/client.c b/client.c
index 9fa9f00..f56042a 100644
--- a/client.c
+++ b/client.c
@@ -222,7 +222,7 @@ static int check_interface(const char *iface)
 	struct ifreq ifr;
 	int sock = -1;
 
-	if (strlen(iface) > IFNAMSIZ) {
+	if (strlen(iface) >= IFNAMSIZ) {
 		fprintf(stderr, "%s: interface name list too long, not changing\n",
 			__func__);
 		return -1;
@@ -262,7 +262,7 @@ int alfred_client_change_interface(struct globals *globals)
 		return -1;
 
 	interface_len = strlen(globals->net_iface);
-	if (interface_len > sizeof(change_interface.ifaces)) {
+	if (interface_len >= sizeof(change_interface.ifaces)) {
 		fprintf(stderr, "%s: interface name list too long, not changing\n",
 			__func__);
 		return 0;
@@ -311,7 +311,7 @@ int alfred_client_change_bat_iface(struct globals *globals)
 		return -1;
 
 	interface_len = strlen(globals->mesh_iface);
-	if (interface_len > sizeof(change_bat_iface.bat_iface)) {
+	if (interface_len >= sizeof(change_bat_iface.bat_iface)) {
 		fprintf(stderr, "%s: batman-adv interface name list too long, not changing\n",
 			__func__);
 		return 0;

-- 
2.47.3

[PATCH 2/6] alfred: update stored interface list on interface change

[Sven Eckelmann]

The interface list received via ALFRED_CHANGE_INTERFACE was passed to
netsock_set_interfaces() but globals->net_iface kept the value from the
daemon start. But the stale string is used by unix_sock_add_data() to
decide whether interface operations are disabled ('none'). And for the none
interface, it is required to be in primary mode and to prefill the
data->source address.

Store a copy of the new interface list in globals->net_iface to dynamically
switch between the disabled/not disabled mode.

Fixes: 94b3c4b5744f ("alfred: receive data with valid source on unix sock without active interface")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 unix_sock.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/unix_sock.c b/unix_sock.c
index e6b44da..64fde70 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -365,6 +365,7 @@ unix_sock_change_iface(struct globals *globals,
 		       struct alfred_change_interface_v0 *change_iface,
 		       int client_sock)
 {
+	char *net_iface;
 	int ret = -1;
 	int len;
 
@@ -383,6 +384,13 @@ unix_sock_change_iface(struct globals *globals,
 		}
 	}
 
+	net_iface = strdup(change_iface->ifaces);
+	if (!net_iface)
+		goto err;
+
+	free(globals->net_iface);
+	globals->net_iface = net_iface;
+
 	netsock_set_interfaces(globals, change_iface->ifaces);
 
 	ret = 0;

-- 
2.47.3

[PATCH 3/6] alfred: don't drop the mesh interface name on OOM

[Sven Eckelmann]

unix_sock_change_bat_iface() freed the old mesh_iface and then assigned
the result of strdup() directly to globals->mesh_iface. When strdup()
failed the old name was already gone and mesh_iface was left NULL. But it
still reported a success success.

Duplicate the name into a temporary first and only replace the stored
pointer once the allocation succeeded.

Fixes: b96cc742ef3e ("alfred: introduce 'change batman-adv interface' IPC call")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 unix_sock.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/unix_sock.c b/unix_sock.c
index 64fde70..417b380 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -404,6 +404,7 @@ unix_sock_change_bat_iface(struct globals *globals,
 			   struct alfred_change_bat_iface_v0 *change_bat_iface,
 			   int client_sock)
 {
+	char *mesh_iface;
 	int ret = -1;
 	int len;
 
@@ -412,9 +413,13 @@ unix_sock_change_bat_iface(struct globals *globals,
 	if (len < (int)(sizeof(*change_bat_iface) - sizeof(change_bat_iface->header)))
 		goto err;
 
-	free(globals->mesh_iface);
 	change_bat_iface->bat_iface[sizeof(change_bat_iface->bat_iface) - 1] = '\0';
-	globals->mesh_iface = strdup(change_bat_iface->bat_iface);
+	mesh_iface = strdup(change_bat_iface->bat_iface);
+	if (!mesh_iface)
+		goto err;
+
+	free(globals->mesh_iface);
+	globals->mesh_iface = mesh_iface;
 
 	ret = 0;
 err:

-- 
2.47.3

[PATCH 4/6] alfred: keep the running interfaces when reconfiguration fails

[Sven Eckelmann]

netsock_set_interfaces() tore down the existing interface list with
netsock_close_all() before building the new one. When an allocation failed
while building the replacement it called netsock_close_all() again and
returned -ENOMEM, leaving the daemon with no interfaces at all.

Build the replacement into a temporary list first and only swap it in once
it is complete. On failure the temporary list is discarded and the running
interfaces are kept intact.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 netsock.c   | 41 +++++++++++++++++++++++++++--------------
 unix_sock.c |  7 +++++--
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/netsock.c b/netsock.c
index 60b2285..3505041 100644
--- a/netsock.c
+++ b/netsock.c
@@ -100,12 +100,12 @@ struct interface *netsock_first_interface(struct globals *globals)
 	return NULL;
 }
 
-static struct interface *netsock_find_interface(struct globals *globals,
+static struct interface *netsock_find_interface(struct list_head *interfaces,
 						const char *name)
 {
 	struct interface *interface;
 
-	list_for_each_entry(interface, &globals->interfaces, list) {
+	list_for_each_entry(interface, interfaces, list) {
 		if (strcmp(name, interface->interface) == 0)
 			return interface;
 	}
@@ -115,30 +115,32 @@ static struct interface *netsock_find_interface(struct globals *globals,
 
 int netsock_set_interfaces(struct globals *globals, char *interfaces)
 {
+	struct list_head new_interfaces;
 	struct interface *interface;
+	struct interface *is;
 	char *saveptr;
 	char *input;
 	char *token;
 
-	netsock_close_all(globals);
+	INIT_LIST_HEAD(&new_interfaces);
 
 	/* interface 'none' disables all interface operations */
-	if (is_iface_disabled(interfaces))
+	if (is_iface_disabled(interfaces)) {
+		netsock_close_all(globals);
 		return 0;
+	}
 
 	input = interfaces;
 	while ((token = strtok_r(input, ",", &saveptr))) {
 		input = NULL;
 
-		interface = netsock_find_interface(globals, token);
+		interface = netsock_find_interface(&new_interfaces, token);
 		if (interface)
 			continue;
 
 		interface = malloc(sizeof(*interface));
-		if (!interface) {
-			netsock_close_all(globals);
-			return -ENOMEM;
-		}
+		if (!interface)
+			goto err;
 
 		memset(&interface->hwaddr, 0, sizeof(interface->hwaddr));
 		memset(&interface->address, 0, sizeof(interface->address));
@@ -151,8 +153,7 @@ int netsock_set_interfaces(struct globals *globals, char *interfaces)
 		interface->interface = strdup(token);
 		if (!interface->interface) {
 			free(interface);
-			netsock_close_all(globals);
-			return -ENOMEM;
+			goto err;
 		}
 
 		interface->server_hash = hash_new(64, server_compare,
@@ -160,14 +161,26 @@ int netsock_set_interfaces(struct globals *globals, char *interfaces)
 		if (!interface->server_hash) {
 			free(interface->interface);
 			free(interface);
-			netsock_close_all(globals);
-			return -ENOMEM;
+			goto err;
 		}
 
-		list_add_tail(&interface->list, &globals->interfaces);
+		list_add_tail(&interface->list, &new_interfaces);
 	}
 
+	netsock_close_all(globals);
+	list_splice_tail(&new_interfaces, &globals->interfaces);
+
 	return 0;
+
+err:
+	list_for_each_entry_safe(interface, is, &new_interfaces, list) {
+		list_del(&interface->list);
+		hash_delete(interface->server_hash, free);
+		free(interface->interface);
+		free(interface);
+	}
+
+	return -ENOMEM;
 }
 
 static int enable_raw_bind_capability(int enable)
diff --git a/unix_sock.c b/unix_sock.c
index 417b380..d0e82dc 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -388,11 +388,14 @@ unix_sock_change_iface(struct globals *globals,
 	if (!net_iface)
 		goto err;
 
+	if (netsock_set_interfaces(globals, change_iface->ifaces) < 0) {
+		free(net_iface);
+		goto err;
+	}
+
 	free(globals->net_iface);
 	globals->net_iface = net_iface;
 
-	netsock_set_interfaces(globals, change_iface->ifaces);
-
 	ret = 0;
 err:
 	close(client_sock);

-- 
2.47.3

[PATCH 5/6] alfred: Report failure when the client refuses to change interfaces

[Sven Eckelmann]

alfred_client_change_interface() and alfred_client_change_bat_iface() open
the connection to the daemon and then an interface that fails the local
checks printed a warning but returned 0. That reported success to the
caller even though nothing was changed and left the connected unix socket
open.

Close the socket and return -1 on these paths so the exit status
reflects that the requested change was rejected.

Fixes: babd772a36e1 ("alfred: support for changing interfaces")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 client.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/client.c b/client.c
index f56042a..d25e3bb 100644
--- a/client.c
+++ b/client.c
@@ -265,7 +265,8 @@ int alfred_client_change_interface(struct globals *globals)
 	if (interface_len >= sizeof(change_interface.ifaces)) {
 		fprintf(stderr, "%s: interface name list too long, not changing\n",
 			__func__);
-		return 0;
+		unix_sock_close(globals);
+		return -1;
 	}
 
 	len = sizeof(change_interface);
@@ -286,8 +287,10 @@ int alfred_client_change_interface(struct globals *globals)
 		input = NULL;
 
 		ret = check_interface(token);
-		if (ret < 0)
-			return 0;
+		if (ret < 0) {
+			unix_sock_close(globals);
+			return -1;
+		}
 	}
 
 	ret = write(globals->unix_sock, &change_interface, len);
@@ -314,7 +317,8 @@ int alfred_client_change_bat_iface(struct globals *globals)
 	if (interface_len >= sizeof(change_bat_iface.bat_iface)) {
 		fprintf(stderr, "%s: batman-adv interface name list too long, not changing\n",
 			__func__);
-		return 0;
+		unix_sock_close(globals);
+		return -1;
 	}
 
 	len = sizeof(change_bat_iface);

-- 
2.47.3

[PATCH 6/6] alfred: don't check the "none" on network interface change

[Sven Eckelmann]

The interface name 'none' disables all network interface operations of the
server. It is accepted by the -i parameter and by netsock_set_interfaces()
but the client tested every name of the -I list before sending the request.
The lookup of 'none' always fails with ENODEV and thus the request was
never sent.

Skip the interface check for the sentinel to ensure the network interface
can also be disabled at runtime.

Fixes: 2cca53171180 ("alfred: Allow operating without any interface specified")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 client.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/client.c b/client.c
index d25e3bb..06de0c4 100644
--- a/client.c
+++ b/client.c
@@ -281,15 +281,20 @@ int alfred_client_change_interface(struct globals *globals)
 	/* test it before sending
 	 * globals->net_iface is now saved in change_interface.ifaces
 	 * and can be modified by strtok_r
+	 *
+	 * the interface 'none' disables all interface operations and is not
+	 * an interface which could be looked up
 	 */
-	input = globals->net_iface;
-	while ((token = strtok_r(input, ",", &saveptr))) {
-		input = NULL;
+	if (!is_iface_disabled(globals->net_iface)) {
+		input = globals->net_iface;
+		while ((token = strtok_r(input, ",", &saveptr))) {
+			input = NULL;
 
-		ret = check_interface(token);
-		if (ret < 0) {
-			unix_sock_close(globals);
-			return -1;
+			ret = check_interface(token);
+			if (ret < 0) {
+				unix_sock_close(globals);
+				return -1;
+			}
 		}
 	}
 

-- 
2.47.3