From 6834db53b239f35c590523f2528deb9144784880 Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:01:54 +1100 Subject: [PATCH 01/13] Allow tracking how many bytes are read into a conn struct --- libs/connslot/connslot.c | 9 +++++---- libs/connslot/connslot.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/libs/connslot/connslot.c b/libs/connslot/connslot.c index b0382c7..f730cce 100644 --- a/libs/connslot/connslot.c +++ b/libs/connslot/connslot.c @@ -177,7 +177,7 @@ void conn_check_ready(conn_t *conn) { return; } -void conn_read(conn_t *conn, int fd) { +ssize_t conn_read(conn_t *conn, int fd) { conn->state = CONN_READING; // If no space available, try increasing our capacity @@ -195,21 +195,22 @@ void conn_read(conn_t *conn, int fd) { // zero-sized read request, the only time we get a zero back is if the // far end has closed conn->state = CONN_CLOSED; - return; + return 0; } if (size == -1) { if (errno == EWOULDBLOCK || errno == EAGAIN) { conn->state = CONN_EMPTY; - return; + return 0; } conn->state = CONN_ERROR; - return; + return 0; } // This will truncate the time to a int - usually 32bits conn->activity = time(NULL); conn_check_ready(conn); + return size; } ssize_t conn_write(conn_t *conn, int fd) { diff --git a/libs/connslot/connslot.h b/libs/connslot/connslot.h index fc24325..2ce3fb5 100644 --- a/libs/connslot/connslot.h +++ b/libs/connslot/connslot.h @@ -64,7 +64,7 @@ void conn_zero(conn_t *); int conn_init(conn_t *, size_t, size_t); void conn_accept(conn_t *, int, enum conn_proto); void conn_check_ready(conn_t *); -void conn_read(conn_t *, int); +ssize_t conn_read(conn_t *, int); ssize_t conn_write(conn_t *, int); int conn_iswriter(conn_t *); void conn_close(conn_t *, int); From 8815dc5f44e831ab7f98d42f6faa84294af9b6f8 Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:02:41 +1100 Subject: [PATCH 02/13] Just because a read would block, doesnt make the conn empty --- libs/connslot/connslot.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/connslot/connslot.c b/libs/connslot/connslot.c index f730cce..6bef3c7 100644 --- a/libs/connslot/connslot.c +++ b/libs/connslot/connslot.c @@ -200,7 +200,6 @@ ssize_t conn_read(conn_t *conn, int fd) { if (size == -1) { if (errno == EWOULDBLOCK || errno == EAGAIN) { - conn->state = CONN_EMPTY; return 0; } conn->state = CONN_ERROR; From 6648dcb42f3eefaa7162e9cdd3de6a26863ef0de Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:03:32 +1100 Subject: [PATCH 03/13] Deprecate the use of the CONN_SENDING state to allow easier bi-directional traffic --- libs/connslot/connslot.c | 21 +++++++++++++-------- libs/connslot/connslot.h | 2 +- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/libs/connslot/connslot.c b/libs/connslot/connslot.c index 6bef3c7..f72de20 100644 --- a/libs/connslot/connslot.c +++ b/libs/connslot/connslot.c @@ -215,8 +215,6 @@ ssize_t conn_read(conn_t *conn, int fd) { ssize_t conn_write(conn_t *conn, int fd) { ssize_t sent; - conn->state = CONN_SENDING; - if (fd == -1) { return 0; } @@ -282,13 +280,20 @@ ssize_t conn_write(conn_t *conn, int fd) { return sent; } -int conn_iswriter(conn_t *conn) { - switch (conn->state) { - case CONN_SENDING: - return 1; - default: - return 0; +bool conn_iswriter(conn_t *conn) { + int endpos = 0; + if (conn->reply_header) { + endpos += sb_len(conn->reply_header); + } + if (conn->reply) { + endpos += sb_len(conn->reply); } + + // If there are any bytes ready to send, then we are writable + if(endpos) { + return true; + } + return false; } void conn_close(conn_t *conn, int fd) { diff --git a/libs/connslot/connslot.h b/libs/connslot/connslot.h index 2ce3fb5..964a178 100644 --- a/libs/connslot/connslot.h +++ b/libs/connslot/connslot.h @@ -66,7 +66,7 @@ void conn_accept(conn_t *, int, enum conn_proto); void conn_check_ready(conn_t *); ssize_t conn_read(conn_t *, int); ssize_t conn_write(conn_t *, int); -int conn_iswriter(conn_t *); +bool conn_iswriter(conn_t *); void conn_close(conn_t *, int); bool conn_closeidle(conn_t *, int, int, int); void conn_dump(strbuf_t **, conn_t *); From 5f63b464e5973c3437aa09cb2f6496f2235e0451 Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:04:00 +1100 Subject: [PATCH 04/13] Just because we have finished writing doesnt make the read side empty --- libs/connslot/connslot.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/connslot/connslot.c b/libs/connslot/connslot.c index f72de20..596566f 100644 --- a/libs/connslot/connslot.c +++ b/libs/connslot/connslot.c @@ -269,7 +269,6 @@ ssize_t conn_write(conn_t *conn, int fd) { if (conn->reply_sendpos >= end_pos) { // We have sent the last bytes of this reply - conn->state = CONN_EMPTY; conn->reply_sendpos = 0; sb_zero(conn->reply_header); sb_zero(conn->reply); From b274360ce5167bfcc638266457760bad5a24e2fa Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:04:51 +1100 Subject: [PATCH 05/13] Ensure that the non blocking states are handled in the write path (and dont decrement the sendpos) --- libs/connslot/connslot.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libs/connslot/connslot.c b/libs/connslot/connslot.c index 596566f..03f0ce4 100644 --- a/libs/connslot/connslot.c +++ b/libs/connslot/connslot.c @@ -265,6 +265,13 @@ ssize_t conn_write(conn_t *conn, int fd) { unsigned int end_pos = sb_len(conn->reply_header) + sb_len(conn->reply); #endif + if (sent == -1) { + sent = 0; + if (errno != EAGAIN && errno != EWOULDBLOCK) { + conn->state = CONN_ERROR; + } + } + conn->reply_sendpos += sent; if (conn->reply_sendpos >= end_pos) { From b5ef52846690101edaa04e6118895892db70dca2 Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:06:08 +1100 Subject: [PATCH 06/13] Increase readability by clearly showing the ITER basic block as indented --- src/edge_utils.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/edge_utils.c b/src/edge_utils.c index e92efbb..bee9b2b 100644 --- a/src/edge_utils.c +++ b/src/edge_utils.c @@ -1630,11 +1630,13 @@ void update_supernode_reg (struct n3n_runtime_data * eee, time_t now) { if(eee->conf.bind_address) { // do not explicitly disconnect every time as the condition described is rare, so ... // ... check that there are no external peers (indicating a working socket) ... - HASH_ITER(hh, eee->known_peers, peer, tmp_peer) - if(!peer->local) { - cnt++; - break; + HASH_ITER(hh, eee->known_peers, peer, tmp_peer) { + if(!peer->local) { + cnt++; + break; + } } + if(!cnt) { // ... and then count the connection retries (eee->close_socket_counter)++; From 9810e9e42bff4d7df0e2547a3a46245ad8e16597 Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:07:18 +1100 Subject: [PATCH 07/13] Update user visible message to not claim UDP - both TCP and UDP are processed here --- src/edge_utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/edge_utils.c b/src/edge_utils.c index bee9b2b..151db20 100644 --- a/src/edge_utils.c +++ b/src/edge_utils.c @@ -2299,7 +2299,7 @@ void process_udp (struct n3n_runtime_data *eee, via_multicast = (in_sock == eee->udp_multicast_sock); #endif - traceEvent(TRACE_DEBUG, "Rx N2N_UDP of size %d from [%s]", + traceEvent(TRACE_DEBUG, "Rx VPN packet of size %d from [%s]", (signed int)udp_size, sock_to_cstr(sockbuf1, &sender)); if(eee->conf.header_encryption == HEADER_ENCRYPTION_ENABLED) { From d0b35e534d04ddedafd5dfe4b49350db89e21dab Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:08:13 +1100 Subject: [PATCH 08/13] Avoid trying to process the release of an error fd --- src/mainloop.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/mainloop.c b/src/mainloop.c index 5bc7ab9..9cb84f8 100644 --- a/src/mainloop.c +++ b/src/mainloop.c @@ -226,6 +226,10 @@ static int fdlist_allocslot (int fd, enum fd_info_proto proto) { static void fdlist_freefd (int fd) { int slot = 0; + if(fd == -1) { + // Cannot release an error fd! + return; + } while(slot < MAX_HANDLES) { if(fdlist[slot].fd != fd) { slot++; From 71d4e5c8c84a907d93e11a7656976fd399b9f274 Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:09:39 +1100 Subject: [PATCH 09/13] Clearly log errors with some setsockopts --- src/n2n.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/n2n.c b/src/n2n.c index 2101643..9c99d0a 100644 --- a/src/n2n.c +++ b/src/n2n.c @@ -62,8 +62,41 @@ SOCKET open_socket (struct sockaddr *local_address, socklen_t addrlen, int type /* fcntl(sock_fd, F_SETFL, O_NONBLOCK); */ #endif + int result; + sockopt = 1; - setsockopt(sock_fd, SOL_SOCKET, SO_REUSEADDR, (char *)&sockopt, sizeof(sockopt)); + result = setsockopt( + sock_fd, + SOL_SOCKET, + SO_REUSEADDR, + (char *)&sockopt, + sizeof(sockopt) + ); + if(result == -1) { + traceEvent( + TRACE_ERROR, + "SO_REUSEADDR fd=%i, error=%s\n", + sock_fd, + strerror(errno) + ); + } +#ifdef SO_REUSEPORT /* no SO_REUSEPORT in Windows / old linux versions */ + result = setsockopt( + sock_fd, + SOL_SOCKET, + SO_REUSEPORT, + (char *)&sockopt, + sizeof(sockopt) + ); + if(result == -1) { + traceEvent( + TRACE_ERROR, + "SO_REUSEPORT fd=%i, error=%s\n", + sock_fd, + strerror(errno) + ); + } +#endif if(!local_address) { // skip binding if we dont have the right details From 99ffa5efa8ab2458cd0d02972b9bcadc3cbb3504 Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 08:09:57 +1100 Subject: [PATCH 10/13] Ensure we do not ignore the return value for a write --- tools/crypto_helper.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tools/crypto_helper.c b/tools/crypto_helper.c index f1a47e3..0bea38f 100644 --- a/tools/crypto_helper.c +++ b/tools/crypto_helper.c @@ -96,7 +96,10 @@ static void cmd_header_decrypt (int argc, char **argv, void *conf) { exit(1); } - write(1, &buf, size); + int r = write(1, &buf, size); + if(r != size) { + printf("Short write\n"); + } exit(0); } @@ -130,7 +133,10 @@ static void cmd_pearson_128 (int argc, char **argv, void *conf) { unsigned char hash[16]; size = read(0, &buf, sizeof(buf)); pearson_hash_128(hash, buf, size); - write(1, &hash, sizeof(hash)); + int r = write(1, &hash, sizeof(hash)); + if(r != sizeof(hash)) { + printf("Short write\n"); + } exit(0); } From 0d4d19b7d84bced83c621641d1fb70a6f579786d Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Thu, 12 Dec 2024 19:25:43 +1100 Subject: [PATCH 11/13] Use a specified OS version for all lint tests The switch to a newer ubuntu-24.04 also imported a set of lint tests that do not match anything the dev systems are using. --- .github/workflows/quick.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/quick.yml b/.github/workflows/quick.yml index f3da7c5..61d2061 100644 --- a/.github/workflows/quick.yml +++ b/.github/workflows/quick.yml @@ -36,7 +36,7 @@ jobs: smoketest_all_opts: name: Smoke test With all options turned on - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v4 @@ -75,7 +75,7 @@ jobs: - name: Generate coverage reports run: | make gcov - make cover COVERAGEDIR=coverage/ubuntu-latest + make cover COVERAGEDIR=coverage/ubuntu-22.04 shell: bash - name: Upload gcovr report artifact @@ -114,7 +114,7 @@ jobs: lint: name: Code syntax - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v4 From 021d831dc904335a34da2af255bdd58090e276ba Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Mon, 16 Dec 2024 21:20:59 +1100 Subject: [PATCH 12/13] Add an example container build --- doc/Containerfile.example | 46 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 doc/Containerfile.example diff --git a/doc/Containerfile.example b/doc/Containerfile.example new file mode 100644 index 0000000..f284117 --- /dev/null +++ b/doc/Containerfile.example @@ -0,0 +1,46 @@ +# +# A simple example of how to build a docker container for n3n-edge +# +# Build with the command: +# podman build \ +# --device=/dev/net/tun \ +# --cap-add=NET_ADMIN \ +# -t=n3n \ +# -f doc/Containerfile.example \ +# . +# +# Start a n3n session with: +# podman run \ +# -it \ +# --name=n3n \ +# --device=/dev/net/tun \ +# --cap-add=NET_ADMIN \ +# -v $PWD/n3n:/etc/n3n/ \ +# n3n start -vvvv +# +# Note that the build could be done without the --device and --cap-add if +# the `make test` is omitted + +FROM docker.io/library/debian:12 AS builder + +RUN apt-get update +RUN apt-get install -y build-essential autoconf git python3 jq sudo + +WORKDIR /n3n + +COPY . . + +RUN \ + ./autogen.sh && \ + ./configure && \ + make clean all && \ + make test + + +FROM docker.io/library/debian:12 + +COPY --from=builder /n3n/apps/n3n-edge /n3n-edge + +VOLUME [ "/etc/n3n" ] +ENTRYPOINT ["/n3n-edge"] +CMD ["start"] From 73d198abfc4a75389f7ad0a59ffc8de5aba783a2 Mon Sep 17 00:00:00 2001 From: Hamish Coleman Date: Mon, 16 Dec 2024 21:23:06 +1100 Subject: [PATCH 13/13] Finish removal of CONN_SENDING state --- libs/connslot/connslot.h | 1 - src/mainloop.c | 2 -- 2 files changed, 3 deletions(-) diff --git a/libs/connslot/connslot.h b/libs/connslot/connslot.h index 964a178..f710106 100644 --- a/libs/connslot/connslot.h +++ b/libs/connslot/connslot.h @@ -29,7 +29,6 @@ enum __attribute__((__packed__)) conn_state { CONN_EMPTY = 0, CONN_READING = 1, CONN_READY = 2, - CONN_SENDING = 3, CONN_CLOSED = 4, CONN_ERROR = 5, }; diff --git a/src/mainloop.c b/src/mainloop.c index 9cb84f8..cf6aff0 100644 --- a/src/mainloop.c +++ b/src/mainloop.c @@ -335,7 +335,6 @@ static void handle_fd (const time_t now, const struct fd_info info, struct n3n_r switch(conn->state) { case CONN_EMPTY: case CONN_READING: - case CONN_SENDING: // These states dont require us to do anything // TODO: // - handle reading/sending simultaneous? @@ -406,7 +405,6 @@ static void handle_fd (const time_t now, const struct fd_info info, struct n3n_r switch(conn->state) { case CONN_EMPTY: case CONN_READING: - case CONN_SENDING: // These states dont require us to do anything // TODO: // - handle reading/sending simultaneous?