BUG/MEDIUM: snapshot: take the proxy's lock while dumping errors

The proxy's lock it held while filling the error but not while dumping
it, so it's possible to dereference pointers being replaced, typically
server pointers. The risk is very low and unlikely but not inexistent.

Since "show errors" is rarely used in parallel, let's simply grab the
proxy's lock while dumping. Ideally we should use an R/W lock here but
it will not make any difference.

This patch must be backported to 1.8, but the code is in proto_http.c
there, though mostly similar.
This commit is contained in:
Willy Tarreau 2018-09-07 19:55:44 +02:00
parent ddb68ac69e
commit 36b2736a69

View File

@ -1995,11 +1995,8 @@ static int cli_io_handler_show_errors(struct appctx *appctx)
tm.tm_hour, tm.tm_min, tm.tm_sec, (int)(date.tv_usec/1000),
error_snapshot_id);
if (ci_putchk(si_ic(si), &trash) == -1) {
/* Socket buffer full. Let's try again later from the same point */
si_applet_cant_put(si);
return 0;
}
if (ci_putchk(si_ic(si), &trash) == -1)
goto cant_send;
appctx->ctx.errors.px = proxies_list;
appctx->ctx.errors.bol = 0;
@ -2012,6 +2009,8 @@ static int cli_io_handler_show_errors(struct appctx *appctx)
while (appctx->ctx.errors.px) {
struct error_snapshot *es;
HA_SPIN_LOCK(PROXY_LOCK, &appctx->ctx.errors.px->lock);
if ((appctx->ctx.errors.flag & 1) == 0) {
es = &appctx->ctx.errors.px->invalid_req;
if (appctx->ctx.errors.flag & 2) // skip req
@ -2086,11 +2085,9 @@ static int cli_io_handler_show_errors(struct appctx *appctx)
chunk_appendf(&trash, " \n");
if (ci_putchk(si_ic(si), &trash) == -1) {
/* Socket buffer full. Let's try again later from the same point */
si_applet_cant_put(si);
return 0;
}
if (ci_putchk(si_ic(si), &trash) == -1)
goto cant_send_unlock;
appctx->ctx.errors.ptr = 0;
appctx->ctx.errors.ev_id = es->ev_id;
}
@ -2099,10 +2096,9 @@ static int cli_io_handler_show_errors(struct appctx *appctx)
/* the snapshot changed while we were dumping it */
chunk_appendf(&trash,
" WARNING! update detected on this snapshot, dump interrupted. Please re-check!\n");
if (ci_putchk(si_ic(si), &trash) == -1) {
si_applet_cant_put(si);
return 0;
}
if (ci_putchk(si_ic(si), &trash) == -1)
goto cant_send_unlock;
goto next;
}
@ -2114,17 +2110,16 @@ static int cli_io_handler_show_errors(struct appctx *appctx)
newline = appctx->ctx.errors.bol;
newptr = dump_text_line(&trash, es->buf, global.tune.bufsize, es->buf_len, &newline, appctx->ctx.errors.ptr);
if (newptr == appctx->ctx.errors.ptr)
return 0;
goto cant_send_unlock;
if (ci_putchk(si_ic(si), &trash) == -1)
goto cant_send_unlock;
if (ci_putchk(si_ic(si), &trash) == -1) {
/* Socket buffer full. Let's try again later from the same point */
si_applet_cant_put(si);
return 0;
}
appctx->ctx.errors.ptr = newptr;
appctx->ctx.errors.bol = newline;
};
next:
HA_SPIN_UNLOCK(PROXY_LOCK, &appctx->ctx.errors.px->lock);
appctx->ctx.errors.bol = 0;
appctx->ctx.errors.ptr = -1;
appctx->ctx.errors.flag ^= 1;
@ -2134,6 +2129,12 @@ static int cli_io_handler_show_errors(struct appctx *appctx)
/* dump complete */
return 1;
cant_send_unlock:
HA_SPIN_UNLOCK(PROXY_LOCK, &appctx->ctx.errors.px->lock);
cant_send:
si_applet_cant_put(si);
return 0;
}
/* register cli keywords */