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.

diff --git a/src/proxy.c b/src/proxy.c
index 644730167593d42a8d7277da67a6a3b9151ed4c9..9306637722376968eacb9f7ccf17bcef7a86717c 100644 (file)
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -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 */