RepoMirrors/haproxy
1
0
Fork 0
mirror of http://git.haproxy.org/git/haproxy.git/ synced 2024-12-16 08:24:42 +00:00
Code Issues Packages Projects Releases Wiki Activity

BUG/MAJOR: http: correctly rewind the request body after start of forwarding

Browse Source 
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.
This commit is contained in:
Willy Tarreau 2014-07-10 19:06:10 +02:00
parent 0dbfdbaef1
commit bb2e669f9e
3 changed files with 26 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
doc/internals/body-parsing.txt
View File

@ -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

11
include/types/proto_http.h
View File

@ -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 */

9
src/proto_http.c
View File

@ -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