From cb9bf08bded3ce89c27d7c975ad110ce38ccdd34 Mon Sep 17 00:00:00 2001 From: Mike Meehan Date: Fri, 18 Sep 2026 10:54:04 -0400 Subject: [PATCH] control_ng: fix type confusion between socket_t* and websocket_conn* in Homer NG tracing control_ng_process() computed ng_ctx.local_ep (used only for Homer NG message tracing, homer-enable-ng) by casting its opaque p1 argument to socket_t* unconditionally: .local_ep = p1 ? &(((socket_t*)p1)->local) : NULL, p1 is not always a socket_t*: control_ng_incoming() (UDP) and control_stream_readable() (TCP) do pass a real socket_t* there, but both websocket transports (websocket_ng_process_generic() and websocket_http_ng_generic() in websocket.c) pass a struct websocket_conn* instead, since p1 is otherwise only used opaquely, handed straight to each transport own cb() callback. Reinterpreting a struct websocket_conn* as a socket_t* and dereferencing ->local produces a garbage-but-non-NULL pointer, which then segfaults inside homer_send() as soon as it is actually sent to Homer -- reliably reproducible on any NG-over-websocket connection (wss:// via --listen-https, or plain ws://) once homer-enable-ng is also on. Fix: add an explicit local_sock parameter to control_ng_process() (and control_ng_process_plain(), which must keep an identical signature -- both are invoked polymorphically through the same __typeof__(control_ng_process) cb in websocket.c) used only to build local_ep, decoupled from p1. Both websocket call sites pass NULL, since neither has a real listening socket_t to offer; UDP and TCP pass their already-existing socket_t* unchanged. --- daemon/control_ng.c | 35 +++++++++++++++++++++++++++++------ daemon/websocket.c | 8 ++++++-- include/control_ng.h | 6 ++++-- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/daemon/control_ng.c b/daemon/control_ng.c index 632ee760e..6e7c23e0a 100644 --- a/daemon/control_ng.c +++ b/daemon/control_ng.c @@ -919,9 +919,24 @@ send_resp: CH(homer_trace_msg_out, hctx, reply); } +// local_sock is the listening/connection socket this request arrived on, used +// only to fill in ng_ctx.local_ep for Homer NG tracing (see homer_trace_msg_in/ +// _out below). It is intentionally separate from the opaque p1 argument, which +// each transport's own cb() interprets however it likes (a socket_t* for UDP/ +// TCP, a struct websocket_conn* for the websocket transports) -- p1 must not be +// reused as local_sock, since that means blindly reinterpreting whatever p1 +// happens to be as a socket_t*. That's exactly what this function used to do, +// and it segfaulted in homer_send() as soon as NG tracing to Homer +// (homer-enable-ng) was combined with an NG-over-websocket connection: the +// websocket transports pass a struct websocket_conn* as p1, which is a +// different type than the socket_t* every other transport passes there. +// Callers that have no real listening socket_t to offer (currently: both +// websocket transports) pass NULL here, same as if p1 itself had been NULL +// before this fix -- Homer tracing then attaches no local endpoint, which is +// preferable to a wild pointer dereference. int control_ng_process(str *buf, const endpoint_t *sin, char *addr, const sockaddr_t *local, void (*cb)(str *, str *, const endpoint_t *, const sockaddr_t *, void *), - void *p1, struct obj *ref) + void *p1, struct obj *ref, const socket_t *local_sock) { str data; str_chr_str(&data, buf, ' '); @@ -941,7 +956,7 @@ int control_ng_process(str *buf, const endpoint_t *sin, char *addr, const sockad ilogs(control, LOG_INFO, "Detected command from %s as a duplicate", addr); ng_ctx hctx = {.sin_ep = sin, - .local_ep = p1 ? &(((socket_t*)p1)->local) : NULL, + .local_ep = local_sock ? &local_sock->local : NULL, .cookie = cookie, .command = cached->command, .callid = cached->callid, @@ -959,7 +974,7 @@ int control_ng_process(str *buf, const endpoint_t *sin, char *addr, const sockad g_autoptr(ng_buffer) ngbuf = NULL; ng_ctx hctx = {.sin_ep = sin, - .local_ep = p1 ? &(((socket_t*)p1)->local) : NULL, + .local_ep = local_sock ? &local_sock->local : NULL, .cookie = cookie, .command = -1}; @@ -975,8 +990,15 @@ int control_ng_process(str *buf, const endpoint_t *sin, char *addr, const sockad int control_ng_process_plain(str *data, const endpoint_t *sin, char *addr, const sockaddr_t *local, void (*cb)(str *, str *, const endpoint_t *, const sockaddr_t *, void *), - void *p1, struct obj *ref) + void *p1, struct obj *ref, const socket_t *local_sock) { + // unused here: this path never runs the homer tracing code + // that local_sock exists for, but the signature must match + // control_ng_process() exactly -- both are invoked through the + // same `__typeof__(control_ng_process) cb` function pointer in + // websocket.c + (void) local_sock; + g_autoptr(ng_buffer) ngbuf = NULL; str reply; @@ -1015,7 +1037,7 @@ static void control_ng_incoming(struct obj *obj, struct udp_buffer *udp_buf) { control_ng_process(&udp_buf->str, &udp_buf->sin, udp_buf->addr, &udp_buf->local_addr, control_ng_send_from, udp_buf->listener, - &udp_buf->obj); + &udp_buf->obj, udp_buf->listener); } static void control_incoming(struct streambuf_stream *s) { @@ -1080,7 +1102,8 @@ static void control_stream_readable(struct streambuf_stream *s) { ilog(LOG_DEBUG, "Got %zu bytes from %s", s->inbuf->buf->len, s->addr); while ((data = chunk_message(s->inbuf))) { ilog(LOG_DEBUG, "Got control ng message from %s", s->addr); - control_ng_process(data, &s->sock.remote, s->addr, NULL, control_ng_send, &s->sock, s->parent); + control_ng_process(data, &s->sock.remote, s->addr, NULL, control_ng_send, &s->sock, s->parent, + &s->sock); free(data); } diff --git a/daemon/websocket.c b/daemon/websocket.c index 95e00e7a4..f7daf9440 100644 --- a/daemon/websocket.c +++ b/daemon/websocket.c @@ -595,7 +595,10 @@ static const char *websocket_ng_process_generic(struct websocket_message *wm, buf->cmd = STR_LEN(buf->body->str, buf->body->len); buf->endpoint = wm->wc->endpoint; - cb(&buf->cmd, &buf->endpoint, buf->addr, NULL, websocket_ng_send_ws, wm->wc, &buf->obj); + // no real listening socket_t behind a websocket connection -- NULL here + // means Homer NG tracing (if enabled) attaches no local endpoint, + // rather than control_ng_process() reinterpreting wm->wc as one + cb(&buf->cmd, &buf->endpoint, buf->addr, NULL, websocket_ng_send_ws, wm->wc, &buf->obj, NULL); obj_put(buf); @@ -622,11 +625,12 @@ static const char *websocket_http_ng_generic(struct websocket_message *wm, buf->cmd = STR_LEN(buf->body->str, buf->body->len); buf->endpoint = wm->wc->endpoint; + // see websocket_ng_process_generic() above re: passing NULL here if (cb(&buf->cmd, &buf->endpoint, buf->addr, NULL, wm->content_type == CT_JSON ? websocket_ng_send_http_json : websocket_ng_send_http_ng, wm->wc, - &buf->obj)) + &buf->obj, NULL)) websocket_http_complete(wm->wc, 600, "text/plain", 6, "error\n"); obj_put(buf); diff --git a/include/control_ng.h b/include/control_ng.h index a912b5734..3326833aa 100644 --- a/include/control_ng.h +++ b/include/control_ng.h @@ -178,9 +178,11 @@ void notify_ng_tcp_clients(str *); void control_ng_init(void); void control_ng_cleanup(void); int control_ng_process(str *buf, const endpoint_t *sin, char *addr, const sockaddr_t *local, - void (*cb)(str *, str *, const endpoint_t *, const sockaddr_t *, void *), void *p1, struct obj *); + void (*cb)(str *, str *, const endpoint_t *, const sockaddr_t *, void *), void *p1, struct obj *, + const socket_t *local_sock); int control_ng_process_plain(str *buf, const endpoint_t *sin, char *addr, const sockaddr_t *local, - void (*cb)(str *, str *, const endpoint_t *, const sockaddr_t *, void *), void *p1, struct obj *); + void (*cb)(str *, str *, const endpoint_t *, const sockaddr_t *, void *), void *p1, struct obj *, + const socket_t *local_sock); void init_ng_tracing(void); ng_buffer *ng_buffer_new(struct obj *ref);