[MEDIUM] http: make safer use of the DONT_READ and AUTO_CLOSE flags

Several HTTP analysers used to set those flags to values that
were useful but without considering the possibility that they
were not called again to clean what they did. First, replace
direct flag manipulation with more explicit macros. Second,
enforce a rule stating that any buffer which changes one of
these flags from the default must restore it after completion,
so that other analysers see correct flags.

With both this fix and the previous one about analyser bits,
we should not see any more stuck sessions.

diff --git a/include/proto/buffers.h b/include/proto/buffers.h
index d5dd781bcbe85b08cd9e64bf912ea0dc02be59c5..e046f39276c0ffb6c02e8b3076f1f903f482bc41 100644 (file)
--- a/include/proto/buffers.h
+++ b/include/proto/buffers.h
@@ -252,6 +252,18 @@ static inline void buffer_dont_close(struct buffer *buf)
        buf->flags &= ~BF_AUTO_CLOSE;
 }
 
+/* allow the producer to read / poll the input */
+static inline void buffer_auto_read(struct buffer *buf)
+{
+       buf->flags &= ~BF_DONT_READ;
+}
+
+/* prevent the producer from read / poll the input */
+static inline void buffer_dont_read(struct buffer *buf)
+{
+       buf->flags |= BF_DONT_READ;
+}
+
 /* returns the maximum number of bytes writable at once in this buffer */
 static inline int buffer_max(const struct buffer *buf)
 {

diff --git a/src/proto_http.c b/src/proto_http.c
index c70543cb717b8b19e6c4092613a8f1199a3303a8..12647df230e78dc9fe970af77f939cc59bcd7eb1 100644 (file)
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -563,6 +563,7 @@ static void http_server_error(struct session *t, struct stream_interface *si,
        buffer_erase(si->ob);
        buffer_erase(si->ib);
        buffer_auto_close(si->ib);
+       buffer_auto_read(si->ib);
        if (status > 0 && msg) {
                t->txn.status = status;
                buffer_write(si->ib, msg->str, msg->len);
@@ -2282,7 +2283,6 @@ int http_wait_for_request(struct session *s, struct buffer *req, int an_bit)
 
                buffer_dont_connect(req);
                req->flags |= BF_READ_DONTWAIT; /* try to get back here ASAP */
-               req->flags &= ~BF_DONT_READ;
 
                /* just set the request timeout once at the beginning of the request */
                if (!tick_isset(req->analyse_exp))
@@ -2861,14 +2861,6 @@ int http_process_req_common(struct session *s, struct buffer *req, int an_bit, s
                }
        }
 
-       /* We can shut read side if we know how we won't transfer any more data && !abort_on_close */
-       if ((txn->flags & TX_REQ_XFER_LEN) &&
-           !(txn->flags & TX_REQ_TE_CHNK) && !txn->req.hdr_content_len &&
-           (req->cons->state == SI_ST_EST || !(s->be->options & PR_O_ABRT_CLOSE)))
-               req->flags |= BF_DONT_READ;
-       else
-               req->flags &= ~BF_DONT_READ;
-
        /* POST requests may be accompanied with an "Expect: 100-Continue" header.
         * If this happens, then the data will not come immediately, so we must
         * send all what we have without waiting. Note that due to the small gain
@@ -3398,8 +3390,8 @@ void http_end_txn_clean_session(struct session *s)
        s->req->cons->err_loc   = NULL;
        s->req->cons->exp       = TICK_ETERNITY;
        s->req->cons->flags     = SI_FL_NONE;
-       s->req->flags &= ~(BF_SHUTW|BF_SHUTW_NOW|BF_AUTO_CONNECT|BF_WRITE_ERROR|BF_STREAMER|BF_STREAMER_FAST|BF_AUTO_CLOSE);
-       s->rep->flags &= ~(BF_SHUTR|BF_SHUTR_NOW|BF_READ_ATTACHED|BF_READ_ERROR|BF_READ_NOEXP|BF_STREAMER|BF_STREAMER_FAST|BF_AUTO_CLOSE|BF_WRITE_PARTIAL);
+       s->req->flags &= ~(BF_SHUTW|BF_SHUTW_NOW|BF_AUTO_CONNECT|BF_WRITE_ERROR|BF_STREAMER|BF_STREAMER_FAST);
+       s->rep->flags &= ~(BF_SHUTR|BF_SHUTR_NOW|BF_READ_ATTACHED|BF_READ_ERROR|BF_READ_NOEXP|BF_STREAMER|BF_STREAMER_FAST|BF_WRITE_PARTIAL);
        s->flags &= ~(SN_DIRECT|SN_ASSIGNED|SN_ADDR_SET|SN_BE_ASSIGNED);
        s->flags &= ~(SN_CURR_SESS|SN_REDIRECTABLE);
        s->txn.meth = 0;
@@ -3411,14 +3403,15 @@ void http_end_txn_clean_session(struct session *s)
        /* if the request buffer is not empty, it means we're
         * about to process another request, so send pending
         * data with MSG_MORE to merge TCP packets when possible.
-        * Also, let's not start reading a small request packet,
-        * we may prefer to read a larger one later.
         */
-       s->req->flags &= ~BF_DONT_READ;
-       if (s->req->l > s->req->send_max) {
+       if (s->req->l > s->req->send_max)
                s->rep->flags |= BF_EXPECT_MORE;
-               s->req->flags |= BF_DONT_READ;
-       }
+
+       /* we're removing the analysers, we MUST re-enable events detection */
+       buffer_auto_read(s->req);
+       buffer_auto_close(s->req);
+       buffer_auto_read(s->rep);
+       buffer_auto_close(s->rep);
 
        /* make ->lr point to the first non-forwarded byte */
        s->req->lr = s->req->w + s->req->send_max;
@@ -3454,8 +3447,11 @@ int http_sync_req_state(struct session *s)
                return 0;
 
        if (txn->req.msg_state == HTTP_MSG_DONE) {
-               /* No need to read anymore, the request was completely parsed */
-               buf->flags |= BF_DONT_READ;
+               /* No need to read anymore, the request was completely parsed.
+                * We can shut the read side unless we want to abort_on_close.
+                */
+               if (buf->cons->state == SI_ST_EST || !(s->be->options & PR_O_ABRT_CLOSE))
+                       buffer_dont_read(buf);
 
                if (txn->rsp.msg_state == HTTP_MSG_ERROR)
                        goto wait_other_side;
@@ -3469,7 +3465,7 @@ int http_sync_req_state(struct session *s)
 
                if (txn->rsp.msg_state == HTTP_MSG_TUNNEL) {
                        /* if any side switches to tunnel mode, the other one does too */
-                       buf->flags &= ~BF_DONT_READ;
+                       buffer_auto_read(buf);
                        txn->req.msg_state = HTTP_MSG_TUNNEL;
                        goto wait_other_side;
                }
@@ -3510,7 +3506,7 @@ int http_sync_req_state(struct session *s)
                }
                else {
                        /* other modes are used as a tunnel right now */
-                       buf->flags &= ~BF_DONT_READ;
+                       buffer_auto_read(buf);
                        txn->req.msg_state = HTTP_MSG_TUNNEL;
                        goto wait_other_side;
                }
@@ -3562,10 +3558,11 @@ int http_sync_res_state(struct session *s)
 
        if (txn->rsp.msg_state == HTTP_MSG_DONE) {
                /* In theory, we don't need to read anymore, but we must
-                * still monitor the server connection for a possible close,
-                * so we don't set the BF_DONT_READ flag here.
+                * still monitor the server connection for a possible close
+                * while the request is being uploaded, so we don't disable
+                * reading.
                 */
-               /* buf->flags |= BF_DONT_READ; */
+               /* buffer_dont_read(buf); */
 
                if (txn->req.msg_state == HTTP_MSG_ERROR)
                        goto wait_other_side;
@@ -3581,7 +3578,7 @@ int http_sync_res_state(struct session *s)
 
                if (txn->req.msg_state == HTTP_MSG_TUNNEL) {
                        /* if any side switches to tunnel mode, the other one does too */
-                       buf->flags &= ~BF_DONT_READ;
+                       buffer_auto_read(buf);
                        txn->rsp.msg_state = HTTP_MSG_TUNNEL;
                        goto wait_other_side;
                }
@@ -3617,7 +3614,7 @@ int http_sync_res_state(struct session *s)
                         * (not implemented). These modes are used as a tunnel right
                         * now.
                         */
-                       buf->flags &= ~BF_DONT_READ;
+                       buffer_auto_read(buf);
                        txn->rsp.msg_state = HTTP_MSG_TUNNEL;
                        goto wait_other_side;
                }
@@ -3656,6 +3653,7 @@ int http_sync_res_state(struct session *s)
                /* drop any pending data */
                buffer_ignore(buf, buf->l - buf->send_max);
                buffer_auto_close(buf);
+               buffer_auto_read(buf);
                goto wait_other_side;
        }
 
@@ -3677,10 +3675,10 @@ int http_resync_states(struct session *s)
        http_silent_debug(__LINE__, s);
        http_sync_req_state(s);
        while (1) {
-       http_silent_debug(__LINE__, s);
+               http_silent_debug(__LINE__, s);
                if (!http_sync_res_state(s))
                        break;
-       http_silent_debug(__LINE__, s);
+               http_silent_debug(__LINE__, s);
                if (!http_sync_req_state(s))
                        break;
        }
@@ -3702,22 +3700,23 @@ int http_resync_states(struct session *s)
            (txn->req.msg_state == HTTP_MSG_CLOSED &&
             txn->rsp.msg_state == HTTP_MSG_CLOSED)) {
                s->req->analysers = 0;
-               s->req->flags &= ~BF_DONT_READ;
                buffer_auto_close(s->req);
+               buffer_auto_read(s->req);
                s->rep->analysers = 0;
                buffer_auto_close(s->rep);
-               s->rep->flags &= ~BF_DONT_READ;
+               buffer_auto_read(s->rep);
        }
        else if (txn->rsp.msg_state == HTTP_MSG_CLOSED ||
                 txn->rsp.msg_state == HTTP_MSG_ERROR ||
                 (s->rep->flags & BF_SHUTW)) {
-               s->rep->flags &= ~BF_DONT_READ;
-               s->req->flags &= ~BF_DONT_READ;
+               s->rep->analysers = 0;
+               buffer_auto_close(s->rep);
+               buffer_auto_read(s->rep);
+               s->req->analysers = 0;
                buffer_abort(s->req);
                buffer_auto_close(s->req);
+               buffer_auto_read(s->req);
                buffer_ignore(s->req, s->req->l - s->req->send_max);
-               s->req->analysers = 0;
-               s->rep->analysers = 0;
        }
        else if (txn->req.msg_state == HTTP_MSG_CLOSED &&
                 txn->rsp.msg_state == HTTP_MSG_DONE &&
@@ -3756,8 +3755,9 @@ int http_request_forward_body(struct session *s, struct buffer *req, int an_bit)
            ((req->flags & BF_SHUTW) && (req->to_forward || req->send_max))) {
                /* Output closed while we were sending data. We must abort. */
                buffer_ignore(req, req->l - req->send_max);
+               buffer_auto_read(req);
+               buffer_auto_close(req);
                req->analysers &= ~an_bit;
-               req->flags &= ~BF_DONT_READ;
                return 1;
        }
 
@@ -3876,6 +3876,8 @@ int http_request_forward_body(struct session *s, struct buffer *req, int an_bit)
        /* Note: we don't send any error if some data were already sent */
        stream_int_cond_close(req->prod, (txn->rsp.msg_state < HTTP_MSG_BODY) ? error_message(s, HTTP_ERR_400) : NULL);
 
+       buffer_auto_read(req);
+       buffer_auto_close(req);
        req->analysers = 0;
        s->fe->counters.failed_req++;
        if (s->listener->counters)
@@ -4003,6 +4005,7 @@ int http_wait_for_response(struct session *s, struct buffer *rep, int an_bit)
                                health_adjust(s->srv, HANA_STATUS_HTTP_HDRRSP);
                        }
 
+                       buffer_auto_close(rep);
                        rep->analysers = 0;
                        txn->status = 502;
                        rep->prod->flags |= SI_FL_NOLINGER;
@@ -4032,6 +4035,7 @@ int http_wait_for_response(struct session *s, struct buffer *rep, int an_bit)
                                health_adjust(s->srv, HANA_STATUS_HTTP_READ_ERROR);
                        }
 
+                       buffer_auto_close(rep);
                        rep->analysers = 0;
                        txn->status = 502;
                        rep->prod->flags |= SI_FL_NOLINGER;
@@ -4055,6 +4059,7 @@ int http_wait_for_response(struct session *s, struct buffer *rep, int an_bit)
                                health_adjust(s->srv, HANA_STATUS_HTTP_READ_TIMEOUT);
                        }
 
+                       buffer_auto_close(rep);
                        rep->analysers = 0;
                        txn->status = 504;
                        rep->prod->flags |= SI_FL_NOLINGER;
@@ -4078,6 +4083,7 @@ int http_wait_for_response(struct session *s, struct buffer *rep, int an_bit)
                                health_adjust(s->srv, HANA_STATUS_HTTP_BROKEN_PIPE);
                        }
 
+                       buffer_auto_close(rep);
                        rep->analysers = 0;
                        txn->status = 502;
                        rep->prod->flags |= SI_FL_NOLINGER;
@@ -4097,6 +4103,7 @@ int http_wait_for_response(struct session *s, struct buffer *rep, int an_bit)
 
                        s->be->counters.failed_resp++;
                        rep->analysers = 0;
+                       buffer_auto_close(rep);
 
                        if (!(s->flags & SN_ERR_MASK))
                                s->flags |= SN_ERR_CLICL;
@@ -4287,6 +4294,7 @@ skip_content_length:
        /* end of job, return OK */
        rep->analysers &= ~an_bit;
        rep->analyse_exp = TICK_ETERNITY;
+       buffer_auto_close(rep);
        return 1;
 }
 
@@ -4696,8 +4704,9 @@ int http_response_forward_body(struct session *s, struct buffer *res, int an_bit
            !s->req->analysers) {
                /* in case of error or if the other analyser went away, we can't analyse HTTP anymore */
                buffer_ignore(res, res->l - res->send_max);
+               buffer_auto_read(res);
+               buffer_auto_close(res);
                res->analysers &= ~an_bit;
-               res->flags &= ~BF_DONT_READ;
                return 1;
        }
 
@@ -4816,6 +4825,8 @@ int http_response_forward_body(struct session *s, struct buffer *res, int an_bit
        txn->status = 502;
        stream_int_cond_close(res->cons, NULL);
 
+       buffer_auto_close(res);
+       buffer_auto_read(res);
        res->analysers = 0;
        s->be->counters.failed_resp++;
        if (s->srv) {

diff --git a/src/session.c b/src/session.c
index 0cad6e857be1a52799834188f59374ac3c3b63b5..406d2c0bdd22e8c93b9a8e65b5ea0df17e1b9e6b 100644 (file)
--- a/src/session.c
+++ b/src/session.c
@@ -804,7 +804,13 @@ resync_stream_interface:
                        unsigned int ana_list;
                        unsigned int ana_back;
 
-                       /* it's up to the analysers to stop new connections */
+                       /* it's up to the analysers to stop new connections,
+                        * disable reading or closing. Note: if an analyser
+                        * disables any of these bits, it is responsible for
+                        * enabling them again when it disables itself, so
+                        * that other analysers are called in similar conditions.
+                        */
+                       buffer_auto_read(s->req);
                        buffer_auto_connect(s->req);
                        buffer_auto_close(s->req);
 
@@ -950,7 +956,13 @@ resync_stream_interface:
                        unsigned int ana_list;
                        unsigned int ana_back;
 
-                       /* it's up to the analysers to reset auto_close */
+                       /* it's up to the analysers to stop disable reading or
+                        * closing. Note: if an analyser disables any of these
+                        * bits, it is responsible for enabling them again when
+                        * it disables itself, so that other analysers are called
+                        * in similar conditions.
+                        */
+                       buffer_auto_read(s->rep);
                        buffer_auto_close(s->rep);
 
                        /* We will call all analysers for which a bit is set in
@@ -1055,6 +1067,7 @@ resync_stream_interface:
                 * attached to it. If any data are left in, we'll permit them to
                 * move.
                 */
+               buffer_auto_read(s->req);
                buffer_auto_connect(s->req);
                buffer_auto_close(s->req);
                buffer_flush(s->req);
@@ -1172,6 +1185,7 @@ resync_stream_interface:
                 * attached to it. If any data are left in, we'll permit them to
                 * move.
                 */
+               buffer_auto_read(s->rep);
                buffer_auto_close(s->rep);
                buffer_flush(s->rep);
                if (!(s->rep->flags & (BF_SHUTR|BF_SHUTW|BF_SHUTW_NOW)))