You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
167 lines
8.4 KiB
167 lines
8.4 KiB
From 5bebcd06287be9024f0fba25f350393f02e050c1 Mon Sep 17 00:00:00 2001
|
|
From: Willy Tarreau <w@1wt.eu>
|
|
Date: Thu, 10 Jul 2014 19:06:10 +0200
|
|
Subject: [PATCH 24/25] BUG/MAJOR: http: correctly rewind the request body
|
|
after start of forwarding
|
|
|
|
Daniel Dubovik reported an interesting bug showing that the request body
|
|
processing was still not 100% fixed. If a POST request contained short
|
|
enough data to be forwarded at once before trying to establish the
|
|
connection to the server, we had no way to correctly rewind the body.
|
|
|
|
The first visible case is that balancing on a header does not always work
|
|
on such POST requests since the header cannot be found. But there are even
|
|
nastier implications which are that http-send-name-header would apply to
|
|
the wrong location and possibly even affect part of the request's body
|
|
due to an incorrect rewinding.
|
|
|
|
There are two options to fix the problem :
|
|
- first one is to force the HTTP_MSG_F_WAIT_CONN flag on all hash-based
|
|
balancing algorithms and http-send-name-header, but there's always a
|
|
risk that any new algorithm forgets to set it ;
|
|
|
|
- the second option is to account for the amount of skipped data before
|
|
the connection establishes so that we always know the position of the
|
|
request's body relative to the buffer's origin.
|
|
|
|
The second option is much more reliable and fits very well in the spirit
|
|
of the past changes to fix forwarding. Indeed, at the moment we have
|
|
msg->sov which points to the start of the body before headers are forwarded
|
|
and which equals zero afterwards (so it still points to the start of the
|
|
body before forwarding data). A minor change consists in always making it
|
|
point to the start of the body even after data have been forwarded. It means
|
|
that it can get a negative value (so we need to change its type to signed)..
|
|
|
|
In order to avoid wrapping, we only do this as long as the other side of
|
|
the buffer is not connected yet.
|
|
|
|
Doing this definitely fixes the issues above for the requests. Since the
|
|
response cannot be rewound we don't need to perform any change there.
|
|
|
|
This bug was introduced/remained unfixed in 1.5-dev23 so the fix must be
|
|
backported to 1.5.
|
|
(cherry picked from commit bb2e669f9e73531ac9cc9277b40066b701eec918)
|
|
---
|
|
doc/internals/body-parsing.txt | 20 +++++++++++++-------
|
|
include/types/proto_http.h | 11 ++++++-----
|
|
src/proto_http.c | 9 +++++++--
|
|
3 files changed, 26 insertions(+), 14 deletions(-)
|
|
|
|
diff --git a/doc/internals/body-parsing.txt b/doc/internals/body-parsing.txt
|
|
index e9c8b4b..5baa549 100644
|
|
--- a/doc/internals/body-parsing.txt
|
|
+++ b/doc/internals/body-parsing.txt
|
|
@@ -67,12 +67,17 @@ msg.next : points to the next byte to inspect. This offset is automatically
|
|
automatically adjusted to the number of bytes already inspected.
|
|
|
|
msg.sov : start of value. First character of the header's value in the header
|
|
- states, start of the body in the data states until headers are
|
|
- forwarded. This offset is automatically adjusted when inserting or
|
|
- removing some headers. In data states, it always constains the size
|
|
- of the whole HTTP headers (including the trailing CRLF) that needs
|
|
- to be forwarded before the first byte of body. Once the headers are
|
|
- forwarded, this value drops to zero.
|
|
+ states, start of the body in the data states. Strictly positive
|
|
+ values indicate that headers were not forwarded yet (<buf.p> is
|
|
+ before the start of the body), and null or positive values are seen
|
|
+ after headers are forwarded (<buf.p> is at or past the start of the
|
|
+ body). The value stops changing when data start to leave the buffer
|
|
+ (in order to avoid integer overflows). So the maximum possible range
|
|
+ is -<buf.size> to +<buf.size>. This offset is automatically adjusted
|
|
+ when inserting or removing some headers. It is useful to rewind the
|
|
+ request buffer to the beginning of the body at any phase. The
|
|
+ response buffer does not really use it since it is immediately
|
|
+ forwarded to the client.
|
|
|
|
msg.sol : start of line. Points to the beginning of the current header line
|
|
while parsing headers. It is cleared to zero in the BODY state,
|
|
@@ -97,7 +102,8 @@ msg.eol : end of line. Points to the CRLF or LF of the current header line
|
|
states nor by forwarding.
|
|
|
|
The beginning of the message headers can always be found this way even after
|
|
-headers have been forwarded :
|
|
+headers or data have been forwarded, provided that everything is still present
|
|
+in the buffer :
|
|
|
|
headers = buf.p + msg->sov - msg->eoh - msg->eol
|
|
|
|
diff --git a/include/types/proto_http.h b/include/types/proto_http.h
|
|
index 12e1141..c53c7fd 100644
|
|
--- a/include/types/proto_http.h
|
|
+++ b/include/types/proto_http.h
|
|
@@ -329,7 +329,8 @@ enum {
|
|
* message or a response message.
|
|
*
|
|
* The values there are a little bit obscure, because their meaning can change
|
|
- * during the parsing :
|
|
+ * during the parsing. Please read carefully doc/internal/body-parsing.txt if
|
|
+ * you need to manipulate them. Quick reminder :
|
|
*
|
|
* - eoh (End of Headers) : relative offset in the buffer of first byte that
|
|
* is not part of a completely processed header.
|
|
@@ -344,9 +345,9 @@ enum {
|
|
* - sov (start of value) : Before HTTP_MSG_BODY, points to the value of
|
|
* the header being parsed. Starting from
|
|
* HTTP_MSG_BODY, will point to the start of the
|
|
- * body (relative to buffer's origin), or to data
|
|
- * following a chunk size. Thus <sov> bytes of
|
|
- * headers will have to be sent only once.
|
|
+ * body (relative to buffer's origin). It can be
|
|
+ * negative when forwarding data. It stops growing
|
|
+ * once data start to leave the buffer.
|
|
*
|
|
* - next (parse pointer) : next relative byte to be parsed. Always points
|
|
* to a byte matching the current state.
|
|
@@ -372,7 +373,7 @@ struct http_msg {
|
|
/* 6 bytes unused here */
|
|
struct channel *chn; /* pointer to the channel transporting the message */
|
|
unsigned int next; /* pointer to next byte to parse, relative to buf->p */
|
|
- unsigned int sov; /* current header: start of value */
|
|
+ int sov; /* current header: start of value ; data: start of body */
|
|
unsigned int eoh; /* End Of Headers, relative to buffer */
|
|
unsigned int sol; /* start of current line during parsing otherwise zero */
|
|
unsigned int eol; /* end of line */
|
|
diff --git a/src/proto_http.c b/src/proto_http.c
|
|
index 4a862b0..94afed7 100644
|
|
--- a/src/proto_http.c
|
|
+++ b/src/proto_http.c
|
|
@@ -5315,7 +5315,7 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
|
|
* an "Expect: 100-continue" header.
|
|
*/
|
|
|
|
- if (msg->sov) {
|
|
+ if (msg->sov > 0) {
|
|
/* we have msg->sov which points to the first byte of message
|
|
* body, and req->buf.p still points to the beginning of the
|
|
* message. We forward the headers now, as we don't need them
|
|
@@ -5429,6 +5429,8 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
|
|
* such as last chunk of data or trailers.
|
|
*/
|
|
b_adv(req->buf, msg->next);
|
|
+ if (unlikely(!(s->rep->flags & CF_READ_ATTACHED)))
|
|
+ msg->sov -= msg->next;
|
|
msg->next = 0;
|
|
|
|
/* for keep-alive we don't want to forward closes on DONE */
|
|
@@ -5479,6 +5481,9 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
|
|
missing_data:
|
|
/* we may have some pending data starting at req->buf->p */
|
|
b_adv(req->buf, msg->next);
|
|
+ if (unlikely(!(s->rep->flags & CF_READ_ATTACHED)))
|
|
+ msg->sov -= msg->next + MIN(msg->chunk_len, req->buf->i);
|
|
+
|
|
msg->next = 0;
|
|
msg->chunk_len -= channel_forward(req, msg->chunk_len);
|
|
|
|
@@ -6493,7 +6498,7 @@ int http_response_forward_body(struct session *s, struct channel *res, int an_bi
|
|
/* in most states, we should abort in case of early close */
|
|
channel_auto_close(res);
|
|
|
|
- if (msg->sov) {
|
|
+ if (msg->sov > 0) {
|
|
/* we have msg->sov which points to the first byte of message
|
|
* body, and res->buf.p still points to the beginning of the
|
|
* message. We forward the headers now, as we don't need them
|
|
--
|
|
1.8.5.5
|
|
|