As discussed on our call, the current line_read() implementation is pretty ugly and has some definite bugs. This series replaces it with a new implementation which should be more solid and more readable. The new implementation is larger than I'd really like, but it turns out its fiddlier to handle the various edge cases than you might think. David Gibson (4): Add cleaner line-by-line reading primitives Parse resolv.conf with new lineread implementation Use new lineread implementation for procfs_scan_listen() Remove unused line_read() Makefile | 8 ++-- conf.c | 22 ++++++---- lineread.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++ lineread.h | 31 +++++++++++++++ util.c | 64 +++-------------------------- 5 files changed, 170 insertions(+), 70 deletions(-) create mode 100644 lineread.c create mode 100644 lineread.h -- 2.36.1
Two places in passt need to read files line by line (one parsing
resolv.conf, the other parsing /proc/net/*. They can't use fgets()
because in glibc that can allocate memory. Instead they use an
implementation line_read() in util.c. This has some problems:
* It has two completely separate modes of operation, one buffering
and one not, the relation between these and how they're activated
is subtle and confusing
* At least in non-buffered mode, it will mishandle an empty line,
folding them onto the start of the next non-empty line
* In non-buffered mode it will use lseek() which prevents using this
on non-regular files (we don't need that at present, but it's a
surprising limitation)
* It has a lot of difficult to read pointer mangling
Add a new cleaner implementation of allocation-free line-by-line
reading in lineread.c. This one always buffers, using a state
structure to keep track of what we need. This is larger than I'd
like, but it turns out handling all the edge cases of line-by-line
reading in C is surprisingly hard.
This just adds the code, subsequent patches will change the existing
users of line_read() to the new implementation.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
Makefile | 8 ++--
lineread.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
lineread.h | 31 +++++++++++++++
3 files changed, 150 insertions(+), 4 deletions(-)
create mode 100644 lineread.c
create mode 100644 lineread.h
diff --git a/Makefile b/Makefile
index d2e25b4..c441af3 100644
--- a/Makefile
+++ b/Makefile
@@ -32,16 +32,16 @@ CFLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL)
CFLAGS += -DARCH=\"$(TARGET_ARCH)\"
PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.c \
- mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c siphash.c \
- tap.c tcp.c tcp_splice.c udp.c util.c
+ lineread.c mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c \
+ siphash.c tap.c tcp.c tcp_splice.c udp.c util.c
QRAP_SRCS = qrap.c
SRCS = $(PASST_SRCS) $(QRAP_SRCS)
MANPAGES = passt.1 pasta.1 qrap.1
PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \
- ndp.h netlink.h packet.h passt.h pasta.h pcap.h siphash.h \
- tap.h tcp.h tcp_splice.h udp.h util.h
+ lineread.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h \
+ siphash.h tap.h tcp.h tcp_splice.h udp.h util.h
HEADERS = $(PASST_HEADERS)
# On gcc 11.2, with -O2 and -flto, tcp_hash() and siphash_20b(), if inlined,
diff --git a/lineread.c b/lineread.c
new file mode 100644
index 0000000..fca7f11
--- /dev/null
+++ b/lineread.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+/* PASST - Plug A Simple Socket Transport
+ * for qemu/UNIX domain socket mode
+ *
+ * PASTA - Pack A Subtle Tap Abstraction
+ * for network namespace/tap device mode
+ *
+ * lineread.c - Allocation free line-by-line buffered file input
+ *
+ * Copyright Red Hat
+ * Author: David Gibson <david(a)gibson.dropbear.id.au>
+ */
+
+#include <stddef.h>
+#include <fcntl.h>
+#include <string.h>
+#include <stdbool.h>
+#include <assert.h>
+#include <unistd.h>
+
+#include "lineread.h"
+
+/**
+ * lineread_init() - Prepare for line by line file reading without allocation
+ * @lr: Line reader state structure to initialize
+ * @fd: File descriptor to read lines from
+ */
+void lineread_init(struct lineread *lr, int fd)
+{
+ lr->fd = fd;
+ lr->next_line = lr->count = 0;
+}
+
+/**
+ * peek_line() - Find and NULL-terminate next line in buffer
+ * @lr: Line reader state structure
+ * @eof: Caller indicates end-of-file was already found by read()
+ *
+ * Return: length of line in bytes, -1 if no line was found
+ */
+static int peek_line(struct lineread *lr, bool eof)
+{
+ char *nl;
+
+ /* Sanity checks (which also document invariants) */
+ assert(lr->count >= 0);
+ assert(lr->next_line >= 0);
+ assert(lr->next_line + lr->count >= lr->next_line);
+ assert(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE);
+
+ nl = memchr(lr->buf + lr->next_line, '\n', lr->count);
+
+ if (nl) {
+ *nl = '\0';
+ return nl - lr->buf - lr->next_line + 1;
+ }
+
+ if (eof) {
+ lr->buf[lr->next_line + lr->count] = '\0';
+ /* No trailing newline, so treat all remaining bytes
+ * as the last line
+ */
+ return lr->count;
+ }
+
+ return -1;
+}
+
+/**
+ * lineread_get() - Read a single line from file (no allocation)
+ * @lr: Line reader state structure
+ * @line: Place a pointer to the next line in this variable
+ *
+ * Return: Length of line read on success, 0 on EOF, negative on error
+ */
+int lineread_get(struct lineread *lr, char **line)
+{
+ bool eof = false;
+ int line_len;
+
+ while ((line_len = peek_line(lr, eof)) < 0) {
+ int rc;
+
+ if ((lr->next_line + lr->count) == LINEREAD_BUFFER_SIZE) {
+ /* No space at end */
+ if (lr->next_line == 0) {
+ /* Buffer is full, which means we've
+ * hit a line too long for us to
+ * process. FIXME: report error
+ * better
+ */
+ return -1;
+ }
+ memmove(lr->buf, lr->buf + lr->next_line, lr->count);
+ lr->next_line = 0;
+ }
+
+ /* Read more data into the end of buffer */
+ rc = read(lr->fd, lr->buf + lr->next_line + lr->count,
+ LINEREAD_BUFFER_SIZE - lr->next_line - lr->count);
+ if (rc < 0) {
+ return rc;
+ } else if (rc == 0) {
+ eof = true;
+ } else {
+ lr->count += rc;
+ }
+ }
+
+ *line = lr->buf + lr->next_line;
+ lr->next_line += line_len;
+ lr->count -= line_len;
+ return line_len;
+}
diff --git a/lineread.h b/lineread.h
new file mode 100644
index 0000000..b6faff5
--- /dev/null
+++ b/lineread.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: AGPL-3.0-or-later
+ * Copyright Red Hat
+ * Author: David Gibson <david(a)gibson.dropbear.id.au>
+ */
+
+#ifndef LINEREAD_H
+#define LINEREAD_H
+
+#define LINEREAD_BUFFER_SIZE 8192
+
+/**
+ * struct lineread - Line reader state
+ * @fd: File descriptor lines are read from
+ * @next_line: Offset in @buf of the start of the first line not yet
+ * returned by lineread_get()
+ * @count: Number of bytes in @buf read from the file, but not yet
+ * returned by lineread_get()
+ * @buf: Buffer storing data read from file.
+ */
+struct lineread {
+ int fd; int next_line;
+ int count;
+
+ /* One extra byte for possible trailing \0 */
+ char buf[LINEREAD_BUFFER_SIZE+1];
+};
+
+void lineread_init(struct lineread *lr, int fd);
+int lineread_get(struct lineread *lr, char **line);
+
+#endif /* _LINEREAD_H */
--
2.36.1
On Fri, 24 Jun 2022 12:17:29 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:[...] +/** + * lineread_get() - Read a single line from file (no allocation) + * @lr: Line reader state structure + * @line: Place a pointer to the next line in this variable + * + * Return: Length of line read on success, 0 on EOF, negative on error + */ +int lineread_get(struct lineread *lr, char **line) +{ + bool eof = false; + int line_len; + + while ((line_len = peek_line(lr, eof)) < 0) { + int rc; + + if ((lr->next_line + lr->count) == LINEREAD_BUFFER_SIZE) { + /* No space at end */ + if (lr->next_line == 0) { + /* Buffer is full, which means we've + * hit a line too long for us to + * process. FIXME: report error + * better + */ + return -1; + } + memmove(lr->buf, lr->buf + lr->next_line, lr->count); + lr->next_line = 0; + } + + /* Read more data into the end of buffer */ + rc = read(lr->fd, lr->buf + lr->next_line + lr->count, + LINEREAD_BUFFER_SIZE - lr->next_line - lr->count); + if (rc < 0) { + return rc; + } else if (rc == 0) {clang-tidy still complains about this one, and I think it's reasonable (though not fundamental) to change it, because (rc < 0) is a clear error condition we're returning right away. I'd just change this on merge. -- Stefano
On Mon, Jun 27, 2022 at 12:36:32PM +0200, Stefano Brivio wrote:On Fri, 24 Jun 2022 12:17:29 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Fair enough. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson[...] +/** + * lineread_get() - Read a single line from file (no allocation) + * @lr: Line reader state structure + * @line: Place a pointer to the next line in this variable + * + * Return: Length of line read on success, 0 on EOF, negative on error + */ +int lineread_get(struct lineread *lr, char **line) +{ + bool eof = false; + int line_len; + + while ((line_len = peek_line(lr, eof)) < 0) { + int rc; + + if ((lr->next_line + lr->count) == LINEREAD_BUFFER_SIZE) { + /* No space at end */ + if (lr->next_line == 0) { + /* Buffer is full, which means we've + * hit a line too long for us to + * process. FIXME: report error + * better + */ + return -1; + } + memmove(lr->buf, lr->buf + lr->next_line, lr->count); + lr->next_line = 0; + } + + /* Read more data into the end of buffer */ + rc = read(lr->fd, lr->buf + lr->next_line + lr->count, + LINEREAD_BUFFER_SIZE - lr->next_line - lr->count); + if (rc < 0) { + return rc; + } else if (rc == 0) {clang-tidy still complains about this one, and I think it's reasonable (though not fundamental) to change it, because (rc < 0) is a clear error condition we're returning right away. I'd just change this on merge.
Switch the resolv.conf parsing in conf.c to use the new lineread
implementation. This means that it can now handle a resolv.conf file which
contains blank lines.
There are quite a few other fragilities with the resolv.conf parsing, but
that's out of scope for this patch.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
conf.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/conf.c b/conf.c
index 3a5d650..22949fd 100644
--- a/conf.c
+++ b/conf.c
@@ -38,6 +38,7 @@
#include "udp.h"
#include "tcp.h"
#include "pasta.h"
+#include "lineread.h"
/**
* get_bound_ports() - Get maps of ports with bound sockets
@@ -320,7 +321,9 @@ static void get_dns(struct ctx *c)
struct in6_addr *dns6 = &c->dns6[0];
struct fqdn *s = c->dns_search;
uint32_t *dns4 = &c->dns4[0];
- char buf[BUFSIZ], *p, *end;
+ struct lineread resolvconf;
+ int line_len;
+ char *line, *p, *end;
dns4_set = !c->v4 || !!*dns4;
dns6_set = !c->v6 || !IN6_IS_ADDR_UNSPECIFIED(dns6);
@@ -333,13 +336,14 @@ static void get_dns(struct ctx *c)
if ((fd = open("/etc/resolv.conf", O_RDONLY | O_CLOEXEC)) < 0)
goto out;
- for (*buf = 0; line_read(buf, BUFSIZ, fd); *buf = 0) {
- if (!dns_set && strstr(buf, "nameserver ") == buf) {
- p = strrchr(buf, ' ');
+ lineread_init(&resolvconf, fd);
+ while ((line_len = lineread_get(&resolvconf, &line)) > 0) {
+ if (!dns_set && strstr(line, "nameserver ") == line) {
+ p = strrchr(line, ' ');
if (!p)
continue;
- end = strpbrk(buf, "%\n");
+ end = strpbrk(line, "%\n");
if (end)
*end = 0;
@@ -356,13 +360,13 @@ static void get_dns(struct ctx *c)
dns6++;
memset(dns6, 0, sizeof(*dns6));
}
- } else if (!dnss_set && strstr(buf, "search ") == buf &&
+ } else if (!dnss_set && strstr(line, "search ") == line &&
s == c->dns_search) {
- end = strpbrk(buf, "\n");
+ end = strpbrk(line, "\n");
if (end)
*end = 0;
- if (!strtok(buf, " \t"))
+ if (!strtok(line, " \t"))
continue;
while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
@@ -374,6 +378,8 @@ static void get_dns(struct ctx *c)
}
}
+ if (line_len < 0)
+ warn("Error reading /etc/resolv.conf: %s", strerror(errno));
close(fd);
out:
--
2.36.1
Use the new more solid implementation of line by line reading for
procfs_scan_listen().
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
util.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/util.c b/util.c
index 7ffd9d1..83729d2 100644
--- a/util.c
+++ b/util.c
@@ -41,6 +41,7 @@
#include "util.h"
#include "passt.h"
#include "packet.h"
+#include "lineread.h"
/* For __openlog() and __setlogmask() wrappers, and passt_vsyslog() */
static int log_mask;
@@ -476,7 +477,8 @@ char *line_read(char *buf, size_t len, int fd)
void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version, int ns,
uint8_t *map, uint8_t *exclude)
{
- char line[BUFSIZ], *path;
+ char *path, *line;
+ struct lineread lr;
unsigned long port;
unsigned int state;
int *fd;
@@ -500,9 +502,9 @@ void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version,
int ns,
else if ((*fd = open(path, O_RDONLY | O_CLOEXEC)) < 0)
return;
- *line = 0;
- line_read(line, sizeof(line), *fd);
- while (line_read(line, sizeof(line), *fd)) {
+ lineread_init(&lr, *fd);
+ lineread_get(&lr, &line); /* throw away header */
+ while (lineread_get(&lr, &line) > 0) {
/* NOLINTNEXTLINE(cert-err34-c): != 2 if conversion fails */
if (sscanf(line, "%*u: %*x:%lx %*x:%*x %x", &port, &state) != 2)
continue;
--
2.36.1
The old, ugly implementation of line_read() is no longer used. Remove it.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
util.c | 54 ------------------------------------------------------
1 file changed, 54 deletions(-)
diff --git a/util.c b/util.c
index 83729d2..98946e4 100644
--- a/util.c
+++ b/util.c
@@ -409,60 +409,6 @@ int bitmap_isset(const uint8_t *map, int bit)
return !!(*word & BITMAP_BIT(bit));
}
-/**
- * line_read() - Similar to fgets(), no heap usage, a file instead of a stream
- * @buf: Read buffer: on non-empty string, use that instead of reading
- * @len: Maximum line length
- * @fd: File descriptor for reading
- *
- * Return: @buf if a line is found, NULL on EOF or error
- */
-char *line_read(char *buf, size_t len, int fd)
-{
- int n, do_read = !*buf;
- char *p;
-
- if (!do_read) {
- char *nl;
-
- buf[len - 1] = 0;
- if (!strlen(buf))
- return NULL;
-
- p = buf + strlen(buf) + 1;
-
- while (*p == '\n' && strlen(p) && (size_t)(p - buf) <
len)
- p++;
-
- if (!(nl = strchr(p, '\n')))
- return NULL;
- *nl = 0;
-
- return memmove(buf, p, len - (p - buf));
- }
-
- n = read(fd, buf, --len);
- if (n <= 0)
- return NULL;
-
- buf[len] = 0;
-
- p = buf;
- while (*p == '\n' && strlen(p) && (size_t)(p - buf) < len)
- p++;
-
- if (!(p = strchr(p, '\n')))
- return buf;
-
- *p = 0;
- if (p == buf)
- return buf;
-
- lseek(fd, (p - buf) - n + 1, SEEK_CUR);
-
- return buf;
-}
-
/**
* procfs_scan_listen() - Set bits for listening TCP or UDP sockets from procfs
* @proto: IPPROTO_TCP or IPPROTO_UDP
--
2.36.1
On Fri, 24 Jun 2022 12:17:28 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:As discussed on our call, the current line_read() implementation is pretty ugly and has some definite bugs. This series replaces it with a new implementation which should be more solid and more readable. The new implementation is larger than I'd really like, but it turns out its fiddlier to handle the various edge cases than you might think. David Gibson (4): Add cleaner line-by-line reading primitives Parse resolv.conf with new lineread implementation Use new lineread implementation for procfs_scan_listen() Remove unused line_read() Makefile | 8 ++-- conf.c | 22 ++++++---- lineread.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++ lineread.h | 31 +++++++++++++++ util.c | 64 +++-------------------------- 5 files changed, 170 insertions(+), 70 deletions(-) create mode 100644 lineread.c create mode 100644 lineread.hApplied, finally, sorry for the delay! -- Stefano