httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: mod_proxy, oooled backend connections and the keep-alive race condition
Date Wed, 02 Oct 2013 22:38:50 GMT
A late (little) fix below...


On Thu, Oct 3, 2013 at 12:14 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c    (revision 1528615)
> +++ modules/proxy/proxy_util.c    (working copy)
> @@ -3092,7 +3092,9 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
>          buf = apr_pstrcat(p, r->method, " ", url, " HTTP/1.1" CRLF, NULL);
>      }
>      if (apr_table_get(r->subprocess_env, "proxy-nokeepalive")) {
> -        origin->keepalive = AP_CONN_CLOSE;
> +        if (origin) {
> +            origin->keepalive = AP_CONN_CLOSE;
> +        }
>          p_conn->close = 1;
>      }
>      ap_xlate_proto_to_ascii(buf, strlen(buf));
> Index: modules/proxy/mod_proxy_http.c
> ===================================================================
> --- modules/proxy/mod_proxy_http.c    (revision 1528615)
> +++ modules/proxy/mod_proxy_http.c    (working copy)
> @@ -677,30 +677,41 @@ static apr_status_t proxy_buckets_lifetime_transfo
>      return rv;
>  }
>
> +enum rb_methods {
> +    RB_INIT,
> +    RB_STREAM_CL,
> +    RB_STREAM_CHUNKED,
> +    RB_SPOOL_CL
> +};
> +
>  static
> -int ap_proxy_http_request(apr_pool_t *p, request_rec *r,
> +int ap_proxy_http_prefetch(apr_pool_t *p, request_rec *r,
>                                     proxy_conn_rec *p_conn, proxy_worker
> *worker,
>                                     proxy_server_conf *conf,
>                                     apr_uri_t *uri,
> -                                   char *url, char *server_portstr)
> +                                   char *url, char *server_portstr,
> +                                   apr_bucket_brigade *header_brigade,
> +                                   apr_bucket_brigade *input_brigade,
> +                                   char **old_cl_val, char **old_te_val,
> +                                   enum rb_methods *rb_method, int
> *keepalive)
>  {
>      conn_rec *c = r->connection;
>      apr_bucket_alloc_t *bucket_alloc = c->bucket_alloc;
> -    apr_bucket_brigade *header_brigade;
> -    apr_bucket_brigade *input_brigade;
>      apr_bucket_brigade *temp_brigade;
>      apr_bucket *e;
>      char *buf;
>      apr_status_t status;
> -    enum rb_methods {RB_INIT, RB_STREAM_CL, RB_STREAM_CHUNKED,
> RB_SPOOL_CL};
> -    enum rb_methods rb_method = RB_INIT;
> -    char *old_cl_val = NULL;
> -    char *old_te_val = NULL;
>      apr_off_t bytes_read = 0;
>
>      apr_off_t bytes;
>      int force10, rv;
> -    conn_rec *origin = p_conn->connection;
>
> +    /* XXX: This somehow should be fixed in the API.
> +     * Assumes p_conn->close is 0 here since ap_proxy_create_hdrbrgd and
> code
> +     * below will use it to disable keep-alive and not for the connection
> to
> +     * be closed before reuse.
> +     */
> +    AP_DEBUG_ASSERT(!p_conn->close);
>
> +
>      if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
>          if (r->expecting_100) {
>              return HTTP_EXPECTATION_FAILED;
> @@ -710,17 +721,13 @@ static
>          force10 = 0;
>      }
>
> -    header_brigade = apr_brigade_create(p, origin->bucket_alloc);
>
>      rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
>                                   worker, conf, uri, url, server_portstr,
> -                                 &old_cl_val, &old_te_val);
> +                                 old_cl_val, old_te_val);
>
>      if (rv != OK) {
>          return rv;
>      }
>
> -    /* We have headers, let's figure out our request body... */
> -    input_brigade = apr_brigade_create(p, bucket_alloc);
> -
>      /* sub-requests never use keepalives, and mustn't pass request bodies.
>       * Because the new logic looks at input_brigade, we will
> self-terminate
>       * input_brigade and jump past all of the request body logic...
> @@ -733,15 +740,15 @@ static
>      if (!r->kept_body && r->main) {
>          /* XXX: Why DON'T sub-requests use keepalives? */
>          p_conn->close = 1;
> -        if (old_cl_val) {
> -            old_cl_val = NULL;
> +        if (*old_cl_val) {
> +            *old_cl_val = NULL;
>              apr_table_unset(r->headers_in, "Content-Length");
>          }
> -        if (old_te_val) {
> -            old_te_val = NULL;
> +        if (*old_te_val) {
> +            *old_te_val = NULL;
>              apr_table_unset(r->headers_in, "Transfer-Encoding");
>          }
> -        rb_method = RB_STREAM_CL;
> +        *rb_method = RB_STREAM_CL;
>          e = apr_bucket_eos_create(input_brigade->bucket_alloc);
>          APR_BRIGADE_INSERT_TAIL(input_brigade, e);
>          goto skip_body;
> @@ -755,20 +762,19 @@ static
>       * encoding has been done by the extensions' handler, and
>       * do not modify add_te_chunked's logic
>       */
> -    if (old_te_val && strcasecmp(old_te_val, "chunked") != 0) {
> +    if (*old_te_val && strcasecmp(*old_te_val, "chunked") != 0) {
>          ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01093)
>                        "%s Transfer-Encoding is not supported",
> old_te_val);
>          return HTTP_INTERNAL_SERVER_ERROR;
>      }
>
> -    if (old_cl_val && old_te_val) {
> +    if (*old_cl_val && *old_te_val) {
>          ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01094)
>                        "client %s (%s) requested Transfer-Encoding "
>                        "chunked body with Content-Length (C-L ignored)",
>                        c->client_ip, c->remote_host ? c->remote_host: "");
>          apr_table_unset(r->headers_in, "Content-Length");
> -        old_cl_val = NULL;
> -        origin->keepalive = AP_CONN_CLOSE;
> +        *old_cl_val = NULL;
>          p_conn->close = 1;
>      }
>
> @@ -867,42 +873,42 @@ static
>           * If we expected no body, and read no body, do not set
>           * the Content-Length.
>           */
> -        if (old_cl_val || old_te_val || bytes_read) {
> -            old_cl_val = apr_off_t_toa(r->pool, bytes_read);
> +        if (*old_cl_val || *old_te_val || bytes_read) {
> +            *old_cl_val = apr_off_t_toa(r->pool, bytes_read);
>          }
> -        rb_method = RB_STREAM_CL;
> +        *rb_method = RB_STREAM_CL;
>      }
> -    else if (old_te_val) {
> +    else if (*old_te_val) {
>
>          if (force10
>               || (apr_table_get(r->subprocess_env, "proxy-sendcl")
>                    && !apr_table_get(r->subprocess_env, "proxy-sendchunks")
>                    && !apr_table_get(r->subprocess_env,
> "proxy-sendchunked"))) {
> -            rb_method = RB_SPOOL_CL;
> +            *rb_method = RB_SPOOL_CL;
>          }
>          else {
> -            rb_method = RB_STREAM_CHUNKED;
> +            *rb_method = RB_STREAM_CHUNKED;
>          }
>      }
> -    else if (old_cl_val) {
> +    else if (*old_cl_val) {
>          if (r->input_filters == r->proto_input_filters) {
> -            rb_method = RB_STREAM_CL;
> +            *rb_method = RB_STREAM_CL;
>
>          }
>          else if (!force10
>                    && (apr_table_get(r->subprocess_env, "proxy-sendchunks")
>                        || apr_table_get(r->subprocess_env,
> "proxy-sendchunked"))
>
>                    && !apr_table_get(r->subprocess_env, "proxy-sendcl")) {
> -            rb_method = RB_STREAM_CHUNKED;
> +            *rb_method = RB_STREAM_CHUNKED;
>          }
>          else {
> -            rb_method = RB_SPOOL_CL;
> +            *rb_method = RB_SPOOL_CL;
>          }
>      }
>      else {
>          /* This is an appropriate default; very efficient for no-body
>           * requests, and has the behavior that it will not add any C-L
> -         * when the old_cl_val is NULL.
> +         * when the *old_cl_val is NULL.
>           */
> -        rb_method = RB_SPOOL_CL;
> +        *rb_method = RB_SPOOL_CL;
>      }
>
>  /* Yes I hate gotos.  This is the subrequest shortcut */
> @@ -924,6 +930,28 @@ skip_body:
>          APR_BRIGADE_INSERT_TAIL(header_brigade, e);
>      }
>
> +    /* XXX: This somehow should be fixed in the API (see beginning
> comment).
> +     * backend->close is set to disable keepalive, no need to close the
> socket
> +     * until further notice, let the caller know about that though.
> +     */
> +    *keepalive = !p_conn->close;
> +    p_conn->close = 0;
> +
> +    return OK;
> +}
> +
> +static
> +int ap_proxy_http_request(apr_pool_t *p, request_rec *r,
> +                                   proxy_conn_rec *p_conn,
> +                                   apr_bucket_brigade *header_brigade,
> +                                   apr_bucket_brigade *input_brigade,
> +                                   char *old_cl_val, char *old_te_val,
> +                                   enum rb_methods rb_method)
> +{
> +    int rv;
> +    apr_off_t bytes_read = 0;
> +    conn_rec *origin = p_conn->connection;
> +
>      /* send the request body, if any. */
>      switch(rb_method) {
>      case RB_STREAM_CHUNKED:
> @@ -935,6 +963,7 @@ skip_body:
>                                     input_brigade, old_cl_val);
>          break;
>      case RB_SPOOL_CL:
> +        apr_brigade_length(input_brigade, 1, &bytes_read);
>          rv = spool_reqbody_cl(p, r, p_conn, origin, header_brigade,
>                                    input_brigade, (old_cl_val != NULL)
>                                                || (old_te_val != NULL)
> @@ -947,6 +976,7 @@ skip_body:
>      }
>
>      if (rv != OK) {
> +        conn_rec *c = r->connection;
>          /* apr_status_t value has been logged in lower level method */
>          ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01097)
>                        "pass request body failed to %pI (%s) from %s (%s)",
> @@ -1854,10 +1884,16 @@ static int proxy_http_handler(request_rec *r, prox
>      char *scheme;
>      const char *proxy_function;
>      const char *u;
> +    apr_bucket_brigade *header_brigade, *header_bb;
> +    apr_bucket_brigade *input_brigade, *input_bb;
>      proxy_conn_rec *backend = NULL;
>      int is_ssl = 0;
>      conn_rec *c = r->connection;
>      int retry = 0;
> +    char *old_cl_val = NULL, *old_te_val = NULL;
> +    enum rb_methods rb_method = RB_INIT;
> +    char *locurl = url;
> +    int keepalive = 1;
>      /*
>       * Use a shorter-lived pool to reduce memory usage
>       * and avoid a memory leak
> @@ -1924,16 +1960,47 @@ static int proxy_http_handler(request_rec *r, prox
>          backend->close = 1;
>      }
>
> +    /* Step One: Determine Who To Connect To */
> +    if ((status = ap_proxy_determine_connection(p, r, conf, worker,
> backend,
> +                                            uri, &locurl, proxyname,
> +                                            proxyport, server_portstr,
> +                                            sizeof(server_portstr))) !=
> OK)
> +        goto cleanup;
> +
> +
> +    /* Step Once: Prefetch (partially) the request body so to increase the
> +     * chances to get whole (or enough) body and determine Content-Length
> vs
> +     * chunked or spool. By doing this before connecting or reusing a
> backend
> +     * connection, minimize the delay between checking whether this
> connection
> +     * is still alive and the first packet sent, should the client be
> slow or
> +     * some input filter retain the data.
> +     */
> +    input_brigade = apr_brigade_create(p, c->bucket_alloc);
> +    header_brigade = apr_brigade_create(p, c->bucket_alloc);
> +    if ((status = ap_proxy_http_prefetch(p, r, backend, worker, conf, uri,
> +                    locurl, server_portstr, header_brigade, input_brigade,
> +                    &old_cl_val, &old_te_val, &rb_method, &keepalive))
!=
> OK) {
> +        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
> +                      "HTTP: failed to prefetch the request body: %s",
> +                      backend->hostname);
> +        goto cleanup;
> +    }
> +
>      while (retry < 2) {
> -        char *locurl = url;
> +        if (retry) {
> +            char *newurl = url;
> +            /* Step One-Retry: Redetermine Who To Connect To */
> +            if ((status = ap_proxy_determine_connection(p, r, conf,
> worker,
> +                            backend, uri, &newurl, proxyname, proxyport,
> +                            server_portstr, sizeof(server_portstr))) !=
> OK)
> +                break;
> +            /* XXX: the code assumes locurl is not changed during the
> loops,
> +             * or ap_proxy_http_prefetch would have to be called every
> time,
> +             * and header_brigade changed accordingly...
> +             */
> +            AP_DEBUG_ASSERT(!strcmp(newurl, locurl));
> +        }
>
> -        /* Step One: Determine Who To Connect To */
> -        if ((status = ap_proxy_determine_connection(p, r, conf, worker,
> backend,
> -                                                uri, &locurl, proxyname,
> -                                                proxyport, server_portstr,
> -                                                sizeof(server_portstr)))
> != OK)
> -            break;
> -
>          /* Step Two: Make the Connection */
>          if (ap_proxy_connect_backend(proxy_function, backend, worker,
> r->server)) {
>              ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01114)
> @@ -1974,6 +2041,9 @@ static int proxy_http_handler(request_rec *r, prox
>                                ssl_hostname);
>              }
>          }
>

Move this block :


> +        if (!keepalive) {
> +            backend->connection->keepalive = AP_CONN_CLOSE;
>
including this addon to ensure closure when not kept alive :

+            backend->close = 1;

> +        }
>
after
"Step Three-and-a-Half" below.


>
>          /* Step Three-and-a-Half: See if the socket is still connected
> (if desired) */
>          if (worker->s->ping_timeout_set && worker->s->ping_timeout
< 0 &&
> @@ -1986,12 +2056,18 @@ static int proxy_http_handler(request_rec *r, prox
>              continue;
>          }
>
> +        /* Preserve the header/input brigades since they may be retried */
> +        input_bb = apr_brigade_create(p,
> backend->connection->bucket_alloc);
> +        header_bb = apr_brigade_create(p,
> backend->connection->bucket_alloc);
> +        proxy_buckets_lifetime_transform(r, input_brigade, input_bb);
> +        proxy_buckets_lifetime_transform(r, header_brigade, header_bb);
> +
>          /* Step Four: Send the Request
>           * On the off-chance that we forced a 100-Continue as a
>           * kinda HTTP ping test, allow for retries
>           */
> -        if ((status = ap_proxy_http_request(p, r, backend, worker,
> -                                        conf, uri, locurl,
> server_portstr)) != OK) {
> +        if ((status = ap_proxy_http_request(p, r, backend, header_bb,
> input_bb,
> +                        old_cl_val, old_te_val, rb_method)) != OK) {
>              if ((status == HTTP_SERVICE_UNAVAILABLE) &&
> worker->s->ping_timeout_set &&
>                   worker->s->ping_timeout > 0) {
>                  backend->close = 1;
> [EOS]
>
>

Mime
View raw message