httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1701005 - in /httpd/httpd/trunk: docs/manual/mod/core.xml include/http_protocol.h modules/ssl/ssl_engine_kernel.c server/core.c server/protocol.c
Date Thu, 03 Sep 2015 15:46:57 GMT
On Thu, Sep 3, 2015 at 2:45 PM,  <icing@apache.org> wrote:
> Author: icing
> Date: Thu Sep  3 12:45:26 2015
> New Revision: 1701005
>
> URL: http://svn.apache.org/r1701005
> Log:
> changed Protocols default to http/1.1 only, updated documentation, changed ap_select_protocol()
to return NULL when no protocol could be agreed upon
>
> Modified:
[]
>     httpd/httpd/trunk/server/protocol.c
[]
>
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1701005&r1=1701004&r2=1701005&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Thu Sep  3 12:45:26 2015
> @@ -1982,53 +1982,77 @@ AP_DECLARE(const char *) ap_select_proto
>                                              apr_array_header_t *choices)
>  {
>      apr_pool_t *pool = r? r->pool : c->pool;
> -    apr_array_header_t *proposals;
> -    const char *protocol = NULL, *existing = ap_get_protocol(c);
>      core_server_config *conf = ap_get_core_module_config(s->module_config);
> +    const char *protocol = NULL, *existing = ap_get_protocol(c);;
> +    apr_array_header_t *proposals;

I think we should not check for NULL "choices" below, it's up to
caller to provide a valid pointer (or we crash where we should).

Also it should probably be a const apr_array_header_t*.

>
>      if (APLOGcdebug(c)) {
>          const char *p = apr_array_pstrcat(pool, conf->protocols, ',');
>          ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
>                        "select protocol from %s, choices=%s for server %s",
> -                      p, apr_array_pstrcat(pool, choices, ','),
> +                      p, choices?
> +                      apr_array_pstrcat(pool, choices, ',') : "NULL",
>                        s->server_hostname);
>      }
>
> -    proposals = apr_array_make(pool, choices->nelts+1, sizeof(char *));
> +    proposals = apr_array_make(pool, choices? choices->nelts+1 : 5,
> +                               sizeof(char *));
>      ap_run_protocol_propose(c, r, s, choices, proposals);
>
>      if (proposals->nelts > 0) {
>          int i;
> -        apr_array_header_t *prefs = ((conf->protocols_honor_order > 0
> -                                      && conf->protocols->nelts > 0)?
> -                                     conf->protocols : choices);
> +        apr_array_header_t *prefs = NULL;
> +
> +        /* Default for protocols_honor_order is 'on' or != 0 */
> +        if (conf->protocols_honor_order == 0 && choices && choices->nelts
> 0) {
> +            prefs = choices;
> +        }
> +        else {
> +            prefs = conf->protocols;
> +        }
>
>          /* If the existing protocol has not been proposed, but is a choice,
>           * add it to the proposals implicitly.
>           */
> -        if (!ap_array_str_contains(proposals, existing)
> +        if (choices
> +            && !ap_array_str_contains(proposals, existing)
>              && ap_array_str_contains(choices, existing)) {
>              APR_ARRAY_PUSH(proposals, const char*) = existing;
>          }

Shouldn't this "if" be moved before "if (proposals->nelts > 0)" above
so that we enter here (i.e. don't return NULL) when "existing" is both
a choice and a configured Protocols?

> -
> +
>          /* Select the most preferred protocol */
>          if (APLOGcdebug(c)) {
>              ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> -                          "select protocol, proposals=%s preferences=%s",
> +                          "select protocol, proposals=%s preferences=%s configured=%s",
>                            apr_array_pstrcat(pool, proposals, ','),
> -                          apr_array_pstrcat(pool, prefs, ','));
> +                          apr_array_pstrcat(pool, prefs, ','),
> +                          apr_array_pstrcat(pool, conf->protocols, ','));
>          }
> -        for (i = 0; i < proposals->nelts; ++i) {
> -            const char *p = APR_ARRAY_IDX(proposals, i, const char *);
> -            if (conf->protocols->nelts > 0
> -                && !ap_array_str_contains(conf->protocols, p)) {
> -                /* not a permitted protocol here */
> -                continue;
> -            }
> -            else if (!protocol
> -                     || (protocol_cmp(prefs, protocol, p) < 0)) {
> -                /* none selected yet or this on has preference */
> -                protocol = p;
> +        if (conf->protocols->nelts <= 0) {
> +            /* nothing configured, by default, we only allow http/1.1 here.
> +             * For now...
> +             */
> +            return (ap_array_str_contains(proposals, AP_PROTOCOL_HTTP1)?
> +                    AP_PROTOCOL_HTTP1 : NULL);
> +        }

Couldn't we move this "if" at the beginning of the function?
Whatever the choices and proposals are, it looks immutable.

> +        else {
> +            for (i = 0; i < proposals->nelts; ++i) {
> +                const char *p = APR_ARRAY_IDX(proposals, i, const char *);
> +                if (conf->protocols->nelts <= 0 && !strcmp(AP_PROTOCOL_HTTP1,
p)) {

"conf->protocols->nelts <= 0" already tested above (always false here).

> +                    /* nothing configured, by default, we only allow http/1.1 here.
> +                     * For now...
> +                     */
> +                    continue;
> +                }
> +                if (!ap_array_str_contains(conf->protocols, p)) {
> +                    /* not a configured protocol here */
> +                    continue;
> +                }
> +                else if (!protocol
> +                         || (protocol_cmp(prefs, protocol, p) < 0)) {
> +                    /* none selected yet or this one has preference */
> +                    protocol = p;
> +                }
>              }
>          }
>      }
> @@ -2037,7 +2061,7 @@ AP_DECLARE(const char *) ap_select_proto
>                        protocol? protocol : "(none)");
>      }
>
> -    return protocol? protocol : existing;
> +    return protocol;
>  }


My comments above also attached as a patch (possibly better than words ;) ...

Regards,
Yann.

Mime
View raw message