From 52a3d807fc332b57b62f5e30aa6f697636a22695 Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Wed, 16 Oct 2024 17:30:16 +0200 Subject: [PATCH] BUG/MAJOR: filters/htx: Add a flag to state the payload is altered by a filter When a filter is registered on the data, it means it may change the payload length by rewritting data. It means consumers of the message cannot trust the expected length of payload as announced by the producer. The commit 8bd835b2d2 ("MEDIUM: filters/htx: Don't rely on HTX extra field if payload is filtered") was pushed to solve this issue. When the HTTP payload of a message is filtered, the extra field is set to 0 to be sure it will never be used by error by any consumer. However, it is not enough. Indeed, the filters must be called before fowarding some data. They cannot be by-passed. But if a consumer is unable to flush the HTX message, some outgoing data can remain blocked in the channel's buffer. If some new data are then pushed because there is some room in the channel's buffe, the producer will set the HTX extra field. At this stage, if the consumer is unblocked and can send again data, it is possible to call it to forward outgoing data blocked in the channel's buffer before waking the stream up to filter new input data. It is the purpose of the data fast-forwarding. In this case, the HTX extra field will be seen by the consumer. It is unexpected and leads to undefined behavior. One consequence of this bug is to perform a wrong chunking on compressed messages, leading to processing errors at the end of the message, reported as "ID--" in logs. To fix the bug, a HTX flag is added to state the payload of the current HTX message is altered. When this flag is set (HTX_FL_ALTERED_PAYLOAD), the HTX extra field must not be trusted. And to keep things simple, when this flag is set, the HTX extra field is automatically set to 0 when the HTX message is loaded, in htxbuf() function. It is probably the less intrusive way to fix the bug for now. But this part must be reviewed to save meta-info of the HTX message outside of the message itself. This commit should solve the issue #2741. It must be backported as far as 2.9. --- include/haproxy/htx-t.h | 2 +- include/haproxy/htx.h | 2 ++ src/filters.c | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/include/haproxy/htx-t.h b/include/haproxy/htx-t.h index 5312ae1e23..a33da886e2 100644 --- a/include/haproxy/htx-t.h +++ b/include/haproxy/htx-t.h @@ -177,7 +177,7 @@ static forceinline char *hsl_show_flags(char *buf, size_t len, const char *delim #define HTX_FL_PARSING_ERROR 0x00000001 /* Set when a parsing error occurred */ #define HTX_FL_PROCESSING_ERROR 0x00000002 /* Set when a processing error occurred */ #define HTX_FL_FRAGMENTED 0x00000004 /* Set when the HTX buffer is fragmented */ -/* 0x00000008 unused */ +#define HTX_FL_ALTERED_PAYLOAD 0x00000008 /* The payload is altered, the extra value must not be trusted */ #define HTX_FL_EOM 0x00000010 /* Set when end-of-message is reached from the HTTP point of view * (at worst, on the EOM block is missing) */ diff --git a/include/haproxy/htx.h b/include/haproxy/htx.h index c991c81e5c..47893ef1d0 100644 --- a/include/haproxy/htx.h +++ b/include/haproxy/htx.h @@ -700,6 +700,8 @@ static inline struct htx *htxbuf(const struct buffer *buf) htx->size = buf->size - sizeof(*htx); htx_reset(htx); } + if (htx->flags & HTX_FL_ALTERED_PAYLOAD) + htx->extra = 0; return htx; } diff --git a/src/filters.c b/src/filters.c index e55adee6b4..cc89469ca3 100644 --- a/src/filters.c +++ b/src/filters.c @@ -704,6 +704,7 @@ flt_http_payload(struct stream *s, struct http_msg *msg, unsigned int len) *strm_off += ret; end: htx = htxbuf(&msg->chn->buf); + htx->flags |= HTX_FL_ALTERED_PAYLOAD; if (msg->flags & HTTP_MSGF_XFER_LEN) htx->extra = 0; DBG_TRACE_LEAVE(STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_FLT_ANA, s);