Re: svn commit: r1885573 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h modules/ssl/ssl_engine_io.c server/core.c server/core_filters.c
On 2/2/21 2:58 PM, Yann Ylavic wrote:
> On Mon, Feb 1, 2021 at 11:24 AM Ruediger Pluem wrote:
>>> +
>>> +should_send_brigade(bb, c, );
>>> +if (flush) {
>>> +apr_int32_t nfd;
>>> +apr_pollfd_t pfd;
>>> +memset(, 0, sizeof(pfd));
>>> +pfd.reqevents = APR_POLLOUT;
>>> +pfd.desc_type = APR_POLL_SOCKET;
>>> +pfd.desc.s = sock;
>>> +pfd.p = c->pool;
>>> +do {
>>> +rv = apr_poll(, 1, , sock_timeout);
>>> +} while (APR_STATUS_IS_EINTR(rv));
>>> }
>>> -/*
>>> - * Defer the actual blocking write to avoid doing many writes.
>>> - */
>>> -flush_upto = next;
>>> +} while (flush);
>>
>> Hm, doesn't that loop forever in case the socket does not become writable
>> again?
>> We don't check the result of the above poll call whether we had an event or
>> if we hit the timeout.
>> Shouldn't we leave the outer while loop (the while(flush)) if apr_poll
>> returns APR_TIMEUP?
>> Otherwise I assume that send_brigade_nonblocking will just return
>> APR_STATUS_IS_EAGAIN.
>
> Yes correct, good catch.
>
> The attached patch aligns with what we do in trunk (which does not
> have this issue), and should fix it.
I agree that it should fix it. Another nitpick I came across when reviewing
this patch:
Shouldn't should_send_brigade return an int instead of apr_status_t?
Regards
Rüdiger
Re: svn commit: r1886141 - /httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
On Tue, Feb 2, 2021 at 8:50 PM wrote:
>
> Author: rpluem
> Date: Tue Feb 2 19:50:14 2021
> New Revision: 1886141
>
[]
> if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
> -if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> - "overlong proxy URL scheme in %s", url);
> -return HTTP_BAD_REQUEST;
> -}
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01113)
>"HTTP: declining URL %s", url);
> return DECLINED; /* only interested in HTTP, WS or FTP via proxy */
> }
> +if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> + "overlong proxy URL scheme in %s", url);
> +return HTTP_BAD_REQUEST;
> +}
Hmm, actually this !scheme here is unreachable now.
I think we can remove this test/block completely now that
get_url_scheme() checks the exact scheme.
The old code was like this (removed hunk from r1885239):
-if ((u - url) > 14)
-return HTTP_BAD_REQUEST;
-scheme = apr_pstrmemdup(p, url, u - url);
and probably meant to avoid allocating an unreasonable scheme, but
thanks to get_url_scheme() we do not allocate anymore, so this check
is probably useless now.
It's quite hard to reject overlong schemes in mod_proxy_http anyway,
because it'll really take the !scheme branch above, and since a scheme
is [[:alnum:].+-] it may well be a hostname too and match a CONNECT
URI.
So should we check for:
if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
if (!scheme && (u = strchr(url, ':')) && (u - url) > 14 &&
!apr_isdigit(u[0])) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
"overlong proxy URL scheme in %s", url);
return HTTP_BAD_REQUEST;
}
return DECLINED; /* only interested in HTTP, WS or FTP via proxy */
}
or something?
Regards;
Yann.
Re: svn commit: r1885239 - in /httpd/httpd/trunk: changes-entries/proxy_wstunnel_to_http.txt docs/log-message-tags/next-number modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/mo
On 2/2/21 3:05 PM, Yann Ylavic wrote: > On Tue, Feb 2, 2021 at 11:32 AM Ruediger Pluem wrote: >> > > Will you commit the above patch (or should I)? Done as r1886141. > Then I could propose it for backport with the other needed > core_output_filter() change. +1 > > Thanks for the review (again and again :). Welcome Regards Rüdiger
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
On Tue, Feb 2, 2021 at 10:32 AM Ruediger Pluem wrote:
>
> > New Revision: 1885605
> >
> > URL: http://svn.apache.org/viewvc?rev=1885605=rev
[]
> > +/* Yield if the output filters stack is full? This is to avoid
> > + * blocking and give the caller a chance to POLLOUT async.
> > + */
> > +if (flags & AP_PROXY_TRANSFER_YIELD_PENDING) {
> > +int rc = OK;
> > +
> > +if (!ap_filter_should_yield(c_o->output_filters)) {
> > +rc = ap_filter_output_pending(c_o);
>
> I am confused here: !ap_filter_should_yield(c_o->output_filters) means there
> is no pending output.
> Why should we try to send it then? Shouldn't it be the other way round?
Yes, !ap_filter_should_yield() means that there is no pending output,
but only for filters that play the
ap_filter_{setaside,reinstate}_brigade() game.
The goal here is to try to flush pending data of filters that don't
ap_filter_setaside_brigade(), like e.g. ssl_io_filter_coalesce() which
is not using setaside/reinstate (but is kind of aware of them still,
see r1879416, I tried to move it to using setaside/resinstate but the
result is more complicated than the original code, so I passed on that
patch for now and pushed the one-liner..).
But (and this is why the code is like this), we don't want to try to
flush those potential non-setaside pending data if there are setaside
pending data already (like in the core output filter), otherwise we
might block in some filter (like the core filter still) if given more
data while already at the limits.
This poll()ing loop really depends on staying in the POLLOUT state
until there are no more pending data (setaside or not), otherwise we
"risk" the blocking call soon or later, at least with the current "no
EAGAIN" on output mechanism. Hope this clarifies why it's done like
this..
Regards;
Yann.
Re: svn commit: r1885239 - in /httpd/httpd/trunk: changes-entries/proxy_wstunnel_to_http.txt docs/log-message-tags/next-number modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/mo
On Tue, Feb 2, 2021 at 11:32 AM Ruediger Pluem wrote:
>
> On 1/7/21 2:19 PM, yla...@apache.org wrote:
> > Author: ylavic
> > Date: Thu Jan 7 13:19:08 2021
> > New Revision: 1885239
[]
> >
> > -/* find the scheme */
> > -u = strchr(url, ':');
> > -if (u == NULL || u[1] != '/' || u[2] != '/' || u[3] == '\0')
> > +scheme = get_url_scheme(, _ssl);
> > +if (!scheme && proxyname && strncasecmp(url, "ftp:", 4) == 0) {
> > +u = url + 4;
> > +scheme = "ftp";
> > +is_ssl = 0;
> > +}
> > +if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
> > +if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> > +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> > + "overlong proxy URL scheme in %s", url);
> > +return HTTP_BAD_REQUEST;
> > +}
>
> This breaks forward proxies with the CONNECT method.
> For CONNECT somwhere123456789.com:443 schema is NULL and u[0] is 's' and
> hence != /.
Yes, sorry about that.
>
> The following patches fixes this:
>
> Index: mod_proxy_http.c
> ===
> --- mod_proxy_http.c(revision 1886120)
> +++ mod_proxy_http.c(working copy)
> @@ -1903,15 +1903,15 @@
> is_ssl = 0;
> }
> if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
> -if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> - "overlong proxy URL scheme in %s", url);
> -return HTTP_BAD_REQUEST;
> -}
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01113)
>"HTTP: declining URL %s", url);
> return DECLINED; /* only interested in HTTP, WS or FTP via proxy */
> }
> +if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> + "overlong proxy URL scheme in %s", url);
> +return HTTP_BAD_REQUEST;
> +}
> if (is_ssl && !ap_proxy_ssl_enable(NULL)) {
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01112)
>"HTTP: declining URL %s (mod_ssl not configured?)",
> url);
+1
>
> Unfortunately this has been already backported in r1885605 and hence 2.4.x is
> now broken as well.
Will you commit the above patch (or should I)?
Then I could propose it for backport with the other needed
core_output_filter() change.
Thanks for the review (again and again :).
Regards;
Yann.
Re: svn commit: r1885573 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h modules/ssl/ssl_engine_io.c server/core.c server/core_filters.c
On Mon, Feb 1, 2021 at 11:24 AM Ruediger Pluem wrote:
> > +
> > +should_send_brigade(bb, c, );
> > +if (flush) {
> > +apr_int32_t nfd;
> > +apr_pollfd_t pfd;
> > +memset(, 0, sizeof(pfd));
> > +pfd.reqevents = APR_POLLOUT;
> > +pfd.desc_type = APR_POLL_SOCKET;
> > +pfd.desc.s = sock;
> > +pfd.p = c->pool;
> > +do {
> > +rv = apr_poll(, 1, , sock_timeout);
> > +} while (APR_STATUS_IS_EINTR(rv));
> > }
> > -/*
> > - * Defer the actual blocking write to avoid doing many writes.
> > - */
> > -flush_upto = next;
> > +} while (flush);
>
> Hm, doesn't that loop forever in case the socket does not become writable
> again?
> We don't check the result of the above poll call whether we had an event or
> if we hit the timeout.
> Shouldn't we leave the outer while loop (the while(flush)) if apr_poll
> returns APR_TIMEUP?
> Otherwise I assume that send_brigade_nonblocking will just return
> APR_STATUS_IS_EAGAIN.
Yes correct, good catch.
The attached patch aligns with what we do in trunk (which does not
have this issue), and should fix it.
I should have done it like this from the start rather than diverging
2.4.x more than necessary, since should_send_brigade() is meant to
align with trunk's ap_filter_reinstate_brigade(), with the notable
difference of THRESHOLD_MIN_WRITE (removed from trunk somehow).
With the attached patch, we'd have the below diff on this code
(whitespace changes ignored/mangled):
$ diff -u -w ~trunk/server/core_filters.c ~2.4.x/server/core_filters.c
...
+if (!new_bb || should_send_brigade(bb, c, NULL)) {
+apr_socket_t *sock = net->client_socket;
+apr_interval_time_t sock_timeout = 0;
+
/* Non-blocking writes on the socket in any case. */
apr_socket_timeout_get(sock, _timeout);
apr_socket_timeout_set(sock, 0);
do {
rv = send_brigade_nonblocking(sock, bb, ctx, c);
-if (APR_STATUS_IS_EAGAIN(rv)) {
+if (new_bb && APR_STATUS_IS_EAGAIN(rv)) {
/* Scan through the brigade and decide whether we must absolutely
- * flush the remaining data, based on ap_filter_reinstate_brigade()
+ * flush the remaining data, based on should_send_brigade()
* rules. If so, wait for writability and retry, otherwise we did
* our best already and can wait for the next call.
*/
-apr_bucket *flush_upto;
-ap_filter_reinstate_brigade(f, bb, _upto);
-if (flush_upto) {
+int flush;
+should_send_brigade(bb, c, );
+if (flush) {
apr_int32_t nfd;
apr_pollfd_t pfd;
memset(, 0, sizeof(pfd));
@@ -422,6 +534,7 @@
/* Restore original socket timeout before leaving. */
apr_socket_timeout_set(sock, sock_timeout);
+}
...
>
> Furthermore doesn't that open a way for a Dos if the client only reads a
> single byte shortly
> before the timeout is hit? But I think we had the same problem with the old
> code and we would need
> to have a mod_reqtimeout like check for a minimum rate and probably a maximum
> timeout.
Another story.. reqtimeout can hardly work as an output filter here
since the core_output_filter is the very last one.
Possibly at AP_FTYPE_NETWORK - 1 and with some
ap_filter_should_yield() alike probes we can cook something, but some
optional function(s) called directly by the core may be easier.
Anyway, we miss the whole reqtimeout "outgoing" configuration for now..
Thanks for the review Rüdiger, better late than shipped :)
Regards;
Yann.
Index: server/core_filters.c
===
--- server/core_filters.c (revision 1886100)
+++ server/core_filters.c (working copy)
@@ -502,7 +502,6 @@ apr_status_t ap_core_output_filter(ap_filter_t *f,
if (!new_bb || should_send_brigade(bb, c, NULL)) {
apr_socket_t *sock = net->client_socket;
apr_interval_time_t sock_timeout = 0;
-int flush;
/* Non-blocking writes on the socket in any case. */
apr_socket_timeout_get(sock, _timeout);
@@ -510,25 +509,29 @@ apr_status_t ap_core_output_filter(ap_filter_t *f,
do {
rv = send_brigade_nonblocking(sock, bb, ctx, c);
-if (!new_bb || !APR_STATUS_IS_EAGAIN(rv)) {
-break;
+if (new_bb && APR_STATUS_IS_EAGAIN(rv)) {
+/* Scan through the brigade and decide whether we must absolutely
+ * flush the remaining data, based on should_send_brigade()
+ * rules. If so, wait for writability and retry, otherwise we did
+ * our best already and can wait for the next call.
+ */
+
Re: svn commit: r1885239 - in /httpd/httpd/trunk: changes-entries/proxy_wstunnel_to_http.txt docs/log-message-tags/next-number modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/mo
On 2/2/21 11:32 AM, Ruediger Pluem wrote:
>
>
> On 1/7/21 2:19 PM, yla...@apache.org wrote:
>> Author: ylavic
>> Date: Thu Jan 7 13:19:08 2021
>> New Revision: 1885239
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1885239=1885238=1885239=diff
>> ==
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Thu Jan 7 13:19:08 2021
>> @@ -1840,9 +1877,8 @@ static int proxy_http_handler(request_re
>>apr_port_t proxyport)
>> {
>> int status;
>> -char *scheme;
>> -const char *proxy_function;
>> -const char *u;
>> +const char *scheme;
>> +const char *u = url;
>> proxy_http_req_t *req = NULL;
>> proxy_conn_rec *backend = NULL;
>> apr_bucket_brigade *input_brigade = NULL;
>> @@ -1860,41 +1896,31 @@ static int proxy_http_handler(request_re
>> apr_pool_t *p = r->pool;
>> apr_uri_t *uri;
>>
>> -/* find the scheme */
>> -u = strchr(url, ':');
>> -if (u == NULL || u[1] != '/' || u[2] != '/' || u[3] == '\0')
>> +scheme = get_url_scheme(, _ssl);
>> +if (!scheme && proxyname && strncasecmp(url, "ftp:", 4) == 0) {
>> +u = url + 4;
>> +scheme = "ftp";
>> +is_ssl = 0;
>> +}
>> +if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
>> +if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
>> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
>> + "overlong proxy URL scheme in %s", url);
>> +return HTTP_BAD_REQUEST;
>> +}
>
> This breaks forward proxies with the CONNECT method.
> For CONNECT somwhere123456789.com:443 schema is NULL and u[0] is 's' and
> hence != /.
>
> The following patches fixes this:
>
> Index: mod_proxy_http.c
> ===
> --- mod_proxy_http.c (revision 1886120)
> +++ mod_proxy_http.c (working copy)
> @@ -1903,15 +1903,15 @@
> is_ssl = 0;
> }
> if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
> -if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> - "overlong proxy URL scheme in %s", url);
> -return HTTP_BAD_REQUEST;
> -}
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01113)
>"HTTP: declining URL %s", url);
> return DECLINED; /* only interested in HTTP, WS or FTP via proxy */
> }
> +if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> + "overlong proxy URL scheme in %s", url);
> +return HTTP_BAD_REQUEST;
> +}
> if (is_ssl && !ap_proxy_ssl_enable(NULL)) {
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01112)
>"HTTP: declining URL %s (mod_ssl not configured?)",
> url);
>
> Unfortunately this has been already backported in r1885605 and hence 2.4.x is
> now broken as well.
>
And it looks like that the test suite has no forward proxy tests at all which
caused this to be missed by the test framework.
Regards
Rüdiger
Re: svn commit: r1885239 - in /httpd/httpd/trunk: changes-entries/proxy_wstunnel_to_http.txt docs/log-message-tags/next-number modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/mo
On 1/7/21 2:19 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Thu Jan 7 13:19:08 2021
> New Revision: 1885239
>
> URL: http://svn.apache.org/viewvc?rev=1885239=rev
> Log:
> mod_proxy_wstunnel: leave Upgrade requests handling to mod_proxy_http.
>
> Let mod_proxy_http's canon and scheme handlers accept "ws[s]:" schemes so that
> mod_proxy_wstunnel can decline requests when mod_proxy_http is loaded.
>
> * modules/proxy/{mod_proxy.h,proxy_util.c} (ap_proxy_worker_can_upgrade):
> Add a "dflt" argument to ap_proxy_worker_can_upgrade() which, if not NULL,
> is matched when no worker upgrade= parameter is configured. This allows to
> handle the default "Upgrade: websocket" case for "ws[s]:" schemes.
>
> * modules/proxy/mod_proxy_http.c (proxy_http_canon, proxy_http_handler):
> Add and use the new get_url_scheme() helper to parse URL schemes handled by
> mod_proxy_http and use it in canon and scheme handlers. This helper now
> accepts ws[s] schemes.
>
> * modules/proxy/mod_proxy_wstunnel.c (proxy_wstunnel_post_config):
> New post_config hook to detect whether mod_proxy_http is loaded and set
> global fallback_to_mod_proxy_http flag in this case.
>
> * modules/proxy/mod_proxy_wstunnel.c (proxy_wstunnel_check_trans,
> proxy_wstunnel_canon,
> proxy_wstunnel_handler):
> These hooks now early return DECLINED if fallback_to_mod_proxy_http is set.
>
> Added:
> httpd/httpd/trunk/changes-entries/proxy_wstunnel_to_http.txt
> Modified:
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/modules/proxy/mod_proxy.h
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Added: httpd/httpd/trunk/changes-entries/proxy_wstunnel_to_http.txt
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/proxy_wstunnel_to_http.txt?rev=1885239=auto
> ==
> --- httpd/httpd/trunk/changes-entries/proxy_wstunnel_to_http.txt (added)
> +++ httpd/httpd/trunk/changes-entries/proxy_wstunnel_to_http.txt Thu Jan 7
> 13:19:08 2021
> @@ -0,0 +1,4 @@
> + *) mod_proxy_wstunnel: Leave Upgrade requests handling to mod_proxy_http,
> + allowing for (non-)Upgrade negotiation with the origin server.
> + [Yann Ylavic]
> +
>
> Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1885239=1885238=1885239=diff
> ==
> --- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
> +++ httpd/httpd/trunk/docs/log-message-tags/next-number Thu Jan 7 13:19:08
> 2021
> @@ -1 +1 @@
> -10262
> +10263
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1885239=1885238=1885239=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Thu Jan 7 13:19:08 2021
> @@ -755,11 +755,13 @@ PROXY_DECLARE(char *) ap_proxy_worker_na
> * @param p memory pool used for displaying worker name
> * @param worker the worker
> * @param upgrade the Upgrade header to match
> + * @param dfltdefault protocol (NULL for none)
> * @return1 (true) or 0 (false)
> */
> PROXY_DECLARE(int) ap_proxy_worker_can_upgrade(apr_pool_t *p,
> const proxy_worker *worker,
> - const char *upgrade);
> + const char *upgrade,
> + const char *dflt);
>
> /**
> * Get the worker from proxy configuration
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1885239=1885238=1885239=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Thu Jan 7 13:19:08 2021
> @@ -28,37 +28,71 @@ static int (*ap_proxy_clear_connection_f
> static apr_status_t ap_proxygetline(apr_bucket_brigade *bb, char *s, int n,
> request_rec *r, int flags, int *read);
>
> +static const char *get_url_scheme(const char **url, int *is_ssl)
> +{
> +const char *u = *url;
> +
> +switch (u[0]) {
> +case 'h':
> +case 'H':
> +if (strncasecmp(u + 1, "ttp", 3) == 0) {
> +if (u[4] == ':') {
> +*is_ssl = 0;
> +*url = u + 5;
> +
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
On 1/17/21 5:21 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Sun Jan 17 16:21:35 2021
> New Revision: 1885605
>
> URL: http://svn.apache.org/viewvc?rev=1885605=rev
> Log:
> Backport to v2.4:
>
> *) mod_proxy_http: handle upgrade/tunneling protocols. BZ 61616 is about
> mod_proxy_connect but there has been wstunnel reports
> on dev@ about that too lately.
> trunk patch: https://svn.apache.org/r1678771
> https://svn.apache.org/r1832348
> https://svn.apache.org/r1869338
> https://svn.apache.org/r1869420
> https://svn.apache.org/r1878367
> https://svn.apache.org/r1877557
> https://svn.apache.org/r1877558
> https://svn.apache.org/r1877646
> https://svn.apache.org/r1877695
> https://svn.apache.org/r1879401
> https://svn.apache.org/r1879402
> https://svn.apache.org/r1880200
> https://svn.apache.org/r1885239
> https://svn.apache.org/r1885240
> https://svn.apache.org/r1885244
> 2.4.x patch:
> http://people.apache.org/~ylavic/patches/2.4.x-mod_proxy_http-upgrade-4on5-v2.patch
> https://github.com/apache/httpd/pull/158
> +1: ylavic, covener, minfrin
> ylavic: All the corresponding trunk changes to mod_proxy_wstunnel (but
> r1885239) have been dropped for this backport proposal, the goal
> being to handle upgrade in mod_proxy_http from now, while
> r1885239
> allows to benefit from the Upgrade improvements done in
> proxy_http
> with existing wstunnel configurations (provided mod_proxy_http
> module is loaded).
>
>
> Modified:
> httpd/httpd/branches/2.4.x/CHANGES
> httpd/httpd/branches/2.4.x/STATUS
> httpd/httpd/branches/2.4.x/include/ap_mmn.h
> httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
> httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.h
> httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_connect.c
> httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_http.c
> httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_wstunnel.c
> httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1885605=1885604=1885605=diff
> ==
> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Sun Jan 17 16:21:35
> 2021
i,
> @@ -4119,81 +4168,498 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tra
> const char *name,
> int *sent,
> apr_off_t bsize,
> - int after)
> + int flags)
> {
> apr_status_t rv;
> +int flush_each = 0;
> +unsigned int num_reads = 0;
> #ifdef DEBUGGING
> apr_off_t len;
> #endif
>
> -do {
> +/*
> + * Compat: since FLUSH_EACH is default (and zero) for legacy reasons, we
> + * pretend it's no FLUSH_AFTER nor YIELD_PENDING flags, the latter
> because
> + * flushing would defeat the purpose of checking for pending data (hence
> + * determine whether or not the output chain/stack is full for stopping).
> + */
> +if (!(flags & (AP_PROXY_TRANSFER_FLUSH_AFTER |
> + AP_PROXY_TRANSFER_YIELD_PENDING))) {
> +flush_each = 1;
> +}
> +
> +for (;;) {
> apr_brigade_cleanup(bb_i);
> rv = ap_get_brigade(c_i->input_filters, bb_i, AP_MODE_READBYTES,
> APR_NONBLOCK_READ, bsize);
> -if (rv == APR_SUCCESS) {
> -if (c_o->aborted) {
> -return APR_EPIPE;
> -}
> -if (APR_BRIGADE_EMPTY(bb_i)) {
> -break;
> +if (rv != APR_SUCCESS) {
> +if (!APR_STATUS_IS_EAGAIN(rv) && !APR_STATUS_IS_EOF(rv)) {
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r, APLOGNO(03308)
> + "ap_proxy_transfer_between_connections: "
> + "error on %s - ap_get_brigade",
> + name);
> +if (rv == APR_INCOMPLETE) {
> +/* Don't return APR_INCOMPLETE, it'd mean "should yield"
> + * for the caller, while it means "incomplete body" here
> + * from ap_http_filter(), which is an error.
> + */
> +rv = APR_EGENERAL;
> +
