From: Christopher Faulet <cfaulet@haproxy.com>
Date: Wed, 27 Feb 2019 14:30:57 +0000 (+0100)
Subject: BUG/MAJOR: stats: Fix how huge POST data are read from the channel
X-Git-Tag: v2.0-dev2~46
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2f9a41d52b28b88d44aab063bb2b88025a388981;p=thirdparty%2Fhaproxy.git

BUG/MAJOR: stats: Fix how huge POST data are read from the channel

When the body length is greater than a chunk size (so if length of POST data
exceeds the buffer size), the requests is rejected with the status code
STAT_STATUS_EXCD. Otherwise the stats applet will wait to have all the data to
copy and parse them. But there is a problem when the total request size
(including the headers) is just lower than the buffer size but greater the
buffer size less the reserve. In such case, the body length is considered as
enough small to be processed but not entierly received. So the stats applet
waits for more data. But because outgoing data are still there, the channel's
buffer is considered as full and nothing more can be read, leading to a freeze
of the session.

Note this bug is pretty easy to reproduce with the legacy HTTP. It is harder
with the HTX but still possible. To fix the bug, in the stats applet, when the
request is not fully received, we check if at least the reserve remains
available the channel's buffer.

This patch must be backported as far as 1.5. But because the HTX does not exist
in 1.8 and lower, it will have to be adapted for these versions.
---

diff --git a/src/stats.c b/src/stats.c
index 7c20453d94..5e32ffc19f 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -2553,6 +2553,7 @@ static void stats_dump_html_info(struct stream_interface *si, struct uri_auth *u
 			              "Action not processed because of invalid parameters."
 			              "<ul>"
 			              "<li>The action is maybe unknown.</li>"
+				      "<li>Invalid key parameter (empty or too long).</li>"
 			              "<li>The backend name is probably unknown or ambiguous (duplicated names).</li>"
 			              "<li>Some server names are probably unknown or ambiguous (duplicated names in the backend).</li>"
 			              "</ul>"
@@ -2761,41 +2762,21 @@ static int stats_process_http_post(struct stream_interface *si)
 	if (IS_HTX_STRM(s)) {
 		struct htx *htx = htxbuf(&s->req.buf);
 		struct htx_blk *blk;
-		size_t count = co_data(&s->req);
 
-		/* we need more data */
-		if (htx->extra || htx->data > count) {
-			appctx->ctx.stats.st_code = STAT_STATUS_NONE;
-			return 0;
-		}
-
-		/* Skip the headers */
-		blk = htx_get_head_blk(htx);
-		while (count && blk) {
-			enum htx_blk_type type = htx_get_blk_type(blk);
-			uint32_t sz = htx_get_blksz(blk);
-
-			if (sz > count) {
-				appctx->ctx.stats.st_code = STAT_STATUS_NONE;
-				return 0;
+		/*  we need more data */
+		if (s->txn->req.msg_state < HTTP_MSG_DONE) {
+			/* check if we can receive more */
+			if (htx_free_data_space(htx) <= global.tune.maxrewrite) {
+				appctx->ctx.stats.st_code = STAT_STATUS_EXCD;
+				goto out;
 			}
-
-			count -= sz;
-			blk = htx_get_next_blk(htx, blk);
-
-			if (type == HTX_BLK_EOH)
-				break;
-		}
-
-		/* too large request */
-		if (count > b_size(temp)) {
-			appctx->ctx.stats.st_code = STAT_STATUS_EXCD;
-			goto out;
+			goto wait;
 		}
 
-		while (count && blk) {
+		/* The request was fully received. Copy data */
+		blk = htx_get_head_blk(htx);
+		while (blk) {
 			enum htx_blk_type type = htx_get_blk_type(blk);
-			uint32_t sz = htx_get_blksz(blk);
 
 			if (type == HTX_BLK_EOM || type == HTX_BLK_EOD)
 				break;
@@ -2807,26 +2788,26 @@ static int stats_process_http_post(struct stream_interface *si)
 					goto out;
 				}
 			}
-
-			count -= sz;
 			blk = htx_get_next_blk(htx, blk);
 		}
 	}
 	else {
 		int reql;
 
-		if (temp->size < s->txn->req.body_len) {
-			/* too large request */
-			appctx->ctx.stats.st_code = STAT_STATUS_EXCD;
-			goto out;
+		/* we need more data */
+		if (s->txn->req.msg_state < HTTP_MSG_DONE) {
+			/* check if we can receive more */
+			if (c_room(&s->req) <= global.tune.maxrewrite) {
+				appctx->ctx.stats.st_code = STAT_STATUS_EXCD;
+				goto out;
+			}
+			goto wait;
 		}
-
 		reql = co_getblk(si_oc(si), temp->area, s->txn->req.body_len,
 				 s->txn->req.eoh + 2);
 		if (reql <= 0) {
-			/* we need more data */
-			appctx->ctx.stats.st_code = STAT_STATUS_NONE;
-			return 0;
+			appctx->ctx.stats.st_code = STAT_STATUS_EXCD;
+			goto out;
 		}
 		temp->data = reql;
 	}
@@ -2857,7 +2838,7 @@ static int stats_process_http_post(struct stream_interface *si)
 				strncpy(key, cur_param + poffset, plen);
 				key[plen - 1] = '\0';
 			} else {
-				appctx->ctx.stats.st_code = STAT_STATUS_EXCD;
+				appctx->ctx.stats.st_code = STAT_STATUS_ERRP;
 				goto out;
 			}
 
@@ -3113,6 +3094,9 @@ static int stats_process_http_post(struct stream_interface *si)
 	}
  out:
 	return 1;
+ wait:
+	appctx->ctx.stats.st_code = STAT_STATUS_NONE;
+	return 0;
 }