Re: [2.2 PATCH] fix HttpProtocolOptions (etc) merging
On Mon, Feb 20, 2017 at 10:16 AM, Joe Orton wrote: > > FYI, since I've seen people thinking seriously about test suites, this > kind of issue is impossible to test comprehensively with the current > test framework. We really need to spin up httpd multiple times with > different configurations, to test different combinations of global to > vhost merges with default vs non-default options. > > (If there is some way to do that with the current test suite, I'd be > interested to hear anyway.) The headache is that testing anything more complex than loopback implies you kick off a remote with -server and then pound on that remote it from the client/framework. Perhaps if we spun up parallel instances in the same port-ordered manner, but those instances were truly distinct configs? (These need distinct var/ directories/pid files etc, of course.)
Re: [2.2 PATCH] fix HttpProtocolOptions (etc) merging
On Fri, Feb 17, 2017 at 12:38:30PM -0500, Eric Covener wrote: > +1 > > On Fri, Feb 17, 2017 at 12:37 PM, William A Rowe Jr > wrote: > > Great catch; +1 to commit to 2.2.x and > > http://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x-merge-http-strict/ > > branches. > > > > And thanks for adding the breadcrumb for the next sucker to miss this :-O Thanks for the reviews. 2.2.x => http://svn.apache.org/viewvc?rev=1783440&view=rev 2.2.x-merge-strict => http://svn.apache.org/viewvc?rev=1783441&view=rev FYI, since I've seen people thinking seriously about test suites, this kind of issue is impossible to test comprehensively with the current test framework. We really need to spin up httpd multiple times with different configurations, to test different combinations of global to vhost merges with default vs non-default options. (If there is some way to do that with the current test suite, I'd be interested to hear anyway.) Regads, Joe
Re: [2.2 PATCH] fix HttpProtocolOptions (etc) merging
+1 On Fri, Feb 17, 2017 at 12:37 PM, William A Rowe Jr wrote: > Great catch; +1 to commit to 2.2.x and > http://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x-merge-http-strict/ > branches. > > And thanks for adding the breadcrumb for the next sucker to miss this :-O > > On Fri, Feb 17, 2017 at 3:30 AM, Joe Orton wrote: >> Found during QA of the CVE-2016-8743 patch here. The merging logic in >> merge_core_server_configs is (confusingly) inverted from 2.2 to 2.4, so >> e.g. HttpProtocolOptions doesn't inherit from global to vhost configs in >> 2.2.32. :( >> >> Index: server/core.c >> === >> --- server/core.c (revision 1783354) >> +++ server/core.c (working copy) >> @@ -546,15 +546,19 @@ >> ? virt->merge_trailers >> : base->merge_trailers; >> >> -if (virt->http09_enable != AP_HTTP09_UNSET) >> -conf->http09_enable = virt->http09_enable; >> +if (conf->http09_enable == AP_HTTP09_UNSET) >> +conf->http09_enable = base->http09_enable; >> >> -if (virt->http_conformance != AP_HTTP_CONFORMANCE_UNSET) >> -conf->http_conformance = virt->http_conformance; >> +if (conf->http_conformance == AP_HTTP_CONFORMANCE_UNSET) >> +conf->http_conformance = base->http_conformance; >> >> -if (virt->http_methods != AP_HTTP_METHODS_UNSET) >> -conf->http_methods = virt->http_methods; >> +if (conf->http_methods == AP_HTTP_METHODS_UNSET) >> +conf->http_methods = base->http_methods; >> >> +/* N.B. If you backport things here from 2.4, note that the >> + * merging logic needs to be inverted, since conf is initially a >> + * copy of vertv not basev. */ >> + >> return conf; >> } >> >> -- Eric Covener cove...@gmail.com
Re: [2.2 PATCH] fix HttpProtocolOptions (etc) merging
Great catch; +1 to commit to 2.2.x and http://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x-merge-http-strict/ branches. And thanks for adding the breadcrumb for the next sucker to miss this :-O On Fri, Feb 17, 2017 at 3:30 AM, Joe Orton wrote: > Found during QA of the CVE-2016-8743 patch here. The merging logic in > merge_core_server_configs is (confusingly) inverted from 2.2 to 2.4, so > e.g. HttpProtocolOptions doesn't inherit from global to vhost configs in > 2.2.32. :( > > Index: server/core.c > === > --- server/core.c (revision 1783354) > +++ server/core.c (working copy) > @@ -546,15 +546,19 @@ > ? virt->merge_trailers > : base->merge_trailers; > > -if (virt->http09_enable != AP_HTTP09_UNSET) > -conf->http09_enable = virt->http09_enable; > +if (conf->http09_enable == AP_HTTP09_UNSET) > +conf->http09_enable = base->http09_enable; > > -if (virt->http_conformance != AP_HTTP_CONFORMANCE_UNSET) > -conf->http_conformance = virt->http_conformance; > +if (conf->http_conformance == AP_HTTP_CONFORMANCE_UNSET) > +conf->http_conformance = base->http_conformance; > > -if (virt->http_methods != AP_HTTP_METHODS_UNSET) > -conf->http_methods = virt->http_methods; > +if (conf->http_methods == AP_HTTP_METHODS_UNSET) > +conf->http_methods = base->http_methods; > > +/* N.B. If you backport things here from 2.4, note that the > + * merging logic needs to be inverted, since conf is initially a > + * copy of vertv not basev. */ > + > return conf; > } > >
[2.2 PATCH] fix HttpProtocolOptions (etc) merging
Found during QA of the CVE-2016-8743 patch here. The merging logic in merge_core_server_configs is (confusingly) inverted from 2.2 to 2.4, so e.g. HttpProtocolOptions doesn't inherit from global to vhost configs in 2.2.32. :( Index: server/core.c === --- server/core.c (revision 1783354) +++ server/core.c (working copy) @@ -546,15 +546,19 @@ ? virt->merge_trailers : base->merge_trailers; -if (virt->http09_enable != AP_HTTP09_UNSET) -conf->http09_enable = virt->http09_enable; +if (conf->http09_enable == AP_HTTP09_UNSET) +conf->http09_enable = base->http09_enable; -if (virt->http_conformance != AP_HTTP_CONFORMANCE_UNSET) -conf->http_conformance = virt->http_conformance; +if (conf->http_conformance == AP_HTTP_CONFORMANCE_UNSET) +conf->http_conformance = base->http_conformance; -if (virt->http_methods != AP_HTTP_METHODS_UNSET) -conf->http_methods = virt->http_methods; +if (conf->http_methods == AP_HTTP_METHODS_UNSET) +conf->http_methods = base->http_methods; +/* N.B. If you backport things here from 2.4, note that the + * merging logic needs to be inverted, since conf is initially a + * copy of vertv not basev. */ + return conf; }
