|
|
@@ -0,0 +1,167 @@
|
|
|
1
|
+From 5bebcd06287be9024f0fba25f350393f02e050c1 Mon Sep 17 00:00:00 2001
|
|
|
2
|
+From: Willy Tarreau <w@1wt.eu>
|
|
|
3
|
+Date: Thu, 10 Jul 2014 19:06:10 +0200
|
|
|
4
|
+Subject: [PATCH 24/25] BUG/MAJOR: http: correctly rewind the request body
|
|
|
5
|
+ after start of forwarding
|
|
|
6
|
+
|
|
|
7
|
+Daniel Dubovik reported an interesting bug showing that the request body
|
|
|
8
|
+processing was still not 100% fixed. If a POST request contained short
|
|
|
9
|
+enough data to be forwarded at once before trying to establish the
|
|
|
10
|
+connection to the server, we had no way to correctly rewind the body.
|
|
|
11
|
+
|
|
|
12
|
+The first visible case is that balancing on a header does not always work
|
|
|
13
|
+on such POST requests since the header cannot be found. But there are even
|
|
|
14
|
+nastier implications which are that http-send-name-header would apply to
|
|
|
15
|
+the wrong location and possibly even affect part of the request's body
|
|
|
16
|
+due to an incorrect rewinding.
|
|
|
17
|
+
|
|
|
18
|
+There are two options to fix the problem :
|
|
|
19
|
+ - first one is to force the HTTP_MSG_F_WAIT_CONN flag on all hash-based
|
|
|
20
|
+ balancing algorithms and http-send-name-header, but there's always a
|
|
|
21
|
+ risk that any new algorithm forgets to set it ;
|
|
|
22
|
+
|
|
|
23
|
+ - the second option is to account for the amount of skipped data before
|
|
|
24
|
+ the connection establishes so that we always know the position of the
|
|
|
25
|
+ request's body relative to the buffer's origin.
|
|
|
26
|
+
|
|
|
27
|
+The second option is much more reliable and fits very well in the spirit
|
|
|
28
|
+of the past changes to fix forwarding. Indeed, at the moment we have
|
|
|
29
|
+msg->sov which points to the start of the body before headers are forwarded
|
|
|
30
|
+and which equals zero afterwards (so it still points to the start of the
|
|
|
31
|
+body before forwarding data). A minor change consists in always making it
|
|
|
32
|
+point to the start of the body even after data have been forwarded. It means
|
|
|
33
|
+that it can get a negative value (so we need to change its type to signed)..
|
|
|
34
|
+
|
|
|
35
|
+In order to avoid wrapping, we only do this as long as the other side of
|
|
|
36
|
+the buffer is not connected yet.
|
|
|
37
|
+
|
|
|
38
|
+Doing this definitely fixes the issues above for the requests. Since the
|
|
|
39
|
+response cannot be rewound we don't need to perform any change there.
|
|
|
40
|
+
|
|
|
41
|
+This bug was introduced/remained unfixed in 1.5-dev23 so the fix must be
|
|
|
42
|
+backported to 1.5.
|
|
|
43
|
+(cherry picked from commit bb2e669f9e73531ac9cc9277b40066b701eec918)
|
|
|
44
|
+---
|
|
|
45
|
+ doc/internals/body-parsing.txt | 20 +++++++++++++-------
|
|
|
46
|
+ include/types/proto_http.h | 11 ++++++-----
|
|
|
47
|
+ src/proto_http.c | 9 +++++++--
|
|
|
48
|
+ 3 files changed, 26 insertions(+), 14 deletions(-)
|
|
|
49
|
+
|
|
|
50
|
+diff --git a/doc/internals/body-parsing.txt b/doc/internals/body-parsing.txt
|
|
|
51
|
+index e9c8b4b..5baa549 100644
|
|
|
52
|
+--- a/doc/internals/body-parsing.txt
|
|
|
53
|
++++ b/doc/internals/body-parsing.txt
|
|
|
54
|
+@@ -67,12 +67,17 @@ msg.next : points to the next byte to inspect. This offset is automatically
|
|
|
55
|
+ automatically adjusted to the number of bytes already inspected.
|
|
|
56
|
+
|
|
|
57
|
+ msg.sov : start of value. First character of the header's value in the header
|
|
|
58
|
+- states, start of the body in the data states until headers are
|
|
|
59
|
+- forwarded. This offset is automatically adjusted when inserting or
|
|
|
60
|
+- removing some headers. In data states, it always constains the size
|
|
|
61
|
+- of the whole HTTP headers (including the trailing CRLF) that needs
|
|
|
62
|
+- to be forwarded before the first byte of body. Once the headers are
|
|
|
63
|
+- forwarded, this value drops to zero.
|
|
|
64
|
++ states, start of the body in the data states. Strictly positive
|
|
|
65
|
++ values indicate that headers were not forwarded yet (<buf.p> is
|
|
|
66
|
++ before the start of the body), and null or positive values are seen
|
|
|
67
|
++ after headers are forwarded (<buf.p> is at or past the start of the
|
|
|
68
|
++ body). The value stops changing when data start to leave the buffer
|
|
|
69
|
++ (in order to avoid integer overflows). So the maximum possible range
|
|
|
70
|
++ is -<buf.size> to +<buf.size>. This offset is automatically adjusted
|
|
|
71
|
++ when inserting or removing some headers. It is useful to rewind the
|
|
|
72
|
++ request buffer to the beginning of the body at any phase. The
|
|
|
73
|
++ response buffer does not really use it since it is immediately
|
|
|
74
|
++ forwarded to the client.
|
|
|
75
|
+
|
|
|
76
|
+ msg.sol : start of line. Points to the beginning of the current header line
|
|
|
77
|
+ while parsing headers. It is cleared to zero in the BODY state,
|
|
|
78
|
+@@ -97,7 +102,8 @@ msg.eol : end of line. Points to the CRLF or LF of the current header line
|
|
|
79
|
+ states nor by forwarding.
|
|
|
80
|
+
|
|
|
81
|
+ The beginning of the message headers can always be found this way even after
|
|
|
82
|
+-headers have been forwarded :
|
|
|
83
|
++headers or data have been forwarded, provided that everything is still present
|
|
|
84
|
++in the buffer :
|
|
|
85
|
+
|
|
|
86
|
+ headers = buf.p + msg->sov - msg->eoh - msg->eol
|
|
|
87
|
+
|
|
|
88
|
+diff --git a/include/types/proto_http.h b/include/types/proto_http.h
|
|
|
89
|
+index 12e1141..c53c7fd 100644
|
|
|
90
|
+--- a/include/types/proto_http.h
|
|
|
91
|
++++ b/include/types/proto_http.h
|
|
|
92
|
+@@ -329,7 +329,8 @@ enum {
|
|
|
93
|
+ * message or a response message.
|
|
|
94
|
+ *
|
|
|
95
|
+ * The values there are a little bit obscure, because their meaning can change
|
|
|
96
|
+- * during the parsing :
|
|
|
97
|
++ * during the parsing. Please read carefully doc/internal/body-parsing.txt if
|
|
|
98
|
++ * you need to manipulate them. Quick reminder :
|
|
|
99
|
+ *
|
|
|
100
|
+ * - eoh (End of Headers) : relative offset in the buffer of first byte that
|
|
|
101
|
+ * is not part of a completely processed header.
|
|
|
102
|
+@@ -344,9 +345,9 @@ enum {
|
|
|
103
|
+ * - sov (start of value) : Before HTTP_MSG_BODY, points to the value of
|
|
|
104
|
+ * the header being parsed. Starting from
|
|
|
105
|
+ * HTTP_MSG_BODY, will point to the start of the
|
|
|
106
|
+- * body (relative to buffer's origin), or to data
|
|
|
107
|
+- * following a chunk size. Thus <sov> bytes of
|
|
|
108
|
+- * headers will have to be sent only once.
|
|
|
109
|
++ * body (relative to buffer's origin). It can be
|
|
|
110
|
++ * negative when forwarding data. It stops growing
|
|
|
111
|
++ * once data start to leave the buffer.
|
|
|
112
|
+ *
|
|
|
113
|
+ * - next (parse pointer) : next relative byte to be parsed. Always points
|
|
|
114
|
+ * to a byte matching the current state.
|
|
|
115
|
+@@ -372,7 +373,7 @@ struct http_msg {
|
|
|
116
|
+ /* 6 bytes unused here */
|
|
|
117
|
+ struct channel *chn; /* pointer to the channel transporting the message */
|
|
|
118
|
+ unsigned int next; /* pointer to next byte to parse, relative to buf->p */
|
|
|
119
|
+- unsigned int sov; /* current header: start of value */
|
|
|
120
|
++ int sov; /* current header: start of value ; data: start of body */
|
|
|
121
|
+ unsigned int eoh; /* End Of Headers, relative to buffer */
|
|
|
122
|
+ unsigned int sol; /* start of current line during parsing otherwise zero */
|
|
|
123
|
+ unsigned int eol; /* end of line */
|
|
|
124
|
+diff --git a/src/proto_http.c b/src/proto_http.c
|
|
|
125
|
+index 4a862b0..94afed7 100644
|
|
|
126
|
+--- a/src/proto_http.c
|
|
|
127
|
++++ b/src/proto_http.c
|
|
|
128
|
+@@ -5315,7 +5315,7 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
|
|
|
129
|
+ * an "Expect: 100-continue" header.
|
|
|
130
|
+ */
|
|
|
131
|
+
|
|
|
132
|
+- if (msg->sov) {
|
|
|
133
|
++ if (msg->sov > 0) {
|
|
|
134
|
+ /* we have msg->sov which points to the first byte of message
|
|
|
135
|
+ * body, and req->buf.p still points to the beginning of the
|
|
|
136
|
+ * message. We forward the headers now, as we don't need them
|
|
|
137
|
+@@ -5429,6 +5429,8 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
|
|
|
138
|
+ * such as last chunk of data or trailers.
|
|
|
139
|
+ */
|
|
|
140
|
+ b_adv(req->buf, msg->next);
|
|
|
141
|
++ if (unlikely(!(s->rep->flags & CF_READ_ATTACHED)))
|
|
|
142
|
++ msg->sov -= msg->next;
|
|
|
143
|
+ msg->next = 0;
|
|
|
144
|
+
|
|
|
145
|
+ /* for keep-alive we don't want to forward closes on DONE */
|
|
|
146
|
+@@ -5479,6 +5481,9 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
|
|
|
147
|
+ missing_data:
|
|
|
148
|
+ /* we may have some pending data starting at req->buf->p */
|
|
|
149
|
+ b_adv(req->buf, msg->next);
|
|
|
150
|
++ if (unlikely(!(s->rep->flags & CF_READ_ATTACHED)))
|
|
|
151
|
++ msg->sov -= msg->next + MIN(msg->chunk_len, req->buf->i);
|
|
|
152
|
++
|
|
|
153
|
+ msg->next = 0;
|
|
|
154
|
+ msg->chunk_len -= channel_forward(req, msg->chunk_len);
|
|
|
155
|
+
|
|
|
156
|
+@@ -6493,7 +6498,7 @@ int http_response_forward_body(struct session *s, struct channel *res, int an_bi
|
|
|
157
|
+ /* in most states, we should abort in case of early close */
|
|
|
158
|
+ channel_auto_close(res);
|
|
|
159
|
+
|
|
|
160
|
+- if (msg->sov) {
|
|
|
161
|
++ if (msg->sov > 0) {
|
|
|
162
|
+ /* we have msg->sov which points to the first byte of message
|
|
|
163
|
+ * body, and res->buf.p still points to the beginning of the
|
|
|
164
|
+ * message. We forward the headers now, as we don't need them
|
|
|
165
|
+--
|
|
|
166
|
+1.8.5.5
|
|
|
167
|
+
|
Thomas Heil
10 years ago