On Tue, Nov 24, 2015 at 5:28 AM, William A Rowe Jr <[email protected]> wrote:
>
> I didn't have a chance to review the patch, but all of the instances that 1.
> map to an RFC protocol string comparison and 2. occur in 2.4 code base
> *should* be coalesced into one patch. I'm +1 for every such change. The
> rest of the RFC token comparisons can be added individually for later
> merging with their associated bug or enhancement backports.
The goal of the changes I committed (and reverted) so far is to
address the cases where we really expect ASCII comparison (ie. don't
want locale to make us think the compared string matches whereas it is
not, eg. "Connection: clôse").
This mainly concerns header names, header values (known tokens), schemes, ...
Hopefully I reverted the other/abusive changes already.
There remain these changes to mod_proxy and mod_rewrite (revert patch
attached, not applied, yet?), which I think makes sense for
performance reasons, but are not strictly required since the compared
strings come from the configuration file (though mod_proxy params may
be configured via the balancer manager).
I whish I could let them applied because of the (huge/costly)
if-else-if-else-if-... pattern, and we do compare real (ours) tokens
here...
Index: modules/mappers/mod_rewrite.c
===================================================================
--- modules/mappers/mod_rewrite.c (revision 1716159)
+++ modules/mappers/mod_rewrite.c (working copy)
@@ -3477,13 +3477,13 @@ static const char *cmd_rewriterule_setflag(apr_poo
switch (*key++) {
case 'b':
case 'B':
- if (!*key || !ap_casecmpstr(key, "ackrefescaping")) {
+ if (!*key || !strcasecmp(key, "ackrefescaping")) {
cfg->flags |= RULEFLAG_ESCAPEBACKREF;
if (val && *val) {
cfg->escapes = val;
}
}
- else if (!ap_casecmpstr(key, "NP") || !ap_casecmpstr(key, "ackrefernoplus")) {
+ else if (!strcasecmp(key, "NP") || !strcasecmp(key, "ackrefernoplus")) {
cfg->flags |= RULEFLAG_ESCAPENOPLUS;
}
else {
@@ -3492,11 +3492,11 @@ static const char *cmd_rewriterule_setflag(apr_poo
break;
case 'c':
case 'C':
- if (!*key || !ap_casecmpstr(key, "hain")) { /* chain */
+ if (!*key || !strcasecmp(key, "hain")) { /* chain */
cfg->flags |= RULEFLAG_CHAIN;
}
else if (((*key == 'O' || *key == 'o') && !key[1])
- || !ap_casecmpstr(key, "ookie")) { /* cookie */
+ || !strcasecmp(key, "ookie")) { /* cookie */
data_item *cp = cfg->cookie;
if (!cp) {
@@ -3519,13 +3519,13 @@ static const char *cmd_rewriterule_setflag(apr_poo
break;
case 'd':
case 'D':
- if (!*key || !ap_casecmpstr(key, "PI") || !ap_casecmpstr(key,"iscardpath")) {
+ if (!*key || !strcasecmp(key, "PI") || !strcasecmp(key,"iscardpath")) {
cfg->flags |= (RULEFLAG_DISCARDPATHINFO);
}
break;
case 'e':
case 'E':
- if (!*key || !ap_casecmpstr(key, "nv")) { /* env */
+ if (!*key || !strcasecmp(key, "nv")) { /* env */
data_item *cp = cfg->env;
if (!cp) {
@@ -3542,7 +3542,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
cp->next = NULL;
cp->data = val;
}
- else if (!ap_casecmpstr(key, "nd")) { /* end */
+ else if (!strcasecmp(key, "nd")) { /* end */
cfg->flags |= RULEFLAG_END;
}
else {
@@ -3552,7 +3552,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 'f':
case 'F':
- if (!*key || !ap_casecmpstr(key, "orbidden")) { /* forbidden */
+ if (!*key || !strcasecmp(key, "orbidden")) { /* forbidden */
cfg->flags |= (RULEFLAG_STATUS | RULEFLAG_NOSUB);
cfg->forced_responsecode = HTTP_FORBIDDEN;
}
@@ -3563,7 +3563,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 'g':
case 'G':
- if (!*key || !ap_casecmpstr(key, "one")) { /* gone */
+ if (!*key || !strcasecmp(key, "one")) { /* gone */
cfg->flags |= (RULEFLAG_STATUS | RULEFLAG_NOSUB);
cfg->forced_responsecode = HTTP_GONE;
}
@@ -3574,7 +3574,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 'h':
case 'H':
- if (!*key || !ap_casecmpstr(key, "andler")) { /* handler */
+ if (!*key || !strcasecmp(key, "andler")) { /* handler */
cfg->forced_handler = val;
}
else {
@@ -3583,7 +3583,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
break;
case 'l':
case 'L':
- if (!*key || !ap_casecmpstr(key, "ast")) { /* last */
+ if (!*key || !strcasecmp(key, "ast")) { /* last */
cfg->flags |= RULEFLAG_LASTRULE;
}
else {
@@ -3594,10 +3594,10 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 'n':
case 'N':
if (((*key == 'E' || *key == 'e') && !key[1])
- || !ap_casecmpstr(key, "oescape")) { /* noescape */
+ || !strcasecmp(key, "oescape")) { /* noescape */
cfg->flags |= RULEFLAG_NOESCAPE;
}
- else if (!*key || !ap_casecmpstr(key, "ext")) { /* next */
+ else if (!*key || !strcasecmp(key, "ext")) { /* next */
cfg->flags |= RULEFLAG_NEWROUND;
if (val && *val) {
cfg->maxrounds = atoi(val);
@@ -3605,11 +3605,11 @@ static const char *cmd_rewriterule_setflag(apr_poo
}
else if (((*key == 'S' || *key == 's') && !key[1])
- || !ap_casecmpstr(key, "osubreq")) { /* nosubreq */
+ || !strcasecmp(key, "osubreq")) { /* nosubreq */
cfg->flags |= RULEFLAG_IGNOREONSUBREQ;
}
else if (((*key == 'C' || *key == 'c') && !key[1])
- || !ap_casecmpstr(key, "ocase")) { /* nocase */
+ || !strcasecmp(key, "ocase")) { /* nocase */
cfg->flags |= RULEFLAG_NOCASE;
}
else {
@@ -3619,11 +3619,11 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 'p':
case 'P':
- if (!*key || !ap_casecmpstr(key, "roxy")) { /* proxy */
+ if (!*key || !strcasecmp(key, "roxy")) { /* proxy */
cfg->flags |= RULEFLAG_PROXY;
}
else if (((*key == 'T' || *key == 't') && !key[1])
- || !ap_casecmpstr(key, "assthrough")) { /* passthrough */
+ || !strcasecmp(key, "assthrough")) { /* passthrough */
cfg->flags |= RULEFLAG_PASSTHROUGH;
}
else {
@@ -3633,11 +3633,11 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 'q':
case 'Q':
- if ( !ap_casecmpstr(key, "SA")
- || !ap_casecmpstr(key, "sappend")) { /* qsappend */
+ if ( !strcasecmp(key, "SA")
+ || !strcasecmp(key, "sappend")) { /* qsappend */
cfg->flags |= RULEFLAG_QSAPPEND;
- } else if ( !ap_casecmpstr(key, "SD")
- || !ap_casecmpstr(key, "sdiscard") ) { /* qsdiscard */
+ } else if ( !strcasecmp(key, "SD")
+ || !strcasecmp(key, "sdiscard") ) { /* qsdiscard */
cfg->flags |= RULEFLAG_QSDISCARD;
}
else {
@@ -3647,18 +3647,18 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 'r':
case 'R':
- if (!*key || !ap_casecmpstr(key, "edirect")) { /* redirect */
+ if (!*key || !strcasecmp(key, "edirect")) { /* redirect */
int status = 0;
cfg->flags |= RULEFLAG_FORCEREDIRECT;
if (*val) {
- if (ap_casecmpstr(val, "permanent") == 0) {
+ if (strcasecmp(val, "permanent") == 0) {
status = HTTP_MOVED_PERMANENTLY;
}
- else if (ap_casecmpstr(val, "temp") == 0) {
+ else if (strcasecmp(val, "temp") == 0) {
status = HTTP_MOVED_TEMPORARILY;
}
- else if (ap_casecmpstr(val, "seeother") == 0) {
+ else if (strcasecmp(val, "seeother") == 0) {
status = HTTP_SEE_OTHER;
}
else if (apr_isdigit(*val)) {
@@ -3688,7 +3688,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 's':
case 'S':
- if (!*key || !ap_casecmpstr(key, "kip")) { /* skip */
+ if (!*key || !strcasecmp(key, "kip")) { /* skip */
cfg->skip = atoi(val);
}
else {
@@ -3698,7 +3698,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
case 't':
case 'T':
- if (!*key || !ap_casecmpstr(key, "ype")) { /* type */
+ if (!*key || !strcasecmp(key, "ype")) { /* type */
cfg->forced_mimetype = val;
}
else {
Index: modules/proxy/mod_proxy.c
===================================================================
--- modules/proxy/mod_proxy.c (revision 1716159)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -64,7 +64,7 @@ static const char *set_worker_param(apr_pool_t *p,
int ival;
apr_interval_time_t timeout;
- if (!ap_casecmpstr(key, "loadfactor")) {
+ if (!strcasecmp(key, "loadfactor")) {
/* Normalized load factor. Used with BalancerMamber,
* it is a number between 1 and 100.
*/
@@ -72,7 +72,7 @@ static const char *set_worker_param(apr_pool_t *p,
if (worker->s->lbfactor < 1 || worker->s->lbfactor > 100)
return "LoadFactor must be a number between 1..100";
}
- else if (!ap_casecmpstr(key, "retry")) {
+ else if (!strcasecmp(key, "retry")) {
/* If set it will give the retry timeout for the worker
* The default value is 60 seconds, meaning that if
* in error state, it will be retried after that timeout.
@@ -83,7 +83,7 @@ static const char *set_worker_param(apr_pool_t *p,
worker->s->retry = apr_time_from_sec(ival);
worker->s->retry_set = 1;
}
- else if (!ap_casecmpstr(key, "ttl")) {
+ else if (!strcasecmp(key, "ttl")) {
/* Time in seconds that will destroy all the connections
* that exceed the smax
*/
@@ -92,7 +92,7 @@ static const char *set_worker_param(apr_pool_t *p,
return "TTL must be at least one second";
worker->s->ttl = apr_time_from_sec(ival);
}
- else if (!ap_casecmpstr(key, "min")) {
+ else if (!strcasecmp(key, "min")) {
/* Initial number of connections to remote
*/
ival = atoi(val);
@@ -100,7 +100,7 @@ static const char *set_worker_param(apr_pool_t *p,
return "Min must be a positive number";
worker->s->min = ival;
}
- else if (!ap_casecmpstr(key, "max")) {
+ else if (!strcasecmp(key, "max")) {
/* Maximum number of connections to remote
*/
ival = atoi(val);
@@ -109,7 +109,7 @@ static const char *set_worker_param(apr_pool_t *p,
worker->s->hmax = ival;
}
/* XXX: More inteligent naming needed */
- else if (!ap_casecmpstr(key, "smax")) {
+ else if (!strcasecmp(key, "smax")) {
/* Maximum number of connections to remote that
* will not be destroyed
*/
@@ -118,7 +118,7 @@ static const char *set_worker_param(apr_pool_t *p,
return "Smax must be a positive number";
worker->s->smax = ival;
}
- else if (!ap_casecmpstr(key, "acquire")) {
+ else if (!strcasecmp(key, "acquire")) {
/* Acquire timeout in given unit (default is milliseconds).
* If set this will be the maximum time to
* wait for a free connection.
@@ -130,7 +130,7 @@ static const char *set_worker_param(apr_pool_t *p,
worker->s->acquire = timeout;
worker->s->acquire_set = 1;
}
- else if (!ap_casecmpstr(key, "timeout")) {
+ else if (!strcasecmp(key, "timeout")) {
/* Connection timeout in seconds.
* Defaults to server timeout.
*/
@@ -140,7 +140,7 @@ static const char *set_worker_param(apr_pool_t *p,
worker->s->timeout = apr_time_from_sec(ival);
worker->s->timeout_set = 1;
}
- else if (!ap_casecmpstr(key, "iobuffersize")) {
+ else if (!strcasecmp(key, "iobuffersize")) {
long s = atol(val);
if (s < 512 && s) {
return "IOBufferSize must be >= 512 bytes, or 0 for system default.";
@@ -148,7 +148,7 @@ static const char *set_worker_param(apr_pool_t *p,
worker->s->io_buffer_size = (s ? s : AP_IOBUFSIZE);
worker->s->io_buffer_size_set = 1;
}
- else if (!ap_casecmpstr(key, "receivebuffersize")) {
+ else if (!strcasecmp(key, "receivebuffersize")) {
ival = atoi(val);
if (ival < 512 && ival != 0) {
return "ReceiveBufferSize must be >= 512 bytes, or 0 for system default.";
@@ -156,34 +156,34 @@ static const char *set_worker_param(apr_pool_t *p,
worker->s->recv_buffer_size = ival;
worker->s->recv_buffer_size_set = 1;
}
- else if (!ap_casecmpstr(key, "keepalive")) {
- if (!ap_casecmpstr(val, "on"))
+ else if (!strcasecmp(key, "keepalive")) {
+ if (!strcasecmp(val, "on"))
worker->s->keepalive = 1;
- else if (!ap_casecmpstr(val, "off"))
+ else if (!strcasecmp(val, "off"))
worker->s->keepalive = 0;
else
return "KeepAlive must be On|Off";
worker->s->keepalive_set = 1;
}
- else if (!ap_casecmpstr(key, "disablereuse")) {
- if (!ap_casecmpstr(val, "on"))
+ else if (!strcasecmp(key, "disablereuse")) {
+ if (!strcasecmp(val, "on"))
worker->s->disablereuse = 1;
- else if (!ap_casecmpstr(val, "off"))
+ else if (!strcasecmp(val, "off"))
worker->s->disablereuse = 0;
else
return "DisableReuse must be On|Off";
worker->s->disablereuse_set = 1;
}
- else if (!ap_casecmpstr(key, "enablereuse")) {
- if (!ap_casecmpstr(val, "on"))
+ else if (!strcasecmp(key, "enablereuse")) {
+ if (!strcasecmp(val, "on"))
worker->s->disablereuse = 0;
- else if (!ap_casecmpstr(val, "off"))
+ else if (!strcasecmp(val, "off"))
worker->s->disablereuse = 1;
else
return "EnableReuse must be On|Off";
worker->s->disablereuse_set = 1;
}
- else if (!ap_casecmpstr(key, "route")) {
+ else if (!strcasecmp(key, "route")) {
/* Worker route.
*/
if (strlen(val) >= sizeof(worker->s->route))
@@ -191,7 +191,7 @@ static const char *set_worker_param(apr_pool_t *p,
(int)sizeof(worker->s->route));
PROXY_STRNCPY(worker->s->route, val);
}
- else if (!ap_casecmpstr(key, "redirect")) {
+ else if (!strcasecmp(key, "redirect")) {
/* Worker redirection route.
*/
if (strlen(val) >= sizeof(worker->s->redirect))
@@ -199,7 +199,7 @@ static const char *set_worker_param(apr_pool_t *p,
(int)sizeof(worker->s->redirect));
PROXY_STRNCPY(worker->s->redirect, val);
}
- else if (!ap_casecmpstr(key, "status")) {
+ else if (!strcasecmp(key, "status")) {
const char *v;
int mode = 1;
apr_status_t rv;
@@ -219,17 +219,17 @@ static const char *set_worker_param(apr_pool_t *p,
return "Unknown status parameter option";
}
}
- else if (!ap_casecmpstr(key, "flushpackets")) {
- if (!ap_casecmpstr(val, "on"))
+ else if (!strcasecmp(key, "flushpackets")) {
+ if (!strcasecmp(val, "on"))
worker->s->flush_packets = flush_on;
- else if (!ap_casecmpstr(val, "off"))
+ else if (!strcasecmp(val, "off"))
worker->s->flush_packets = flush_off;
- else if (!ap_casecmpstr(val, "auto"))
+ else if (!strcasecmp(val, "auto"))
worker->s->flush_packets = flush_auto;
else
return "flushpackets must be on|off|auto";
}
- else if (!ap_casecmpstr(key, "flushwait")) {
+ else if (!strcasecmp(key, "flushwait")) {
ival = atoi(val);
if (ival > 1000 || ival < 0) {
return "flushwait must be <= 1000, or 0 for system default of 10 millseconds.";
@@ -239,7 +239,7 @@ static const char *set_worker_param(apr_pool_t *p,
else
worker->s->flush_wait = ival * 1000; /* change to microseconds */
}
- else if (!ap_casecmpstr(key, "ping")) {
+ else if (!strcasecmp(key, "ping")) {
/* Ping/Pong timeout in given unit (default is second).
*/
if (ap_timeout_parameter_parse(val, &timeout, "s") != APR_SUCCESS)
@@ -249,13 +249,13 @@ static const char *set_worker_param(apr_pool_t *p,
worker->s->ping_timeout = timeout;
worker->s->ping_timeout_set = 1;
}
- else if (!ap_casecmpstr(key, "lbset")) {
+ else if (!strcasecmp(key, "lbset")) {
ival = atoi(val);
if (ival < 0 || ival > 99)
return "lbset must be between 0 and 99";
worker->s->lbset = ival;
}
- else if (!ap_casecmpstr(key, "connectiontimeout")) {
+ else if (!strcasecmp(key, "connectiontimeout")) {
/* Request timeout in given unit (default is second).
* Defaults to connection timeout
*/
@@ -266,7 +266,7 @@ static const char *set_worker_param(apr_pool_t *p,
worker->s->conn_timeout = timeout;
worker->s->conn_timeout_set = 1;
}
- else if (!ap_casecmpstr(key, "flusher")) {
+ else if (!strcasecmp(key, "flusher")) {
if (strlen(val) >= sizeof(worker->s->flusher))
apr_psprintf(p, "flusher name length must be < %d characters",
(int)sizeof(worker->s->flusher));
@@ -286,7 +286,7 @@ static const char *set_balancer_param(proxy_server
{
int ival;
- if (!ap_casecmpstr(key, "stickysession")) {
+ if (!strcasecmp(key, "stickysession")) {
char *path;
/* Balancer sticky session name.
* Set to something like JSESSIONID or
@@ -303,12 +303,12 @@ static const char *set_balancer_param(proxy_server
PROXY_STRNCPY(balancer->s->sticky_path, path);
}
}
- else if (!ap_casecmpstr(key, "stickysessionsep")) {
+ else if (!strcasecmp(key, "stickysessionsep")) {
/* separator/delimiter for sessionid and route,
* normally '.'
*/
if (strlen(val) != 1) {
- if (!ap_casecmpstr(val, "off"))
+ if (!strcasecmp(val, "off"))
balancer->s->sticky_separator = 0;
else
return "stickysessionsep must be a single character or Off";
@@ -317,20 +317,20 @@ static const char *set_balancer_param(proxy_server
balancer->s->sticky_separator = *val;
balancer->s->sticky_separator_set = 1;
}
- else if (!ap_casecmpstr(key, "nofailover")) {
+ else if (!strcasecmp(key, "nofailover")) {
/* If set to 'on' the session will break
* if the worker is in error state or
* disabled.
*/
- if (!ap_casecmpstr(val, "on"))
+ if (!strcasecmp(val, "on"))
balancer->s->sticky_force = 1;
- else if (!ap_casecmpstr(val, "off"))
+ else if (!strcasecmp(val, "off"))
balancer->s->sticky_force = 0;
else
return "failover must be On|Off";
balancer->s->sticky_force_set = 1;
}
- else if (!ap_casecmpstr(key, "timeout")) {
+ else if (!strcasecmp(key, "timeout")) {
/* Balancer timeout in seconds.
* If set this will be the maximum time to
* wait for a free worker.
@@ -341,7 +341,7 @@ static const char *set_balancer_param(proxy_server
return "timeout must be at least one second";
balancer->s->timeout = apr_time_from_sec(ival);
}
- else if (!ap_casecmpstr(key, "maxattempts")) {
+ else if (!strcasecmp(key, "maxattempts")) {
/* Maximum number of failover attempts before
* giving up.
*/
@@ -351,7 +351,7 @@ static const char *set_balancer_param(proxy_server
balancer->s->max_attempts = ival;
balancer->s->max_attempts_set = 1;
}
- else if (!ap_casecmpstr(key, "lbmethod")) {
+ else if (!strcasecmp(key, "lbmethod")) {
proxy_balancer_method *provider;
if (strlen(val) > (sizeof(balancer->s->lbpname)-1))
return "unknown lbmethod";
@@ -368,20 +368,20 @@ static const char *set_balancer_param(proxy_server
}
return "unknown lbmethod";
}
- else if (!ap_casecmpstr(key, "scolonpathdelim")) {
+ else if (!strcasecmp(key, "scolonpathdelim")) {
/* If set to 'on' then ';' will also be
* used as a session path separator/delim (ala
* mod_jk)
*/
- if (!ap_casecmpstr(val, "on"))
+ if (!strcasecmp(val, "on"))
balancer->s->scolonsep = 1;
- else if (!ap_casecmpstr(val, "off"))
+ else if (!strcasecmp(val, "off"))
balancer->s->scolonsep = 0;
else
return "scolonpathdelim must be On|Off";
balancer->s->scolonsep_set = 1;
}
- else if (!ap_casecmpstr(key, "failonstatus")) {
+ else if (!strcasecmp(key, "failonstatus")) {
char *val_split;
char *status;
char *tok_state;
@@ -403,17 +403,17 @@ static const char *set_balancer_param(proxy_server
}
}
- else if (!ap_casecmpstr(key, "failontimeout")) {
- if (!ap_casecmpstr(val, "on"))
+ else if (!strcasecmp(key, "failontimeout")) {
+ if (!strcasecmp(val, "on"))
balancer->failontimeout = 1;
- else if (!ap_casecmpstr(val, "off"))
+ else if (!strcasecmp(val, "off"))
balancer->failontimeout = 0;
else
return "failontimeout must be On|Off";
balancer->failontimeout_set = 1;
}
- else if (!ap_casecmpstr(key, "nonce")) {
- if (!ap_casecmpstr(val, "None")) {
+ else if (!strcasecmp(key, "nonce")) {
+ if (!strcasecmp(val, "None")) {
*balancer->s->nonce = '\0';
}
else {
@@ -423,7 +423,7 @@ static const char *set_balancer_param(proxy_server
}
balancer->s->nonce_set = 1;
}
- else if (!ap_casecmpstr(key, "growth")) {
+ else if (!strcasecmp(key, "growth")) {
ival = atoi(val);
if (ival < 1 || ival > 100) /* arbitrary limit here */
return "growth must be between 1 and 100";
@@ -430,10 +430,10 @@ static const char *set_balancer_param(proxy_server
balancer->growth = ival;
balancer->growth_set = 1;
}
- else if (!ap_casecmpstr(key, "forcerecovery")) {
- if (!ap_casecmpstr(val, "on"))
+ else if (!strcasecmp(key, "forcerecovery")) {
+ if (!strcasecmp(val, "on"))
balancer->s->forcerecovery = 1;
- else if (!ap_casecmpstr(val, "off"))
+ else if (!strcasecmp(val, "off"))
balancer->s->forcerecovery = 0;
else
return "forcerecovery must be On|Off";
