BUG/MAJOR: http: always ensure response buffer has some room for a response

Since 1.5-dev12 and commit 3bf1b2b8 (MAJOR: channel: stop relying on
BF_FULL to take action), the HTTP parser switched to channel_full()
instead of BF_FULL to decide whether a buffer had enough room to start
parsing a request or response. The problem is that channel_full()
intentionally ignores outgoing data, so a corner case exists where a
large response might still be left in a response buffer with just a
few bytes left (much less than the reserve), enough to accept a second
response past the last data, but not enough to permit the HTTP processor
to add some headers. Since all the processing relies on this space being
available, we can get some random crashes when clients pipeline requests.

The analysis of a core from haproxy configured with 20480 bytes buffers
shows this : with enough "luck", when sending back the response for the
first request, the client is slow, the TCP window is congested, the socket
buffers are full, and haproxy's buffer fills up. We still have 20230 bytes
of response data in a 20480 response buffer. The second request is sent to
the server which returns 214 bytes which fit in the small 250 bytes left
in this buffer. And the buffer arrangement makes it possible to escape all
the controls in http_wait_for_response() :

    |<------ response buffer = 20480 bytes ------>|
    [ 2/2  | 3 | 4 |          1/2                 ]
           ^ start of circular buffer

      1/2 = beginning of previous response (18240)
      2/2 = end of previous response       (1990)
        3 = current response               (214)
        4 = free space                     (36)

  - channel_full() returns false (20230 bytes are going to leave)
  - the response headers does not wrap at the end of the buffer
  - the remaining linear room after the headers is larger than the
    reserve, because it's the previous response which wraps :
  => response is processed

Header rewriting causes it to reach 260 bytes, 10 bytes larger than what
the buffer could hold. So all computations during header addition are
wrong and lead to the corruption we've observed.

All the conditions are very hard to meet (which explains why it took
almost one year for this bug to show up) and are almost impossible to
reproduce on purpose on a test platform. But the bug is clearly there.

This issue was reported by Dinko Korunic who kindly devoted a lot of
time to provide countless traces and cores, and to experiment with
troubleshooting patches to knock the bug down. Thanks Dinko!

No backport is needed, but all 1.5-dev versions between dev12 and dev18
included must be upgraded. A workaround consists in setting option
forceclose to prevent pipelined requests from being processed.

diff --git a/include/proto/channel.h b/include/proto/channel.h
index 44cb890123914a16f02a09442c926731a1a393da..49a68b26bb4d815f6c776be7f94e2d64c0a69599 100644 (file)
--- a/include/proto/channel.h
+++ b/include/proto/channel.h
@@ -102,10 +102,28 @@ static inline unsigned int channel_is_empty(struct channel *c)
        return !(c->buf->o | (long)c->pipe);
 }
 
-/* Returns non-zero if the buffer input is considered full. The reserved space
- * is taken into account if ->to_forward indicates that an end of transfer is
- * close to happen. The test is optimized to avoid as many operations as
- * possible for the fast case and to be used as an "if" condition.
+/* Returns non-zero if the buffer input has all of its reserve available. This
+ * is used to decide when a request or response may be parsed when some data
+ * from a previous exchange might still be present.
+ */
+static inline int channel_reserved(const struct channel *chn)
+{
+       int rem = chn->buf->size;
+
+       rem -= chn->buf->o;
+       rem -= chn->buf->i;
+       rem -= global.tune.maxrewrite;
+       return rem >= 0;
+}
+
+/* Returns non-zero if the buffer input is considered full. This is used to
+ * decide when to stop reading into a buffer when we want to ensure that we
+ * leave the reserve untouched after all pending outgoing data are forwarded.
+ * The reserved space is taken into account if ->to_forward indicates that an
+ * end of transfer is close to happen. Note that both ->buf->o and ->to_forward
+ * are considered as available since they're supposed to leave the buffer. The
+ * test is optimized to avoid as many operations as possible for the fast case
+ * and to be used as an "if" condition.
  */
 static inline int channel_full(const struct channel *chn)
 {

diff --git a/src/proto_http.c b/src/proto_http.c
index d1bf51ead094fdb713449283a77ea19e8f8f2293..d4e93e592a7b7bd5548e9320340515213bb3daa2 100644 (file)
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -2219,11 +2219,8 @@ int http_wait_for_request(struct session *s, struct channel *req, int an_bit)
         * data later, which is much more complicated.
         */
        if (buffer_not_empty(req->buf) && msg->msg_state < HTTP_MSG_ERROR) {
-               if ((txn->flags & TX_NOT_FIRST) &&
-                   unlikely(channel_full(req) ||
-                            bi_end(req->buf) < b_ptr(req->buf, msg->next) ||
-                            bi_end(req->buf) > req->buf->data + req->buf->size - global.tune.maxrewrite)) {
-                       if (req->buf->o) {
+               if (txn->flags & TX_NOT_FIRST) {
+                       if (unlikely(!channel_reserved(req))) {
                                if (req->flags & (CF_SHUTW|CF_SHUTW_NOW|CF_WRITE_ERROR|CF_WRITE_TIMEOUT))
                                        goto failed_keep_alive;
                                /* some data has still not left the buffer, wake us once that's done */
@@ -2231,9 +2228,9 @@ int http_wait_for_request(struct session *s, struct channel *req, int an_bit)
                                req->flags |= CF_READ_DONTWAIT; /* try to get back here ASAP */
                                return 0;
                        }
-                       if (bi_end(req->buf) < b_ptr(req->buf, msg->next) ||
-                           bi_end(req->buf) > req->buf->data + req->buf->size - global.tune.maxrewrite)
-                               buffer_slow_realign(msg->chn->buf);
+                       if (unlikely(bi_end(req->buf) < b_ptr(req->buf, msg->next) ||
+                                    bi_end(req->buf) > req->buf->data + req->buf->size - global.tune.maxrewrite))
+                               buffer_slow_realign(req->buf);
                }
 
                /* Note that we have the same problem with the response ; we
@@ -2244,7 +2241,7 @@ int http_wait_for_request(struct session *s, struct channel *req, int an_bit)
                 * keep-alive requests.
                 */
                if ((txn->flags & TX_NOT_FIRST) &&
-                   unlikely(channel_full(s->rep) ||
+                   unlikely(!channel_reserved(s->rep) ||
                             bi_end(s->rep->buf) < b_ptr(s->rep->buf, txn->rsp.next) ||
                             bi_end(s->rep->buf) > s->rep->buf->data + s->rep->buf->size - global.tune.maxrewrite)) {
                        if (s->rep->buf->o) {
@@ -5004,21 +5001,19 @@ int http_wait_for_response(struct session *s, struct channel *rep, int an_bit)
         * data later, which is much more complicated.
         */
        if (buffer_not_empty(rep->buf) && msg->msg_state < HTTP_MSG_ERROR) {
-               if (unlikely(channel_full(rep) ||
-                            bi_end(rep->buf) < b_ptr(rep->buf, msg->next) ||
-                            bi_end(rep->buf) > rep->buf->data + rep->buf->size - global.tune.maxrewrite)) {
-                       if (rep->buf->o) {
-                               /* some data has still not left the buffer, wake us once that's done */
-                               if (rep->flags & (CF_SHUTW|CF_SHUTW_NOW|CF_WRITE_ERROR|CF_WRITE_TIMEOUT))
-                                       goto abort_response;
-                               channel_dont_close(rep);
-                               rep->flags |= CF_READ_DONTWAIT; /* try to get back here ASAP */
-                               return 0;
-                       }
-                       if (rep->buf->i <= rep->buf->size - global.tune.maxrewrite)
-                               buffer_slow_realign(msg->chn->buf);
+               if (unlikely(!channel_reserved(rep))) {
+                       /* some data has still not left the buffer, wake us once that's done */
+                       if (rep->flags & (CF_SHUTW|CF_SHUTW_NOW|CF_WRITE_ERROR|CF_WRITE_TIMEOUT))
+                               goto abort_response;
+                       channel_dont_close(rep);
+                       rep->flags |= CF_READ_DONTWAIT; /* try to get back here ASAP */
+                       return 0;
                }
 
+               if (unlikely(bi_end(rep->buf) < b_ptr(rep->buf, msg->next) ||
+                            bi_end(rep->buf) > rep->buf->data + rep->buf->size - global.tune.maxrewrite))
+                       buffer_slow_realign(rep->buf);
+
                if (likely(msg->next < rep->buf->i))
                        http_msg_analyzer(msg, &txn->hdr_idx);
        }