Skip to content

Commit 39fbc34

Browse files
authored
Fixes 17 (#206)
* Revert "Prevent use-after-free in TCP client removal" This reverts commit 442815e. There should not be any use-after-free. * Reverting code changes and minor improvements - tighten TCP accept/read/send error handling and non-blocking socket setup - improve logging for dropped TCP clients and response failures - fix UDP bind cleanup - correct address-list buffer bounds checks in dns_poller - refine DNS truncation handling and related response logic * Update HTTP3 development build script * Visual Studio Code development document added With suggestion to use clangd for IntelliSense purpose. * Uplift github actions/upload-artifact * CMakeLists.txt: More strict compilation for security Also: - breaking long lines for readability - set gcc/clang specific warnings * Small dns_poller optimalizations * Added resplver IP override and port change support Some polishing in doh_proxy.c * Truncation 2.0 Details in huge comment in code * Using Ubuntu 26.04 in github actions
1 parent 0ba0525 commit 39fbc34

19 files changed

Lines changed: 426 additions & 234 deletions

.github/workflows/cmake.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,12 @@ jobs:
88
# well on Windows or Mac. You can convert this to a matrix build if you need
99
# cross-platform coverage.
1010
# See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix
11-
runs-on: ubuntu-24.04
11+
runs-on: ubuntu-26.04
1212

1313
strategy:
1414
fail-fast: false
1515
matrix:
16-
compiler: [gcc-13, clang-18]
16+
compiler: [gcc-15, clang-22]
1717

1818
steps:
1919
- uses: actions/checkout@main
@@ -44,7 +44,7 @@ jobs:
4444
- name: Test
4545
run: make -C ${{github.workspace}}/ test ARGS="--verbose"
4646

47-
- uses: actions/upload-artifact@v4
47+
- uses: actions/upload-artifact@v7
4848
if: ${{ success() || failure() }}
4949
with:
5050
name: robot-logs-${{ matrix.compiler }}

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,5 @@ report.html
2020
custom_curl/
2121
valgrind-*.log
2222
tests/robot/__pycache__
23+
compile_commands.json
24+
.cache/clangd

CMakeLists.txt

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,26 @@ if (NOT CMAKE_INSTALL_BINDIR)
2727
set(CMAKE_INSTALL_BINDIR bin)
2828
endif()
2929

30-
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wextra -Wpedantic -Wstrict-aliasing -Wformat=2 -Wunused -Wno-variadic-macros -Wnull-dereference -Wshadow -Wconversion -Wsign-conversion -Wfloat-conversion -Wimplicit-fallthrough")
30+
string(APPEND CMAKE_C_FLAGS " -Wall -Wextra -Wpedantic -Wstrict-aliasing -Wformat=2 -Wunused -Wno-variadic-macros")
31+
string(APPEND CMAKE_C_FLAGS " -Wnull-dereference -Wshadow -Wconversion -Wsign-conversion -Wfloat-conversion -Wundef")
32+
string(APPEND CMAKE_C_FLAGS " -Wimplicit-fallthrough -Wstrict-overflow=2 -Wredundant-decls -Wdouble-promotion")
33+
string(APPEND CMAKE_C_FLAGS " -fstack-protector-strong -D_FORTIFY_SOURCE=3 -fPIE")
34+
3135
set(CMAKE_C_FLAGS_DEBUG "-gdwarf-4 -DDEBUG")
3236
set(CMAKE_C_FLAGS_RELEASE "-O2")
3337

38+
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fPIE -Wl,-z,relro,-z,now")
39+
40+
if (CMAKE_C_COMPILER_ID MATCHES GNU)
41+
string(APPEND CMAKE_C_FLAGS " -Warray-bounds=2 -Wduplicated-cond -Wduplicated-branches -Wrestrict")
42+
endif()
43+
if (CMAKE_C_COMPILER_ID MATCHES Clang)
44+
string(APPEND CMAKE_C_FLAGS " -Wno-format-nonliteral -Wno-double-promotion")
45+
endif()
3446
if (((CMAKE_C_COMPILER_ID MATCHES GNU AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 9) AND
3547
(CMAKE_C_COMPILER_ID MATCHES GNU AND CMAKE_C_COMPILER_VERSION VERSION_LESS 14)) OR
3648
( CMAKE_C_COMPILER_ID MATCHES Clang AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 10))
37-
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-gnu-zero-variadic-macro-arguments -Wno-gnu-folding-constant")
49+
string(APPEND CMAKE_C_FLAGS " -Wno-gnu-zero-variadic-macro-arguments -Wno-gnu-folding-constant")
3850
endif()
3951

4052
set(SERVICE_EXTRA_OPTIONS "")
@@ -116,7 +128,13 @@ if(USE_CLANG_TIDY)
116128
message(STATUS "clang-tidy not found.")
117129
else()
118130
message(STATUS "clang-tidy found: ${CLANG_TIDY_EXE}")
119-
set(DO_CLANG_TIDY "${CLANG_TIDY_EXE}" "-fix" "-fix-errors" "-checks=*,-readability-identifier-length,-altera-unroll-loops,-bugprone-easily-swappable-parameters,-concurrency-mt-unsafe,-*magic-numbers,-hicpp-signed-bitwise,-readability-function-cognitive-complexity,-altera-id-dependent-backward-branch,-misc-include-cleaner,-llvmlibc-restrict-system-libc-headers,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
131+
set(CLANG_TIDY_CHECKS "-checks=*,-readability-identifier-length,-altera-unroll-loops,-concurrency-mt-unsafe,")
132+
string(APPEND CLANG_TIDY_CHECKS "-bugprone-easily-swappable-parameters,-*magic-numbers,-hicpp-signed-bitwise,")
133+
string(APPEND CLANG_TIDY_CHECKS "-readability-function-cognitive-complexity,-altera-id-dependent-backward-branch,")
134+
string(APPEND CLANG_TIDY_CHECKS "-misc-include-cleaner,-llvmlibc-restrict-system-libc-headers,")
135+
string(APPEND CLANG_TIDY_CHECKS "-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,")
136+
string(APPEND CLANG_TIDY_CHECKS "-*function-size,-clang-diagnostic-*variadic-macro-arguments*")
137+
set(DO_CLANG_TIDY "${CLANG_TIDY_EXE}" "-fix" "-fix-errors" "${CLANG_TIDY_CHECKS}")
120138
endif()
121139
else()
122140
message(STATUS "Not using clang-tidy.")

development_build_with_http3.sh

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@ echo "WARNING !!!"
77
echo
88
echo "Use only for development and testing!"
99
echo "It is highly highly not recommended, to use in production!"
10-
echo "This script was based on: https://github.com/curl/curl/blob/curl-8_12_1/docs/HTTP3.md"
10+
echo "This script was based on: https://github.com/curl/curl/blob/curl-8_19_0/docs/HTTP3.md"
1111
echo
12-
echo "Extra packages suggested to be installed: autoconf libtool"
12+
echo "Extra packages suggested to be installed: pkg-config pkgconf autoconf automake libtool"
1313
echo
1414

1515
sleep 5
@@ -22,14 +22,14 @@ cd custom_curl
2222

2323
###
2424

25-
git clone --depth 1 -b openssl-3.1.4+quic https://github.com/quictls/openssl
25+
git clone --depth 1 -b openssl-3.5.6 https://github.com/openssl/openssl
2626
cd openssl
27-
./config enable-tls1_3 --prefix=$INSTALL_DIR
28-
make -j build_libs
27+
./config --prefix=$INSTALL_DIR --libdir=lib
28+
make -j
2929
make install_dev
3030
cd ..
3131

32-
git clone --depth 1 -b v1.1.0 https://github.com/ngtcp2/nghttp3
32+
git clone --depth 1 -b v1.15.0 https://github.com/ngtcp2/nghttp3
3333
cd nghttp3
3434
git submodule update --init
3535
autoreconf -fi
@@ -38,26 +38,26 @@ make -j
3838
make install
3939
cd ..
4040

41-
git clone --depth 1 -b v1.2.0 https://github.com/ngtcp2/ngtcp2
41+
git clone --depth 1 -b v1.22.0 https://github.com/ngtcp2/ngtcp2
4242
cd ngtcp2
4343
autoreconf -fi
44-
./configure PKG_CONFIG_PATH=$INSTALL_DIR/lib64/pkgconfig:$INSTALL_DIR/lib64/pkgconfig LDFLAGS="-Wl,-rpath,$INSTALL_DIR/lib64" --prefix=$INSTALL_DIR --enable-lib-only --with-openssl
44+
./configure PKG_CONFIG_PATH=$INSTALL_DIR/lib/pkgconfig LDFLAGS="-Wl,-rpath,$INSTALL_DIR/lib" --prefix=$INSTALL_DIR --enable-lib-only --with-openssl
4545
make -j
4646
make install
4747
cd ..
4848

49-
git clone --depth 1 -b v1.64.0 https://github.com/nghttp2/nghttp2
49+
git clone --depth 1 -b v1.68.1 https://github.com/nghttp2/nghttp2
5050
cd nghttp2
5151
autoreconf -fi
52-
./configure PKG_CONFIG_PATH=$INSTALL_DIR/lib64/pkgconfig:$INSTALL_DIR/lib64/pkgconfig LDFLAGS="-Wl,-rpath,$INSTALL_DIR/lib64" --prefix=$INSTALL_DIR --enable-lib-only --with-openssl
52+
./configure PKG_CONFIG_PATH=$INSTALL_DIR/lib/pkgconfig LDFLAGS="-Wl,-rpath,$INSTALL_DIR/lib" --prefix=$INSTALL_DIR --enable-lib-only --with-openssl
5353
make -j
5454
make install
5555
cd ..
5656

57-
git clone --depth 1 -b curl-8_12_1 https://github.com/curl/curl
57+
git clone --depth 1 -b curl-8_19_0 https://github.com/curl/curl
5858
cd curl
5959
autoreconf -fi
60-
LDFLAGS="-Wl,-rpath,$INSTALL_DIR/lib64" ./configure --with-openssl=$INSTALL_DIR --with-nghttp2=$INSTALL_DIR --with-nghttp3=$INSTALL_DIR --with-ngtcp2=$INSTALL_DIR --prefix=$INSTALL_DIR
60+
LDFLAGS="-Wl,-rpath,$INSTALL_DIR/lib" ./configure PKG_CONFIG_PATH=$INSTALL_DIR/lib/pkgconfig --with-openssl=$INSTALL_DIR --with-nghttp2=$INSTALL_DIR --with-nghttp3=$INSTALL_DIR --with-ngtcp2=$INSTALL_DIR --prefix=$INSTALL_DIR --without-libpsl
6161
make -j
6262
make install
6363
cd ..

src/dns_listener_tcp.c

Lines changed: 41 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,3 @@
1-
//NOLINTNEXTLINE(bugprone-reserved-identifier,cert-dcl37-c,cert-dcl51-cpp)
2-
#define _GNU_SOURCE // needed for having accept4()
3-
41
#include <arpa/inet.h>
52
#include <errno.h>
63
#include <fcntl.h>
@@ -34,7 +31,8 @@
3431
enum {
3532
LISTEN_BACKLOG = 5,
3633
IDLE_TIMEOUT_S = 120, // "two minutes" according to RFC1035 4.2.2
37-
RESEND_DELAY_US = 500, // 0.0005 sec
34+
RESPONSE_SEND_ATTEMPTS = 50, // 0.025 sec max wait
35+
RESPONSE_SEND_DELAY_US = 500, // 0.0005 sec
3836
TCP_DNS_MAX_PAYLOAD = UINT16_MAX - sizeof(uint16_t), // Max after 2-byte length prefix
3937
};
4038

@@ -95,17 +93,13 @@ static void remove_client(struct tcp_client_s * client) {
9593

9694
close(client->sock);
9795

98-
// Save next pointer before freeing. Safe because this is single-threaded
99-
// event loop - no callbacks can run during this function.
100-
struct tcp_client_s *next = client->next;
101-
10296
if (d->clients == client) {
103-
d->clients = next;
97+
d->clients = client->next;
10498
}
10599
else {
106100
for (struct tcp_client_s * cur = d->clients; cur != NULL; cur = cur->next) {
107101
if (cur->next == client) {
108-
cur->next = next;
102+
cur->next = client->next;
109103
break;
110104
}
111105
}
@@ -144,11 +138,11 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop,
144138
ssize_t len = recv(w->fd, buf, DNS_REQUEST_BUFFER_SIZE, 0);
145139
if (len <= 0) {
146140
if (len == 0 || errno == ECONNRESET) {
147-
DLOG_CLIENT("Connection closed");
141+
DLOG_CLIENT("TCP client closed connection");
148142
} else if (errno == EAGAIN || errno == EWOULDBLOCK) {
149143
return;
150144
} else {
151-
WLOG_CLIENT("Read error: %s", strerror(errno));
145+
WLOG_CLIENT("Read error: %s (%d), dropping client", strerror(errno), errno);
152146
}
153147
remove_client(client);
154148
return;
@@ -194,12 +188,13 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop,
194188
uint8_t request_received = 0;
195189
while (get_dns_request(client, &dns_req, &req_size)) {
196190
if (req_size < DNS_HEADER_LENGTH) {
197-
WLOG_CLIENT("Malformed request received, too short: %u", req_size);
191+
WLOG_CLIENT("Malformed request received, too short: %u, dropping client", req_size);
198192
free(dns_req);
199193
remove_client(client);
200194
return;
201195
}
202196

197+
DLOG_CLIENT("Requested %04hX", ntohs(*((uint16_t*)dns_req)));
203198
d->cb(d->cb_data, &d->base, (struct sockaddr*)&client->raddr, dns_req, req_size);
204199
request_received = 1;
205200
}
@@ -223,16 +218,27 @@ static void accept_cb(struct ev_loop __attribute__((unused)) *loop,
223218
struct sockaddr_storage client_addr;
224219
socklen_t client_addr_len = sizeof(client_addr);
225220

226-
int client_sock = accept(w->fd, (struct sockaddr *)&client_addr, &client_addr_len);
227-
if (client_sock != -1) {
228-
// Set non-blocking mode for macOS compatibility (Linux accept4 does this atomically)
229-
int flags = fcntl(client_sock, F_GETFL, 0);
230-
if (flags != -1) {
231-
fcntl(client_sock, F_SETFL, flags | O_NONBLOCK);
221+
// NOLINTNEXTLINE(android-cloexec-accept)
222+
const int client_sock = accept(w->fd, (struct sockaddr *)&client_addr, &client_addr_len);
223+
if (client_sock == -1) {
224+
if (errno != EAGAIN && errno != EWOULDBLOCK) {
225+
ELOG("Failed to accept TCP client: %s (%d)", strerror(errno), errno);
232226
}
227+
return;
233228
}
234-
if (client_sock == -1 && errno != EAGAIN && errno != EWOULDBLOCK) {
235-
ELOG("Failed to accept TCP client: %s", strerror(errno));
229+
230+
// Set non-blocking mode for macOS compatibility (Linux accept4 does this atomically)
231+
const int flags = fcntl(client_sock, F_GETFL, 0);
232+
if (flags == -1) {
233+
ELOG("Error getting TCP client socket flags: %s (%d), dropping client",
234+
strerror(errno), errno);
235+
close(client_sock);
236+
return;
237+
}
238+
if (fcntl(client_sock, F_SETFL, flags | O_NONBLOCK) == -1) {
239+
ELOG("Error setting TCP client socket to non-blocking: %s (%d), dropping client",
240+
strerror(errno), errno);
241+
close(client_sock);
236242
return;
237243
}
238244

@@ -329,6 +335,7 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr,
329335
WLOG("Malformed response received, invalid length: %u", resp_len);
330336
return;
331337
}
338+
const uint16_t response_id = ntohs(*((uint16_t*)resp));
332339

333340
// find client data
334341
struct tcp_client_s *client = NULL;
@@ -339,7 +346,6 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr,
339346
}
340347
}
341348
if (client == NULL) {
342-
uint16_t response_id = ntohs(*((uint16_t*)resp));
343349
WLOG("Could not find client, can not send DNS response: %04hX", response_id);
344350
return;
345351
}
@@ -355,37 +361,38 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr,
355361
uint16_t resp_size = htons((uint16_t)resp_len);
356362
ssize_t len = send(client->sock, &resp_size, sizeof(uint16_t), MSG_MORE | MSG_NOSIGNAL);
357363
if (len != sizeof(uint16_t)) {
358-
WLOG_CLIENT("Send error: %s, len: %d", strerror(errno), len);
364+
WLOG_CLIENT("Send error: %s (%d), len: %d, dropping client", strerror(errno), errno, len);
359365
remove_client(client);
360366
return;
361367
}
362368

363369
// send the response
364370
ssize_t sent = 0;
365371
int attempts = 0;
366-
for (; attempts < 50; ++attempts) // 25ms max wait
372+
for (; attempts < RESPONSE_SEND_ATTEMPTS; ++attempts)
367373
{
368374
len = send(client->sock, resp + sent, resp_len - (size_t)sent, MSG_NOSIGNAL);
369-
if (len < 0) {
375+
if (len > 0) {
376+
sent += len;
377+
if (sent == (ssize_t)resp_len) {
378+
break;
379+
}
380+
} else if (len < 0) {
370381
if (errno != EAGAIN && errno != EWOULDBLOCK) {
371-
WLOG_CLIENT("Send error: %s", strerror(errno));
382+
WLOG_CLIENT("Send error: %s (%d), dropping client", strerror(errno), errno);
372383
remove_client(client);
373384
return;
374385
}
375-
// EAGAIN/EWOULDBLOCK - socket buffer full, retry after delay
376-
continue;
377-
}
378-
sent += len;
379-
if (sent == (ssize_t)resp_len) {
380-
break;
381386
}
382-
usleep(RESEND_DELAY_US);
387+
usleep(RESPONSE_SEND_DELAY_US);
383388
}
384389
if (sent != (ssize_t)resp_len) {
385-
WLOG_CLIENT("Send timeout after %d attempts, sent %zd/%zu bytes", attempts, sent, resp_len);
390+
WLOG_CLIENT("Send timeout after %d attempts, sent %zd/%zu bytes, dropping client",
391+
attempts, sent, resp_len);
386392
remove_client(client);
387393
return;
388394
}
395+
DLOG_CLIENT("Responded %04hX", response_id);
389396

390397
ev_timer_again(d->loop, &client->timer_watcher);
391398
}

src/dns_listener_udp.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ typedef struct dns_listener_udp_s {
2121

2222
dns_request_fn cb;
2323
void *cb_data;
24-
} dns_listener_udp_t;
24+
} __attribute__((aligned(128))) dns_listener_udp_t;
2525

2626
// Creates and binds a listening UDP socket for incoming requests.
2727
static int get_listen_sock(struct addrinfo *listen_addrinfo) {
@@ -44,7 +44,6 @@ static int get_listen_sock(struct addrinfo *listen_addrinfo) {
4444

4545
int res = bind(sock, listen_addrinfo->ai_addr, listen_addrinfo->ai_addrlen);
4646
if (res < 0) {
47-
close(sock);
4847
FLOG("Error binding on %s:%d UDP: %s (%d)", ipstr, port,
4948
strerror(errno), errno);
5049
}

src/dns_poller.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ static char *get_addr_listing(struct ares_addrinfo_node * nodes) {
5757
const char *res = NULL;
5858

5959
// Check that we have space for at least one character plus null terminator
60-
if (pos >= list + POLLER_ADDR_LIST_SIZE - 1) {
60+
if ((pos - list) >= POLLER_ADDR_LIST_SIZE - 1) {
6161
DLOG("Not enough space for more addresses");
6262
break;
6363
}
@@ -81,7 +81,7 @@ static char *get_addr_listing(struct ares_addrinfo_node * nodes) {
8181
if (res != NULL) {
8282
pos += strlen(pos);
8383
// Check we have room for the comma and null terminator
84-
if (pos >= list + POLLER_ADDR_LIST_SIZE - 1) {
84+
if ((pos - list) >= POLLER_ADDR_LIST_SIZE - 1) {
8585
DLOG("Not enough space for comma separator");
8686
break;
8787
}
@@ -163,7 +163,7 @@ static ev_tstamp get_timeout(dns_poller_t *d)
163163
static struct timeval max_tv = {.tv_sec = 5, .tv_usec = 0};
164164
struct timeval tv;
165165
struct timeval *tvp = ares_timeout(d->ares, &max_tv, &tv);
166-
ev_tstamp after = (double)tvp->tv_sec + (double)tvp->tv_usec * 1e-6;
166+
ev_tstamp after = (double)tvp->tv_sec + ((double)tvp->tv_usec * 1e-6);
167167
return after > 0.1 ? after : 0.1;
168168
}
169169

0 commit comments

Comments
 (0)