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.
pull/2176/head
Mike Meehan 4 days ago committed by mike.meehan
parent b3de179087
commit cb9bf08bde
No known key found for this signature in database
GPG Key ID: 3B66229B8C19CB15

@ -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);
}

@ -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);

@ -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);

Loading…
Cancel
Save