BUG/MEDIUM: cli: make "show sess" really thread-safe

This one used to rely on a few spin locks around lists manipulations
only but 1) there were still a few races (e.g. when aborting, or
between STAT_ST_INIT and STAT_ST_LIST), and 2) after last commit
which dumps htx info it became obvious that dereferencing the buffer
contents is not safe at all.

This patch uses the thread isolation from the rendez-vous point
instead, to guarantee that nothing moves during the dump. It may
make the dump a bit slower but it will be 100% safe.

This fix must be backported to 1.9, and possibly to 1.8 which likely
suffers from the short races above, eventhough they're extremely
hard to trigger.

diff --git a/src/stream.c b/src/stream.c
index 48bbed1f5c7312141b0422b51ca886ee146002b5..2bc111d997138778e114c5a77aa4c61b47b42f84 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -2812,13 +2812,9 @@ static int stats_dump_full_strm_to_buffer(struct stream_interface *si, struct st
        if (appctx->ctx.sess.section > 0 && appctx->ctx.sess.uid != strm->uniq_id) {
                /* stream changed, no need to go any further */
                chunk_appendf(&trash, "  *** session terminated while we were watching it ***\n");
-               if (ci_putchk(si_ic(si), &trash) == -1) {
-                       si_rx_room_blk(si);
-                       return 0;
-               }
-               appctx->ctx.sess.uid = 0;
-               appctx->ctx.sess.section = 0;
-               return 1;
+               if (ci_putchk(si_ic(si), &trash) == -1)
+                       goto full;
+               goto done;
        }
 
        switch (appctx->ctx.sess.section) {
@@ -3136,17 +3132,18 @@ static int stats_dump_full_strm_to_buffer(struct stream_interface *si, struct st
                                      (unsigned long long)htx->extra);
                }
 
-               if (ci_putchk(si_ic(si), &trash) == -1) {
-                       si_rx_room_blk(si);
-                       return 0;
-               }
+               if (ci_putchk(si_ic(si), &trash) == -1)
+                       goto full;
 
                /* use other states to dump the contents */
        }
        /* end of dump */
+ done:
        appctx->ctx.sess.uid = 0;
        appctx->ctx.sess.section = 0;
        return 1;
+ full:
+       return 0;
 }
 
 
@@ -3169,14 +3166,16 @@ static int cli_parse_show_sess(char **args, char *payload, struct appctx *appctx
 
 /* This function dumps all streams' states onto the stream interface's
  * read buffer. It returns 0 if the output buffer is full and it needs
- * to be called again, otherwise non-zero. It is designed to be called
- * from stats_dump_sess_to_buffer() below.
+ * to be called again, otherwise non-zero. It proceeds in an isolated
+ * thread so there is no thread safety issue here.
  */
 static int cli_io_handler_dump_sess(struct appctx *appctx)
 {
        struct stream_interface *si = appctx->owner;
        struct connection *conn;
 
+       thread_isolate();
+
        if (unlikely(si_ic(si)->flags & (CF_WRITE_ERROR|CF_SHUTW))) {
                /* If we're forced to shut down, we might have to remove our
                 * reference to the last stream being dumped.
@@ -3187,7 +3186,7 @@ static int cli_io_handler_dump_sess(struct appctx *appctx)
                                LIST_INIT(&appctx->ctx.sess.bref.users);
                        }
                }
-               return 1;
+               goto done;
        }
 
        chunk_reset(&trash);
@@ -3202,14 +3201,11 @@ static int cli_io_handler_dump_sess(struct appctx *appctx)
                 * pointer points back to the head of the streams list.
                 */
                LIST_INIT(&appctx->ctx.sess.bref.users);
-               HA_SPIN_LOCK(STRMS_LOCK, &streams_lock);
                appctx->ctx.sess.bref.ref = streams.n;
-               HA_SPIN_UNLOCK(STRMS_LOCK, &streams_lock);
                appctx->st2 = STAT_ST_LIST;
                /* fall through */
 
        case STAT_ST_LIST:
-               HA_SPIN_LOCK(STRMS_LOCK, &streams_lock);
                /* first, let's detach the back-ref from a possible previous stream */
                if (!LIST_ISEMPTY(&appctx->ctx.sess.bref.users)) {
                        LIST_DEL(&appctx->ctx.sess.bref.users);
@@ -3229,10 +3225,8 @@ static int cli_io_handler_dump_sess(struct appctx *appctx)
 
                                LIST_ADDQ(&curr_strm->back_refs, &appctx->ctx.sess.bref.users);
                                /* call the proper dump() function and return if we're missing space */
-                               if (!stats_dump_full_strm_to_buffer(si, curr_strm)) {
-                                       HA_SPIN_UNLOCK(STRMS_LOCK, &streams_lock);
-                                       return 0;
-                               }
+                               if (!stats_dump_full_strm_to_buffer(si, curr_strm))
+                                       goto full;
 
                                /* stream dump complete */
                                LIST_DEL(&appctx->ctx.sess.bref.users);
@@ -3357,10 +3351,8 @@ static int cli_io_handler_dump_sess(struct appctx *appctx)
                                /* let's try again later from this stream. We add ourselves into
                                 * this stream's users so that it can remove us upon termination.
                                 */
-                               si_rx_room_blk(si);
                                LIST_ADDQ(&curr_strm->back_refs, &appctx->ctx.sess.bref.users);
-                               HA_SPIN_UNLOCK(STRMS_LOCK, &streams_lock);
-                               return 0;
+                               goto full;
                        }
 
                next_sess:
@@ -3374,25 +3366,26 @@ static int cli_io_handler_dump_sess(struct appctx *appctx)
                        else
                                chunk_appendf(&trash, "Session not found.\n");
 
-                       if (ci_putchk(si_ic(si), &trash) == -1) {
-                               si_rx_room_blk(si);
-                               HA_SPIN_UNLOCK(STRMS_LOCK, &streams_lock);
-                               return 0;
-                       }
+                       if (ci_putchk(si_ic(si), &trash) == -1)
+                               goto full;
 
                        appctx->ctx.sess.target = NULL;
                        appctx->ctx.sess.uid = 0;
-                       HA_SPIN_UNLOCK(STRMS_LOCK, &streams_lock);
-                       return 1;
+                       goto done;
                }
-
-               HA_SPIN_UNLOCK(STRMS_LOCK, &streams_lock);
                /* fall through */
 
        default:
                appctx->st2 = STAT_ST_FIN;
-               return 1;
+               goto done;
        }
+ done:
+       thread_release();
+       return 1;
+ full:
+       thread_release();
+       si_rx_room_blk(si);
+       return 0;
 }
 
 static void cli_release_show_sess(struct appctx *appctx)