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 @@ static void control_ng_process_payload(ng_ctx *hctx, str *reply, str *data, cons 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);