From a8b7684319a48110afb61d50f004566886a6baf1 Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Fri, 16 Dec 2022 15:08:36 +0100 Subject: [PATCH] BUG/MEDIUM: stats: Rely on a local trash buffer to dump the stats It is possible to block the stats applet if a line exceeds the free space in the responsse buffer while the buffer is empty. It is only an issue in HTTP becaues of the HTX overhead and, AFAIK, only with json output. In this case, the applet is unable to write anything in the response buffer and waits for some free space to proceed further. On the other hand, because the response channel is empty, nothing is sent and thus no space can be freed. At this stage, the stream and the applet are blocked waiting for the other side. To avoid this situation, we must take care to not dump a line exceeding the free space in the HTX message. It means we cannot rely anymore on the global trash buffer. At least, not directly. The trick is to use a local trash buffer, mapped on the global one but with a different size. We use b_make() to do so. The local trash buffer is thread local to avoid any concurrency issue. It is a valid fix. However it could be good to review the internal API of the stats applet to not rely on a global variable. This patch should solve the #1873. It must be backported at least as far as 2.6. Older versions must be evaluated first but it is probably possible to hit this bug with long proxy/server names. --- src/stats.c | 208 +++++++++++++++++++++++++++------------------------- 1 file changed, 110 insertions(+), 98 deletions(-) diff --git a/src/stats.c b/src/stats.c index dad626172..7b664c129 100644 --- a/src/stats.c +++ b/src/stats.c @@ -282,6 +282,8 @@ static struct list stats_module_list[STATS_DOMAIN_COUNT] = { }; THREAD_LOCAL void *trash_counters; +static THREAD_LOCAL struct buffer trash_chunk = BUF_NULL; + static inline uint8_t stats_get_domain(uint32_t domain) { @@ -346,8 +348,8 @@ static const char *stats_scope_ptr(struct appctx *appctx, struct stconn *sc) */ -/* Dumps the stats CSV header to the trash buffer which. The caller is responsible - * for clearing it if needed. +/* Dumps the stats CSV header to the local trash buffer. The caller is + * responsible for clearing it if needed. * NOTE: Some tools happen to rely on the field position instead of its name, * so please only append new fields at the end, never in the middle. */ @@ -355,19 +357,19 @@ static void stats_dump_csv_header(enum stats_domain domain) { int field; - chunk_appendf(&trash, "# "); + chunk_appendf(&trash_chunk, "# "); if (stat_f[domain]) { for (field = 0; field < stat_count[domain]; ++field) { - chunk_appendf(&trash, "%s,", stat_f[domain][field].name); + chunk_appendf(&trash_chunk, "%s,", stat_f[domain][field].name); /* print special delimiter on proxy stats to mark end of static fields */ if (domain == STATS_DOMAIN_PROXY && field + 1 == ST_F_TOTAL_FIELDS) - chunk_appendf(&trash, "-,"); + chunk_appendf(&trash_chunk, "-,"); } } - chunk_appendf(&trash, "\n"); + chunk_appendf(&trash_chunk, "\n"); } /* Emits a stats field without any surrounding element and properly encoded to @@ -1648,13 +1650,13 @@ int stats_dump_one_line(const struct field *stats, size_t stats_count, int ret; if (ctx->flags & STAT_FMT_HTML) - ret = stats_dump_fields_html(&trash, stats, ctx); + ret = stats_dump_fields_html(&trash_chunk, stats, ctx); else if (ctx->flags & STAT_FMT_TYPED) - ret = stats_dump_fields_typed(&trash, stats, stats_count, ctx); + ret = stats_dump_fields_typed(&trash_chunk, stats, stats_count, ctx); else if (ctx->flags & STAT_FMT_JSON) - ret = stats_dump_fields_json(&trash, stats, stats_count, ctx); + ret = stats_dump_fields_json(&trash_chunk, stats, stats_count, ctx); else - ret = stats_dump_fields_csv(&trash, stats, stats_count, ctx); + ret = stats_dump_fields_csv(&trash_chunk, stats, stats_count, ctx); if (ret) ctx->flags |= STAT_STARTED; @@ -1842,9 +1844,10 @@ int stats_fill_fe_stats(struct proxy *px, struct field *stats, int len, return 1; } -/* Dumps a frontend's line to the trash for the current proxy and uses - * the state from stream connector . The caller is responsible for clearing - * the trash if needed. Returns non-zero if it emits anything, zero otherwise. +/* Dumps a frontend's line to the local trash buffer for the current proxy + * and uses the state from stream connector . The caller is responsible for + * clearing the local trash buffer if needed. Returns non-zero if it emits + * anything, zero otherwise. */ static int stats_dump_fe_stats(struct stconn *sc, struct proxy *px) { @@ -2010,9 +2013,10 @@ int stats_fill_li_stats(struct proxy *px, struct listener *l, int flags, return 1; } -/* Dumps a line for listener and proxy to the trash and uses the state - * from stream connector . The caller is responsible for clearing the trash - * if needed. Returns non-zero if it emits anything, zero otherwise. +/* Dumps a line for listener and proxy to the local trash buffer and + * uses the state from stream connector . The caller is responsible for + * clearing the local trash buffer if needed. Returns non-zero if it emits + * anything, zero otherwise. */ static int stats_dump_li_stats(struct stconn *sc, struct proxy *px, struct listener *l) { @@ -2524,10 +2528,10 @@ int stats_fill_sv_stats(struct proxy *px, struct server *sv, int flags, return 1; } -/* Dumps a line for server and proxy to the trash and uses the state - * from stream connector , and server state . The caller is - * responsible for clearing the trash if needed. Returns non-zero if it emits - * anything, zero otherwise. +/* Dumps a line for server and proxy to the local trash vbuffer and + * uses the state from stream connector , and server state . The + * caller is responsible for clearing the local trash buffer if needed. Returns + * non-zero if it emits anything, zero otherwise. */ static int stats_dump_sv_stats(struct stconn *sc, struct proxy *px, struct server *sv) { @@ -2855,9 +2859,10 @@ int stats_fill_be_stats(struct proxy *px, int flags, struct field *stats, int le return 1; } -/* Dumps a line for backend to the trash for and uses the state from stream - * interface . The caller is responsible for clearing the trash if needed. - * Returns non-zero if it emits anything, zero otherwise. +/* Dumps a line for backend to the local trash buffer for and uses the + * state from stream interface . The caller is responsible for clearing the + * local trash buffer if needed. Returns non-zero if it emits anything, zero + * otherwise. */ static int stats_dump_be_stats(struct stconn *sc, struct proxy *px) { @@ -2897,9 +2902,9 @@ static int stats_dump_be_stats(struct stconn *sc, struct proxy *px) return stats_dump_one_line(stats, stats_count, appctx); } -/* Dumps the HTML table header for proxy to the trash for and uses the state from - * stream connector and per-uri parameters . The caller is responsible - * for clearing the trash if needed. +/* Dumps the HTML table header for proxy to the local trash buffer for and + * uses the state from stream connector and per-uri parameters . The + * caller is responsible for clearing the local trash buffer if needed. */ static void stats_dump_html_px_hdr(struct stconn *sc, struct proxy *px) { @@ -2922,17 +2927,17 @@ static void stats_dump_html_px_hdr(struct stconn *sc, struct proxy *px) scope_txt[strlen(STAT_SCOPE_PATTERN) + ctx->scope_len] = 0; } - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "
"); } /* print a new table */ - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "\n" "" "" "" "\n" @@ -2960,17 +2965,17 @@ static void stats_dump_html_px_hdr(struct stconn *sc, struct proxy *px) if (ctx->flags & STAT_ADMIN) { /* Column heading for Enable or Disable server */ if ((px->cap & PR_CAP_BE) && px->srv) - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "", px->id, px->id); else - chunk_appendf(&trash, ""); + chunk_appendf(&trash_chunk, ""); } - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "" "" "" @@ -2983,11 +2988,11 @@ static void stats_dump_html_px_hdr(struct stconn *sc, struct proxy *px) list_for_each_entry(mod, &stats_module_list[STATS_DOMAIN_PROXY], list) { ++stats_module_len; } - chunk_appendf(&trash, "", + chunk_appendf(&trash_chunk, "", stats_module_len); } - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "\n" "" "" @@ -3001,26 +3006,27 @@ static void stats_dump_html_px_hdr(struct stconn *sc, struct proxy *px) if (ctx->flags & STAT_SHMODULES) { list_for_each_entry(mod, &stats_module_list[STATS_DOMAIN_PROXY], list) { - chunk_appendf(&trash, "", mod->name); + chunk_appendf(&trash_chunk, "", mod->name); } } - chunk_appendf(&trash, ""); + chunk_appendf(&trash_chunk, ""); } -/* Dumps the HTML table trailer for proxy to the trash for and uses the state from - * stream connector . The caller is responsible for clearing the trash if needed. +/* Dumps the HTML table trailer for proxy to the local trash buffer for and + * uses the state from stream connector . The caller is responsible for + * clearing the local trash buffer if needed. */ static void stats_dump_html_px_end(struct stconn *sc, struct proxy *px) { struct appctx *appctx = __sc_appctx(sc); struct show_stat_ctx *ctx = appctx->svcctx; - chunk_appendf(&trash, "
"); - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "%s" "%s", px->id, @@ -2941,13 +2946,13 @@ static void stats_dump_html_px_hdr(struct stconn *sc, struct proxy *px) if (ctx->flags & STAT_SHLGNDS) { /* cap, mode, id */ - chunk_appendf(&trash, "
cap: %s, mode: %s, id: %d", + chunk_appendf(&trash_chunk, "
cap: %s, mode: %s, id: %d", proxy_cap_str(px->cap), proxy_mode_str(px->mode), px->uuid); - chunk_appendf(&trash, "
"); + chunk_appendf(&trash_chunk, "
"); } - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "%s		%s
QueueSession rateSessionsExtra modulesExtra modules
CurMaxLimit%s%s
"); + chunk_appendf(&trash_chunk, ""); if ((px->cap & PR_CAP_BE) && px->srv && (ctx->flags & STAT_ADMIN)) { /* close the form used to enable/disable this proxy servers */ - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "Choose the action to perform on the checked servers : " "
\n", (ctx->scope_len > 0) ? scope_txt : "", STAT_SCOPE_TXT_MAXLEN); @@ -3543,14 +3550,14 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) } if (ctx->flags & STAT_HIDE_DOWN) - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "
  • Show all servers
    \n", uri->uri_prefix, "", (ctx->flags & STAT_NO_REFRESH) ? ";norefresh" : "", scope_txt); else - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "
  • Hide 'DOWN' servers
    \n", uri->uri_prefix, ";up", @@ -3559,14 +3566,14 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) if (uri->refresh > 0) { if (ctx->flags & STAT_NO_REFRESH) - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "
  • Enable refresh
    \n", uri->uri_prefix, (ctx->flags & STAT_HIDE_DOWN) ? ";up" : "", "", scope_txt); else - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "
  • Disable refresh
    \n", uri->uri_prefix, (ctx->flags & STAT_HIDE_DOWN) ? ";up" : "", @@ -3574,26 +3581,26 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) scope_txt); } - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "
  • Refresh now
    \n", uri->uri_prefix, (ctx->flags & STAT_HIDE_DOWN) ? ";up" : "", (ctx->flags & STAT_NO_REFRESH) ? ";norefresh" : "", scope_txt); - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "
  • CSV export
    \n", uri->uri_prefix, (uri->refresh > 0) ? ";norefresh" : "", scope_txt); - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "
  • JSON export (schema)
    \n", uri->uri_prefix, (uri->refresh > 0) ? ";norefresh" : "", scope_txt, uri->uri_prefix); - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "" "" "External resources:
      \n" @@ -3609,7 +3616,7 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) if (ctx->st_code) { switch (ctx->st_code) { case STAT_STATUS_DONE: - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "

      " "[X] " "Action processed successfully." @@ -3619,7 +3626,7 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) scope_txt); break; case STAT_STATUS_NONE: - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "

      " "[X] " "Nothing has changed." @@ -3629,7 +3636,7 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) scope_txt); break; case STAT_STATUS_PART: - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "

      " "[X] " "Action partially processed.
      " @@ -3640,7 +3647,7 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) scope_txt); break; case STAT_STATUS_ERRP: - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "

      " "[X] " "Action not processed because of invalid parameters." @@ -3656,7 +3663,7 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) scope_txt); break; case STAT_STATUS_EXCD: - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "

      " "[X] " "Action not processed : the buffer couldn't store all the data.
      " @@ -3667,7 +3674,7 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) scope_txt); break; case STAT_STATUS_DENY: - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "

      " "[X] " "Action denied." @@ -3677,7 +3684,7 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) scope_txt); break; case STAT_STATUS_IVAL: - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "

      " "[X] " "Invalid requests (unsupported method or chunked encoded request)." @@ -3687,7 +3694,7 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) scope_txt); break; default: - chunk_appendf(&trash, + chunk_appendf(&trash_chunk, "

      " "[X] " "Unexpected result." @@ -3696,33 +3703,33 @@ static void stats_dump_html_info(struct stconn *sc, struct uri_auth *uri) (ctx->flags & STAT_NO_REFRESH) ? ";norefresh" : "", scope_txt); } - chunk_appendf(&trash, "

      \n"); + chunk_appendf(&trash_chunk, "

      \n"); } } -/* Dumps the HTML stats trailer block to the trash. The caller is responsible - * for clearing the trash if needed. +/* Dumps the HTML stats trailer block to the local trash buffer. The caller is + * responsible for clearing the local trash buffer if needed. */ static void stats_dump_html_end() { - chunk_appendf(&trash, "\n"); + chunk_appendf(&trash_chunk, "\n"); } -/* Dumps the stats JSON header to the trash buffer which. The caller is responsible - * for clearing it if needed. +/* Dumps the stats JSON header to the local trash buffer buffer which. The + * caller is responsible for clearing it if needed. */ static void stats_dump_json_header() { - chunk_strcat(&trash, "["); + chunk_strcat(&trash_chunk, "["); } -/* Dumps the JSON stats trailer block to the trash. The caller is responsible - * for clearing the trash if needed. +/* Dumps the JSON stats trailer block to the local trash buffer. The caller is + * responsible for clearing the local trash buffer if needed. */ static void stats_dump_json_end() { - chunk_strcat(&trash, "]\n"); + chunk_strcat(&trash_chunk, "]\n"); } /* Uses as a pointer to the current proxy and as @@ -3786,7 +3793,7 @@ static int stats_dump_stat_to_buffer(struct stconn *sc, struct htx *htx, struct channel *rep = sc_ic(sc); enum stats_domain domain = ctx->domain; - chunk_reset(&trash); + chunk_reset(&trash_chunk); switch (ctx->state) { case STAT_STATE_INIT: @@ -3797,13 +3804,13 @@ static int stats_dump_stat_to_buffer(struct stconn *sc, struct htx *htx, if (ctx->flags & STAT_FMT_HTML) stats_dump_html_head(appctx, uri); else if (ctx->flags & STAT_JSON_SCHM) - stats_dump_json_schema(&trash); + stats_dump_json_schema(&trash_chunk); else if (ctx->flags & STAT_FMT_JSON) stats_dump_json_header(); else if (!(ctx->flags & STAT_FMT_TYPED)) stats_dump_csv_header(ctx->domain); - if (!stats_putchk(rep, htx, &trash)) + if (!stats_putchk(rep, htx, &trash_chunk)) goto full; if (ctx->flags & STAT_JSON_SCHM) { @@ -3816,7 +3823,7 @@ static int stats_dump_stat_to_buffer(struct stconn *sc, struct htx *htx, case STAT_STATE_INFO: if (ctx->flags & STAT_FMT_HTML) { stats_dump_html_info(sc, uri); - if (!stats_putchk(rep, htx, &trash)) + if (!stats_putchk(rep, htx, &trash_chunk)) goto full; } @@ -3855,7 +3862,7 @@ static int stats_dump_stat_to_buffer(struct stconn *sc, struct htx *htx, stats_dump_html_end(); else stats_dump_json_end(); - if (!stats_putchk(rep, htx, &trash)) + if (!stats_putchk(rep, htx, &trash_chunk)) goto full; } @@ -4375,6 +4382,7 @@ static void http_stats_io_handler(struct appctx *appctx) } if (appctx->st0 == STAT_HTTP_DUMP) { + trash_chunk = b_make(trash.area, channel_htx_recv_limit(res, res_htx), 0, 0); if (stats_dump_stat_to_buffer(sc, res_htx, s->be->uri_auth)) appctx->st0 = STAT_HTTP_DONE; } @@ -4619,18 +4627,18 @@ static int stats_dump_info_to_buffer(struct stconn *sc) if (!stats_fill_info(info, INF_TOTAL_FIELDS, ctx->flags)) return 0; - chunk_reset(&trash); + chunk_reset(&trash_chunk); more: current_field = ctx->field; if (ctx->flags & STAT_FMT_TYPED) - ret = stats_dump_typed_info_fields(&trash, info, ctx); + ret = stats_dump_typed_info_fields(&trash_chunk, info, ctx); else if (ctx->flags & STAT_FMT_JSON) - ret = stats_dump_json_info_fields(&trash, info, ctx); + ret = stats_dump_json_info_fields(&trash_chunk, info, ctx); else - ret = stats_dump_info_fields(&trash, info, ctx); + ret = stats_dump_info_fields(&trash_chunk, info, ctx); - if (applet_putchk(appctx, &trash) == -1) { + if (applet_putchk(appctx, &trash_chunk) == -1) { /* restore previous field */ ctx->field = current_field; return 0; @@ -4860,11 +4868,12 @@ static void stats_dump_json_schema(struct buffer *out) */ static int stats_dump_json_schema_to_buffer(struct appctx *appctx) { - chunk_reset(&trash); - stats_dump_json_schema(&trash); + chunk_reset(&trash_chunk); - if (applet_putchk(appctx, &trash) == -1) + stats_dump_json_schema(&trash_chunk); + + if (applet_putchk(appctx, &trash_chunk) == -1) return 0; return 1; @@ -5074,6 +5083,7 @@ static int cli_parse_show_stat(char **args, char *payload, struct appctx *appctx static int cli_io_handler_dump_info(struct appctx *appctx) { + trash_chunk = b_make(trash.area, trash.size, 0, 0); return stats_dump_info_to_buffer(appctx_sc(appctx)); } @@ -5082,11 +5092,13 @@ static int cli_io_handler_dump_info(struct appctx *appctx) */ static int cli_io_handler_dump_stat(struct appctx *appctx) { + trash_chunk = b_make(trash.area, trash.size, 0, 0); return stats_dump_stat_to_buffer(appctx_sc(appctx), NULL, NULL); } static int cli_io_handler_dump_json_schema(struct appctx *appctx) { + trash_chunk = b_make(trash.area, trash.size, 0, 0); return stats_dump_json_schema_to_buffer(appctx); }