On Wed, May 7, 2014 at 11:47 AM, Joe Orton <[email protected]> wrote:
> I hacked the attached up yesterday, before seeing Yann's mail, untested.
The patch looks good (against 2.4.x), but as I said to Edward, I think
r->status should be restored (at least, "error-notes" probably too)
when r->proxyreq == PROXYREQ_RESPONSE.
Indeed, if the response is read in one go (headers + body + trailers),
and read_chunked_trailers() fails, r->status will be modified to
something different from the orginal one (while the body will be
forwarded as is).
Otherwise if the headers are forwarded before the trailers, the change
on r->status won't reach the client but may still disrupt the logging.
>
> Yann, in your smaller patch is it deliberate that you only handle one of
> the two places trailers are read in ap_http_filter()? Either that is an
> oversight or I am missing something.
My oversight, I was influenced by the trunk (longer) patch where there
is only one place (with the BODY_CHUNK_TRAILER state).
>
> I'm also daunted by the size of change required to a rewrite core
> function like ap_rgetline* to be non-blocking, at least in 2.[24]; that
> seems really like an orthogonal issue.
The patch looks huge but actually this is mostly due to the
accompanying reindentation.
Below is the (latest) patch for ap_rgetline_core() =>
ap_rgetline_ex(), with space changes ignored.
I don't find it so daunting, but I agree it is orthogonal since it
concerns non-blocking, ie. mpm_event, with which I am not sure the
trailers are read in non-blocking mode by any handler (although
ap_http_filter() can be called in non-blocking mode)...
+1 to leave it aside from 2.4.x for now, but I think we will probably
need a non-blocking ap_get_mime_headers_ex() (hence ap_rgetline_ex)
soon.
Regards,
Yann.
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1594967)
+++ server/protocol.c (working copy)
@@ -193,13 +193,15 @@
* stricter protocol adherence and better input filter behavior during
* chunked trailer processing (for http).
*
- * If s is NULL, ap_rgetline_core will allocate necessary memory from r->pool.
+ * If s is NULL, ap_rgetline_ex will allocate necessary memory from r->pool.
*
* Returns APR_SUCCESS if there are no problems and sets *read to be
* the full length of s.
*
* APR_ENOSPC is returned if there is not enough buffer space.
- * Other errors may be returned on other errors.
+ * APR_EAGAIN is returned if a non-blocking read would have blocked, possibly
+ * with a partial line available in *s (and a *read > 0).
+ * Other downstream errors may be returned.
*
* The LF is *not* returned in the buffer. Therefore, a *read of 0
* indicates that an empty line was read.
@@ -210,9 +212,10 @@
* If no LF is detected on the last line due to a dropped connection
* or a full buffer, that's considered an error.
*/
-AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
+AP_DECLARE(apr_status_t) ap_rgetline_ex(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
- int fold, apr_bucket_brigade *bb)
+ int fold, apr_bucket_brigade *bb,
+ apr_read_type_e block, ap_filter_t *f)
{
apr_status_t rv;
apr_bucket *e;
@@ -220,6 +223,10 @@
char *pos, *last_char = *s;
int do_alloc = (*s == NULL), saw_eos = 0;
+ if (f == NULL) {
+ f = r->proto_input_filters;
+ }
+
/*
* Initialize last_char as otherwise a random value will be compared
* against APR_ASCII_LF at the end of the loop if bb only contains
@@ -230,8 +237,28 @@
for (;;) {
apr_brigade_cleanup(bb);
- rv = ap_get_brigade(r->proto_input_filters, bb, AP_MODE_GETLINE,
- APR_BLOCK_READ, 0);
+ rv = ap_get_brigade(f, bb, AP_MODE_GETLINE, block, 0);
+ if (block == APR_NONBLOCK_READ
+ && (APR_STATUS_IS_EAGAIN(rv)
+ || (rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb)))) {
+ *read = bytes_handled;
+ if (*s) {
+ /* ensure this string is NUL terminated */
+ if (n < bytes_handled + 1) {
+ if (bytes_handled > 0) {
+ (*s)[bytes_handled-1] = '\0';
+ }
+ else {
+ (*s)[0] = '\0';
+ }
+ return APR_ENOSPC;
+ }
+ (*s)[bytes_handled] = '\0';
+ }
+ if (rv == APR_SUCCESS) {
+ rv = APR_EAGAIN;
+ }
+ }
if (rv != APR_SUCCESS) {
return rv;
}
@@ -287,7 +314,7 @@
/* We'll assume the common case where one bucket is enough. */
if (!*s) {
current_alloc = len;
- *s = apr_palloc(r->pool, current_alloc);
+ *s = apr_palloc(r->pool, current_alloc + 1);
}
else if (bytes_handled + len > current_alloc) {
/* Increase the buffer size */
@@ -298,7 +325,7 @@
new_size = (bytes_handled + len) * 2;
}
- new_buffer = apr_palloc(r->pool, new_size);
+ new_buffer = apr_palloc(r->pool, new_size + 1);
/* Copy what we already had. */
memcpy(new_buffer, *s, bytes_handled);
@@ -327,8 +354,9 @@
if (last_char > *s && last_char[-1] == APR_ASCII_CR) {
last_char--;
}
+ bytes_handled = last_char - *s;
+ *read = bytes_handled;
*last_char = '\0';
- bytes_handled = last_char - *s;
/* If we're folding, we have more work to do.
*
@@ -344,8 +372,11 @@
apr_brigade_cleanup(bb);
/* We only care about the first byte. */
- rv = ap_get_brigade(r->proto_input_filters, bb,
AP_MODE_SPECULATIVE,
- APR_BLOCK_READ, 1);
+ rv = ap_get_brigade(f, bb, AP_MODE_SPECULATIVE, block, 1);
+ if (rv == APR_SUCCESS && block == APR_NONBLOCK_READ &&
+ APR_BRIGADE_EMPTY(bb)) {
+ rv = APR_EAGAIN;
+ }
if (rv != APR_SUCCESS) {
return rv;
}
@@ -401,38 +432,43 @@
}
next_size = n - bytes_handled;
-
- rv = ap_rgetline_core(&tmp, next_size,
- &next_len, r, 0, bb);
- if (rv != APR_SUCCESS) {
- return rv;
- }
-
- if (do_alloc && next_len > 0) {
+
+ rv = ap_rgetline_ex(&tmp, next_size, &next_len,
+ r, 0, bb, block, f);
+ if ((rv == APR_SUCCESS
+ || (block == APR_NONBLOCK_READ
+ && APR_STATUS_IS_EAGAIN(rv)))
+ && (next_len > 0)) {
+ if (do_alloc) {
char *new_buffer;
- apr_size_t new_size = bytes_handled + next_len + 1;
- /* we need to alloc an extra byte for a null */
- new_buffer = apr_palloc(r->pool, new_size);
+ /* we need to alloc an extra byte for a nul */
+ new_buffer = apr_palloc(r->pool,
+ bytes_handled +
+ next_len + 1);
/* Copy what we already had. */
memcpy(new_buffer, *s, bytes_handled);
+ *s = new_buffer;
- /* copy the new line, including the trailing null */
- memcpy(new_buffer + bytes_handled, tmp, next_len + 1);
- *s = new_buffer;
+ /* copy the new line, including trailing nul */
+ last_char = *s + bytes_handled;
+ memcpy(last_char, tmp, next_len + 1);
}
-
last_char += next_len;
bytes_handled += next_len;
+ *read = bytes_handled;
}
+ if (rv != APR_SUCCESS) {
+ return rv;
}
+ }
+ }
else { /* next character is not tab or space */
break;
}
}
}
- *read = bytes_handled;
/* PR#43039: We shouldn't accept NULL bytes within the line */
if (strlen(*s) < bytes_handled) {
@@ -442,6 +478,13 @@
return APR_SUCCESS;
}
+AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
+ apr_size_t *read, request_rec *r,
+ int fold, apr_bucket_brigade *bb)
+{
+ return ap_rgetline_ex(s, n, read, r, fold, bb, APR_BLOCK_READ, NULL);
+}
+
#if APR_CHARSET_EBCDIC
AP_DECLARE(apr_status_t) ap_rgetline(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
