[PATCH 0/8] alfred: vis: random fixes

[PATCH 0/8] alfred: vis: 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.

This time, we will only handle the vis changes. 

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Sven Eckelmann (8):
      alfred: vis: reject too long unix socket paths
      alfred: vis: keep interface names always in heap
      alfred: vis: return query_rtnl_link error on failed send
      alfred: vis: handle short reads on the unix socket
      alfred: vis: report failures while reading the answer
      alfred: vis: don't drop the rest of the answer for a bogus dataset
      alfred: vis: fix jsondoc output for nodes without vis entries
      alfred: vis: skip originators with unresolvable hard interface

 vis/vis.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++-------------
 vis/vis.h |   2 +-
 2 files changed, 86 insertions(+), 22 deletions(-)
---
base-commit: 462a0cf40615f62e0912c36d72a75accbb26f5ee
change-id: 20260728-bugfixes-vis-002f70769af9

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

[PATCH 1/8] alfred: vis: reject too long unix socket paths

[Sven Eckelmann]

The unix socket path needs to be stored in sockaddr_un::sun_path. But
strncpy() is silently truncating the string when the target buffer is too
small.

Reject such paths with a clear error message.

Fixes: 6c6a6f735054 ("alfred: Make unix socket path configurable")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 vis/vis.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/vis/vis.c b/vis/vis.c
index 4642227..d635798 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -152,6 +152,12 @@ static int alfred_open_sock(struct globals *globals)
 {
 	struct sockaddr_un addr;
 
+	if (strlen(globals->unix_path) >= sizeof(addr.sun_path)) {
+		fprintf(stderr, "unix socket path too long\n");
+		globals->unix_sock = -1;
+		return -1;
+	}
+
 	globals->unix_sock = socket(AF_LOCAL, SOCK_STREAM, 0);
 	if (globals->unix_sock < 0) {
 		perror("can't create unix socket");

-- 
2.47.3

[PATCH 2/8] alfred: vis: keep interface names always in heap

[Sven Eckelmann]

The interface pointer in globals was used to store an address in .rodata or
in on the heap. This makes the handling of the allocation/deallocation
hard. At the same time, nothing checked for allocations problems. An
allocation error could cause an hard-to-debug problem later when trying to
calculate the ifindex.

Adjust the code to always allocate the string as an heap object to simplify
the handling.

Fixes: 2e3e312f7757 ("alfred: add vis")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 vis/vis.c | 12 +++++++++++-
 vis/vis.h |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/vis/vis.c b/vis/vis.c
index d635798..97c6093 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -1058,10 +1058,15 @@ static struct globals *vis_init(int argc, char *argv[])
 	memset(globals, 0, sizeof(*globals));
 
 	globals->opmode = OPMODE_CLIENT;
-	globals->interface = "bat0";
 	globals->vis_format = FORMAT_DOT;
 	globals->unix_path = ALFRED_SOCK_PATH_DEFAULT;
 
+	globals->interface = strdup("bat0");
+	if (!globals->interface) {
+		perror("strdup");
+		return NULL;
+	}
+
 	while ((opt = getopt_long(argc, argv, "shf:i:vu:", long_options,
 				  &opt_ind)) != -1) {
 		switch (opt) {
@@ -1081,7 +1086,12 @@ static struct globals *vis_init(int argc, char *argv[])
 			}
 			break;
 		case 'i':
+			free(globals->interface);
 			globals->interface = strdup(optarg);
+			if (!globals->interface) {
+				perror("strdup");
+				return NULL;
+			}
 			break;
 		case 'u':
 			globals->unix_path = optarg;
diff --git a/vis/vis.h b/vis/vis.h
index e257c45..d3486f9 100644
--- a/vis/vis.h
+++ b/vis/vis.h
@@ -73,7 +73,7 @@ struct vis_list_entry {
 	 (vis_data)->entries_n * sizeof(struct vis_entry))
 
 struct globals {
-	const char *interface;
+	char *interface;
 	enum opmode opmode;
 	enum vis_format vis_format;
 	uint8_t buf[65536];

-- 
2.47.3

[PATCH 3/8] alfred: vis: return query_rtnl_link error on failed send

[Sven Eckelmann]

When nl_send_auto_complete() failed, query_rtnl_link() jumped to the
cleanup path without setting err. The function then returned 0 and the
caller assumed a successful interface query even though no request was ever
sent to the kernel.

Handle this as EIO error because nl_send_auto_complete uses an internal
error code which cannot be translated easily in errno codes.

Fixes: a34f044de561 ("alfred: vis: Use rtnl to query list of hardifs of meshif")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 vis/vis.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/vis/vis.c b/vis/vis.c
index 97c6093..d529583 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -316,8 +316,10 @@ static int query_rtnl_link(int ifindex, nl_recvmsg_msg_cb_t func, void *arg)
 	}
 
 	ret = nl_send_auto_complete(sock, msg);
-	if (ret < 0)
+	if (ret < 0) {
+		err = -EIO;
 		goto err_free_msg;
+	}
 
 	nl_recvmsgs(sock, cb);
 

-- 
2.47.3

[PATCH 4/8] alfred: vis: handle short reads on the unix socket

[Sven Eckelmann]

The answer of the alfred daemon is received over a SOCK_STREAM unix socket
which doesn't preserve message boundaries. read() can therefore return less
than the requested number of bytes. Such a short read was treated like an
end of stream, the current packet was dropped and the answer loop
terminated.

Retry the read until the requested amount of data, end of stream or a real
error was received (ignoring temporary interruptions EINTR).

Fixes: 2e3e312f7757 ("alfred: add vis")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 vis/vis.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/vis/vis.c b/vis/vis.c
index d529583..33fa692 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -694,6 +694,29 @@ static int vis_request_data(struct globals *globals)
 	return globals->unix_sock;
 }
 
+static ssize_t read_full(int fd, void *buf, size_t count)
+{
+	size_t read_len = 0;
+	uint8_t *pos = buf;
+	ssize_t ret;
+
+	while (read_len < count) {
+		ret = read(fd, pos + read_len, count - read_len);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+
+			return ret;
+		}
+
+		if (ret == 0)
+			break;
+
+		read_len += ret;
+	}
+
+	return read_len;
+}
 
 static struct vis_v1 *vis_receive_answer_packet(int sock, uint16_t *len)
 {
@@ -704,11 +727,11 @@ static struct vis_v1 *vis_receive_answer_packet(int sock, uint16_t *len)
 	int ret;
 	int l;
 
-	ret = read(sock, buf, sizeof(*tlv));
+	ret = read_full(sock, buf, sizeof(*tlv));
 	if (ret < 0)
 		return NULL;
 
-	if (ret < (int)sizeof(*tlv)) 
+	if (ret < (int)sizeof(*tlv))
 		return NULL;
 
 	tlv = (struct alfred_tlv *)buf;
@@ -726,7 +749,7 @@ static struct vis_v1 *vis_receive_answer_packet(int sock, uint16_t *len)
 		return NULL;
 
 	/* read the rest of the packet */
-	ret = read(sock, buf + sizeof(*tlv), l);
+	ret = read_full(sock, buf + sizeof(*tlv), l);
 	if (ret < l)
 		return NULL;
 

-- 
2.47.3

[PATCH 5/8] alfred: vis: report failures while reading the answer

[Sven Eckelmann]

vis_read_answer() returns NULL for a:

* closed connection
* read error
* malformed packet
* unexpected answer

All of them were indistinguishable from the latter regular end of the
answer. Even on errors batadv-vis printed an empty (well formed document).
An error could not directly be detected by the return code.

Report the error state of the packet parsing separately and use it for the
exit code.

Fixes: 2e3e312f7757 ("alfred: add vis")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 vis/vis.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/vis/vis.c b/vis/vis.c
index 33fa692..451d79c 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -718,7 +718,8 @@ static ssize_t read_full(int fd, void *buf, size_t count)
 	return read_len;
 }
 
-static struct vis_v1 *vis_receive_answer_packet(int sock, uint16_t *len)
+static struct vis_v1 *vis_receive_answer_packet(int sock, uint16_t *len,
+						int *error)
 {
 	struct alfred_push_data_v0 *push;
 	static uint8_t buf[65536];
@@ -727,15 +728,29 @@ static struct vis_v1 *vis_receive_answer_packet(int sock, uint16_t *len)
 	int ret;
 	int l;
 
+	*error = -1;
+
 	ret = read_full(sock, buf, sizeof(*tlv));
-	if (ret < 0)
+	if (ret < 0) {
+		perror("read from unix socket failed");
 		return NULL;
+	}
+
+	/* end of the answer */
+	if (ret == 0) {
+		*error = 0;
+		return NULL;
+	}
 
 	if (ret < (int)sizeof(*tlv))
 		return NULL;
 
 	tlv = (struct alfred_tlv *)buf;
-	/* TODO: might return an ALFRED_STATUS_ERROR too, handle it */
+	if (tlv->type == ALFRED_STATUS_ERROR) {
+		fprintf(stderr, "Request of vis data failed\n");
+		return NULL;
+	}
+
 	if (tlv->type != ALFRED_PUSH_DATA)
 		return NULL;
 
@@ -763,6 +778,8 @@ static struct vis_v1 *vis_receive_answer_packet(int sock, uint16_t *len)
 	if (data->header.version != VIS_PACKETVERSION)
 		return NULL;
 
+	*error = 0;
+
 	return (struct vis_v1 *) data->data;
 }
 
@@ -995,6 +1012,7 @@ static int vis_read_answer(struct globals *globals)
 	struct vis_iface *ifaces;
 	struct vis_v1 *vis_data;
 	uint16_t len;
+	int ret = 0;
 
 	switch (globals->vis_format) {
 	case FORMAT_DOT:
@@ -1013,7 +1031,7 @@ static int vis_read_answer(struct globals *globals)
 	ops->preamble();
 
 	while ((vis_data =
-		vis_receive_answer_packet(globals->unix_sock, &len)) != NULL) {
+		vis_receive_answer_packet(globals->unix_sock, &len, &ret)) != NULL) {
 		if (len < sizeof(*vis_data))
 			return -1;
 
@@ -1037,19 +1055,21 @@ static int vis_read_answer(struct globals *globals)
 	}
 	ops->postamble();
 
-	return 0;
+	return ret;
 }
 
 static int vis_get_data(struct globals *globals)
 {
+	int ret;
+
 	globals->unix_sock = vis_request_data(globals);
 	if (globals->unix_sock < 0)
 		return -1;
 
-	vis_read_answer(globals);
+	ret = vis_read_answer(globals);
 	close(globals->unix_sock);
 
-	return 0;
+	return ret;
 }
 
 static void vis_usage(void)
@@ -1176,8 +1196,10 @@ int main(int argc, char *argv[])
 		return vis_server(globals);
 		break;
 	case OPMODE_CLIENT:
-		return vis_get_data(globals);
-		break;
+		if (vis_get_data(globals) < 0)
+			return 1;
+
+		return 0;
 	}
 
 	return 0;

-- 
2.47.3

[PATCH 6/8] alfred: vis: don't drop the rest of the answer for a bogus dataset

[Sven Eckelmann]

A dataset which is too short to even hold the length of the vis_data
(header) aborted the processing of the answer. The positions of all
following nodes were dropped from the printed document although only the
dataset of a single node was malformed.

This skipped the ops->postamble() call. For the jsondoc/dot format, the
syntax was wrong because of this, the output was never finished.

Skip such a bogus datasets instead and process the rest of the answers.

Fixes: 38eae4d50e81 ("vis: Add an output format which is json conformant.")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 vis/vis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vis/vis.c b/vis/vis.c
index 451d79c..93f444b 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -1033,7 +1033,7 @@ static int vis_read_answer(struct globals *globals)
 	while ((vis_data =
 		vis_receive_answer_packet(globals->unix_sock, &len, &ret)) != NULL) {
 		if (len < sizeof(*vis_data))
-			return -1;
+			continue;
 
 		/* check size and skip bogus packets */
 		if (len != VIS_DATA_SIZE(vis_data))

-- 
2.47.3

[PATCH 7/8] alfred: vis: fix jsondoc output for nodes without vis entries

[Sven Eckelmann]

vis_read_answer() skipped the entries handler when a received vis record
contained no entries. For the jsondoc format, the per-node object is opened
by vis_jsondoc_interfaces() but only closed at the end of
vis_jsondoc_entries().

The entries handler must always be called and all entry handlers must
handled `entries_n == 0`.

Fixes: 38eae4d50e81 ("vis: Add an output format which is json conformant.")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 vis/vis.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/vis/vis.c b/vis/vis.c
index 93f444b..025745d 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -1046,10 +1046,6 @@ static int vis_read_answer(struct globals *globals)
 		vis_entries = (struct vis_entry *) &ifaces[vis_data->iface_n];
 
 		ops->interfaces(vis_data->iface_n, ifaces);
-
-		if (vis_data->entries_n == 0)
-			continue;
-
 		ops->entries(vis_data->entries_n, vis_entries,
 			     vis_data->iface_n, ifaces);
 	}

-- 
2.47.3

[PATCH 8/8] alfred: vis: skip originators with unresolvable hard interface

[Sven Eckelmann]

get_if_index_devindex() returns -1 on an error. But the result can also be
larger than 255. In both cases, the originator cannot be reported using the
vis_v1 type which is using an u8 for the ifindex storage.

The index must be prechecked before the entry is allocated. The value 255
must also be avoided because it used as marker for TT entries.

Fixes: bca55a86fecd ("alfred: vis: Add support for netlink")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 vis/vis.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/vis/vis.c b/vis/vis.c
index 025745d..b7a50af 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -531,6 +531,7 @@ static int parse_orig_list_netlink_cb(struct nl_msg *msg, void *arg)
 	uint32_t hardif;
 	uint8_t *neigh;
 	uint8_t *orig;
+	int ifindex;
 	uint8_t tq;
 
 	opts = container_of(query_opts, struct vis_netlink_opts,
@@ -567,12 +568,16 @@ static int parse_orig_list_netlink_cb(struct nl_msg *msg, void *arg)
 	if (memcmp(orig, neigh, ETH_ALEN) != 0)
 		return NL_OK;
 
+	ifindex = get_if_index_devindex(opts->globals, hardif);
+	if (ifindex < 0 || ifindex >= 255)
+		return NL_OK;
+
 	v_entry = malloc(sizeof(*v_entry));
 	if (!v_entry)
 		return NL_OK;
 
 	memcpy(v_entry->v.mac, orig, ETH_ALEN);
-	v_entry->v.ifindex = get_if_index_devindex(opts->globals, hardif);
+	v_entry->v.ifindex = ifindex;
 	v_entry->v.qual = tq;
 	list_add_tail(&v_entry->list, &opts->globals->entry_list);
 

-- 
2.47.3