MAJOR: http: do not use msg->sol while processing messages or forwarding data

There are still some pending issues in the gzip compressor, and fixing
them requires a better handling of intermediate parsing states.

Another issue to deal with is the rewinding of a buffer during a redispatch
when a load balancing algorithm involves L7 data because the exact amount of
data to rewind is not clear. At the moment, this is handled by unwinding all
pending data, which cannot work in responses due to pipelining.

Last, having a first analysis which parses the body and another one which
restarts from where the parsing was left is wrong. Right now it only works
because we never both parse and transform in the same direction. But that
is wrong anyway.

In order to address the first issue, we'll have to use msg->eoh + msg->eol
to find the end of headers, and we still need to store the information about
the forwarded header length somewhere (msg->sol might be reused for this).

msg->sov may only be used for the start of data and not for subsequent chunks
if possible. This first implies that we stop sharing it with header length,
and stop using msg->sol there. In fact we don't need it already as it is
always zero when reaching the HTTP_MSG_BODY state. It was only updated to
reflect a copy of msg->sov.

So now as a first step into that direction, this patch ensure that msg->sol
is never re-assigned after being set to zero and is not used anymore when
we're dealing with HTTP processing and forwarding. We'll later reuse it
differently but for now it's secured.

The patch does nothing magic, it only removes msg->sol everywhere it was
already zero and avoids setting it. In order to keep the sov-sol difference,
it now resets sov after forwarding data. In theory there's no problem here,
but the patch is still tagged major because that code is complex.

diff --git a/include/types/proto_http.h b/include/types/proto_http.h
index 95860c05e5ab7d7148f567e1499eae34b546a301..d937ce1f3ca41eb781dc6fd1af42a874c0466331 100644 (file)
--- a/include/types/proto_http.h
+++ b/include/types/proto_http.h
@@ -324,9 +324,17 @@ enum {
  *                             so that we can rewind the buffer to change some
  *                             headers if needed (eg: http-send-name-header).
  *
- *  - sov                    : When in HTTP_MSG_BODY, will point to the first
- *                             byte of data (relative to buffer's origin).
- *  - sol (start of line)    : start of current line during parsing, or zero.
+ *  - sov (start of value)   : Before HTTP_MSG_BODY, points to the value of
+ *                             the header being parsed. Starting from
+ *                             HTTP_MSG_BODY, will point to the start of the
+ *                             body (relative to buffer's origin), or to data
+ *                             following a chunk size. Thus <sov> bytes of
+ *                             headers will have to be sent only once.
+ *
+ *  - next (parse pointer)   : next relative byte to be parsed. Always points
+ *                             to a byte matching the current state.
+ *
+ *  - sol (start of line)    : start of current line before MSG_BODY, or zero.
  *
  *  - eol (End of Line)      : Before HTTP_MSG_BODY, relative offset in the
  *                             buffer of the first byte which marks the end of

diff --git a/src/compression.c b/src/compression.c
index 3c53a077bca4976788548e2f6eb7fbef928195da..868083263cf92b7ac93d7ca592312f0cfc3796b6 100644 (file)
--- a/src/compression.c
+++ b/src/compression.c
@@ -142,7 +142,6 @@ int http_compression_buffer_init(struct session *s, struct buffer *in, struct bu
        b_adv(in, msg->next);
        msg->next = 0;
        msg->sov = 0;
-       msg->sol = 0;
 
        out->size = global.tune.bufsize;
        out->i = 0;
@@ -182,7 +181,6 @@ int http_compression_buffer_add_data(struct session *s, struct buffer *in, struc
        b_adv(in, msg->next);
        msg->next = 0;
        msg->sov = 0;
-       msg->sol = 0;
 
        /*
         * select the smallest size between the announced chunk size, the input

diff --git a/src/proto_http.c b/src/proto_http.c
index 7c0b7fe735974062d621c4bba780a9b8a57ef938..4c78af814134b46f795c59e6eeb0a7ac9489b972 100644 (file)
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -1924,8 +1924,8 @@ void http_change_connection_header(struct http_txn *txn, struct http_msg *msg, i
 }
 
 /* Parse the chunk size at msg->next. Once done, it adjusts ->next to point to the
- * first byte of body, and increments msg->sov by the number of bytes parsed,
- * so that we know we can forward between ->sol and ->sov.
+ * first byte of body, and increments msg->sov by the number of bytes parsed.
+ * so that we know we can forward ->sov bytes.
  * Return >0 on success, 0 when some data is missing, <0 on error.
  * Note: this function is designed to parse wrapped CRLF at the end of the buffer.
  */
@@ -2034,10 +2034,9 @@ static inline int http_parse_chunk_size(struct http_msg *msg)
  * change anything except maybe msg->next and msg->sov. Note that the message
  * must already be in HTTP_MSG_TRAILERS state before calling this function,
  * which implies that all non-trailers data have already been scheduled for
- * forwarding, and that the difference between msg->sol and msg->sov exactly
- * matches the length of trailers already parsed and not forwarded. It is also
- * important to note that this function is designed to be able to parse wrapped
- * headers at end of buffer.
+ * forwarding, and that msg->sov exactly matches the length of trailers already
+ * parsed and not forwarded. It is also important to note that this function is
+ * designed to be able to parse wrapped headers at end of buffer.
  */
 static int http_forward_trailers(struct http_msg *msg)
 {
@@ -2105,7 +2104,7 @@ static int http_forward_trailers(struct http_msg *msg)
 
 /* This function may be called only in HTTP_MSG_CHUNK_CRLF. It reads the CRLF or
  * a possible LF alone at the end of a chunk. It automatically adjusts msg->sov,
- * ->sol, ->next in order to include this part into the next forwarding phase.
+ * and ->next in order to include this part into the next forwarding phase.
  * Note that the caller must ensure that ->p points to the first byte to parse.
  * It also sets msg_state to HTTP_MSG_CHUNK_SIZE and returns >0 on success. If
  * not enough data are available, the function does not change anything and
@@ -2143,8 +2142,7 @@ static inline int http_skip_chunk_crlf(struct http_msg *msg)
        ptr++;
        if (unlikely(ptr >= buf->data + buf->size))
                ptr = buf->data;
-       /* prepare the CRLF to be forwarded (between ->sol and ->sov) */
-       msg->sol = 0;
+       /* prepare the CRLF to be forwarded (->sov) */
        msg->sov = msg->next = bytes;
        msg->msg_state = HTTP_MSG_CHUNK_SIZE;
        return 1;
@@ -2439,7 +2437,7 @@ int http_wait_for_request(struct session *s, struct channel *req, int an_bit)
         *   msg->next          = first non-visited byte
         *
         * At end of parsing, we may perform a capture of the error (if any), and
-        * we will set a few fields (msg->sol, txn->meth, sn->flags/SN_REDIRECTABLE).
+        * we will set a few fields (txn->meth, sn->flags/SN_REDIRECTABLE).
         * We also check for monitor-uri, logging, HTTP/0.9 to 1.0 conversion, and
         * finally headers capture.
         */
@@ -3378,7 +3376,7 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct session *
                host = "";
                hostlen = 0;
                ctx.idx = 0;
-               if (http_find_header2("Host", 4, txn->req.chn->buf->p + txn->req.sol, &txn->hdr_idx, &ctx)) {
+               if (http_find_header2("Host", 4, txn->req.chn->buf->p, &txn->hdr_idx, &ctx)) {
                        host = ctx.line + ctx.val;
                        hostlen = ctx.vlen;
                }
@@ -4324,9 +4322,9 @@ int http_wait_for_request_body(struct session *s, struct channel *req, int an_bi
                }
 
                /* we have msg->sov which points to the first byte of message body.
-                * req->buf->p still points to the beginning of the message and msg->sol
-                * is still null. We must save the body in msg->next because it
-                * survives buffer re-alignments.
+                * req->buf->p still points to the beginning of the message. We
+                * must save the body in msg->next because it survives buffer
+                * re-alignments.
                 */
                msg->next = msg->sov;
 
@@ -4450,6 +4448,8 @@ int http_send_name_header(struct http_txn *txn, struct proxy* be, const char* sr
        if (old_o) {
                /* The request was already skipped, let's restore it */
                b_rew(chn->buf, old_o);
+               txn->req.next += old_o;
+               txn->req.sov  += old_o;
        }
 
        old_i = chn->buf->i;
@@ -4470,12 +4470,13 @@ int http_send_name_header(struct http_txn *txn, struct proxy* be, const char* sr
        if (old_o) {
                /* If this was a forwarded request, we must readjust the amount of
                 * data to be forwarded in order to take into account the size
-                * variations. Note that if the request was already scheduled for
-                * forwarding, it had its req->sol pointing to the body, which
-                * must then be updated too.
+                * variations. Note that the current state is >= HTTP_MSG_BODY,
+                * so we don't have to adjust ->sol.
                 */
-               txn->req.sol += chn->buf->i - old_i;
-               b_adv(chn->buf, old_o + chn->buf->i - old_i);
+               old_o += chn->buf->i - old_i;
+               b_adv(chn->buf, old_o);
+               txn->req.next -= old_o;
+               txn->req.sov  -= old_o;
        }
 
        return 0;
@@ -5044,9 +5045,9 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
 
        if (msg->msg_state < HTTP_MSG_CHUNK_SIZE) {
                /* we have msg->sov which points to the first byte of message body.
-                * req->buf->p still points to the beginning of the message and msg->sol
-                * is still null. We must save the body in msg->next because it
-                * survives buffer re-alignments.
+                * req->buf->p still points to the beginning of the message. We
+                * must save the body in msg->next because it survives buffer
+                * re-alignments.
                 */
                msg->next = msg->sov;
 
@@ -5060,16 +5061,14 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
        channel_auto_close(req);
 
        while (1) {
-               unsigned int bytes;
-
                http_silent_debug(__LINE__, s);
-               /* we may have some data pending between sol and sov */
-               bytes = msg->sov - msg->sol;
-               if (msg->chunk_len || bytes) {
-                       msg->sol = msg->sov;
-                       msg->next -= bytes; /* will be forwarded */
-                       msg->chunk_len += bytes;
+
+               /* we may have some pending data starting at req->buf->p */
+               if (msg->chunk_len || msg->sov) {
+                       msg->chunk_len += msg->sov;
                        msg->chunk_len -= channel_forward(req, msg->chunk_len);
+                       msg->next      -= msg->sov;
+                       msg->sov        = 0;
                }
 
                if (msg->msg_state == HTTP_MSG_DATA) {
@@ -6160,7 +6159,6 @@ int http_response_forward_body(struct session *s, struct channel *res, int an_bi
 {
        struct http_txn *txn = &s->txn;
        struct http_msg *msg = &s->txn.rsp;
-       unsigned int bytes;
        static struct buffer *tmpbuf = NULL;
        int compressing = 0;
        int consumed_data = 0;
@@ -6191,8 +6189,8 @@ int http_response_forward_body(struct session *s, struct channel *res, int an_bi
 
        if (msg->msg_state < HTTP_MSG_CHUNK_SIZE) {
                /* we have msg->sov which points to the first byte of message body.
-                * rep->buf.p still points to the beginning of the message and msg->sol
-                * is still null. We forward the headers, we don't need them.
+                * res->buf.p still points to the beginning of the message. We
+                * forward the headers, we don't need them.
                 */
                channel_forward(res, msg->sov);
                msg->next = 0;
@@ -6215,14 +6213,14 @@ int http_response_forward_body(struct session *s, struct channel *res, int an_bi
 
        while (1) {
                http_silent_debug(__LINE__, s);
-               /* we may have some data pending between sol and sov */
+
+               /* we may have some pending data starting at res->buf->p */
                if (s->comp_algo == NULL) {
-                       bytes = msg->sov - msg->sol;
-                       if (msg->chunk_len || bytes) {
-                               msg->sol = msg->sov;
-                               msg->next -= bytes; /* will be forwarded */
-                               msg->chunk_len += bytes;
+                       if (msg->chunk_len || msg->sov) {
+                               msg->chunk_len += msg->sov;
                                msg->chunk_len -= channel_forward(res, msg->chunk_len);
+                               msg->next      -= msg->sov;
+                               msg->sov        = 0;
                        }
                }
 
@@ -6268,7 +6266,6 @@ int http_response_forward_body(struct session *s, struct channel *res, int an_bi
                                b_adv(res->buf, msg->next);
                                msg->next = 0;
                                msg->sov = 0;
-                               msg->sol = 0;
                        }
                        /* we're in MSG_CHUNK_SIZE now, fall through */
 
@@ -6292,7 +6289,6 @@ int http_response_forward_body(struct session *s, struct channel *res, int an_bi
                                        b_adv(res->buf, msg->next);
                                        msg->next = 0;
                                        msg->sov = 0;
-                                       msg->sol = 0;
                                } else {
                                        if (consumed_data) {
                                                http_compression_buffer_end(s, &res->buf, &tmpbuf, 1);
@@ -6381,14 +6377,13 @@ int http_response_forward_body(struct session *s, struct channel *res, int an_bi
        if (!s->req->analysers)
                goto return_bad_res;
 
-       /* forward any data pending between sol and sov */
+       /* forward any pending data starting at res->buf->p */
        if (s->comp_algo == NULL) {
-               bytes = msg->sov - msg->sol;
-               if (msg->chunk_len || bytes) {
-                       msg->sol = msg->sov;
-                       msg->next -= bytes; /* will be forwarded */
-                       msg->chunk_len += bytes;
+               if (msg->chunk_len || msg->sov) {
+                       msg->chunk_len += msg->sov;
                        msg->chunk_len -= channel_forward(res, msg->chunk_len);
+                       msg->next      -= msg->sov;
+                       msg->sov        = 0;
                }
        }
 
@@ -9622,8 +9617,7 @@ smp_fetch_base(struct proxy *px, struct session *l4, void *l7, unsigned int opt,
        CHECK_HTTP_MESSAGE_FIRST();
 
        ctx.idx = 0;
-       if (!http_find_header2("Host", 4, txn->req.chn->buf->p + txn->req.sol, &txn->hdr_idx, &ctx) ||
-           !ctx.vlen)
+       if (!http_find_header2("Host", 4, txn->req.chn->buf->p, &txn->hdr_idx, &ctx) || !ctx.vlen)
                return smp_fetch_path(px, l4, l7, opt, args, smp, kw);
 
        /* OK we have the header value in ctx.line+ctx.val for ctx.vlen bytes */
@@ -9633,7 +9627,7 @@ smp_fetch_base(struct proxy *px, struct session *l4, void *l7, unsigned int opt,
        smp->data.str.len = ctx.vlen;
 
        /* now retrieve the path */
-       end = txn->req.chn->buf->p + txn->req.sol + txn->req.sl.rq.u + txn->req.sl.rq.u_l;
+       end = txn->req.chn->buf->p + txn->req.sl.rq.u + txn->req.sl.rq.u_l;
        beg = http_get_path(txn);
        if (!beg)
                beg = end;
@@ -9670,7 +9664,7 @@ smp_fetch_base32(struct proxy *px, struct session *l4, void *l7, unsigned int op
        CHECK_HTTP_MESSAGE_FIRST();
 
        ctx.idx = 0;
-       if (http_find_header2("Host", 4, txn->req.chn->buf->p + txn->req.sol, &txn->hdr_idx, &ctx)) {
+       if (http_find_header2("Host", 4, txn->req.chn->buf->p, &txn->hdr_idx, &ctx)) {
                /* OK we have the header value in ctx.line+ctx.val for ctx.vlen bytes */
                ptr = ctx.line + ctx.val;
                len = ctx.vlen;
@@ -9679,7 +9673,7 @@ smp_fetch_base32(struct proxy *px, struct session *l4, void *l7, unsigned int op
        }
 
        /* now retrieve the path */
-       end = txn->req.chn->buf->p + txn->req.sol + txn->req.sl.rq.u + txn->req.sl.rq.u_l;
+       end = txn->req.chn->buf->p + txn->req.sl.rq.u + txn->req.sl.rq.u_l;
        beg = http_get_path(txn);
        if (!beg)
                beg = end;
@@ -10382,7 +10376,7 @@ smp_fetch_url32(struct proxy *px, struct session *l4, void *l7, unsigned int opt
        CHECK_HTTP_MESSAGE_FIRST();
 
        ctx.idx = 0;
-       if (http_find_header2("Host", 4, txn->req.chn->buf->p + txn->req.sol, &txn->hdr_idx, &ctx)) {
+       if (http_find_header2("Host", 4, txn->req.chn->buf->p, &txn->hdr_idx, &ctx)) {
                /* OK we have the header value in ctx.line+ctx.val for ctx.vlen bytes */
                ptr = ctx.line + ctx.val;
                len = ctx.vlen;
@@ -10391,7 +10385,7 @@ smp_fetch_url32(struct proxy *px, struct session *l4, void *l7, unsigned int opt
        }
 
        /* now retrieve the path */
-       end = txn->req.chn->buf->p + txn->req.sol + txn->req.sl.rq.u + txn->req.sl.rq.u_l;
+       end = txn->req.chn->buf->p + txn->req.sl.rq.u + txn->req.sl.rq.u_l;
        beg = http_get_path(txn);
        if (!beg)
                beg = end;