]>
Commit | Line | Data |
---|---|---|
6edc270a MF |
1 | ------------------------------------------------------------ |
2 | revno: 14180 | |
3 | revision-id: squid3@treenet.co.nz-20170701120848-q2xznzfvxb4kwvb6 | |
4 | parent: squid3@treenet.co.nz-20170701095916-wknqmneq2w0mxt6a | |
5 | fixes bug: http://bugs.squid-cache.org/show_bug.cgi?id=4464 | |
6 | author: Christos Tsantilas <chtsanti@users.sourceforge.net> | |
7 | committer: Amos Jeffries <squid3@treenet.co.nz> | |
8 | branch nick: 3.5 | |
9 | timestamp: Sun 2017-07-02 00:08:48 +1200 | |
10 | message: | |
11 | Bug 4464: Reduce "!Comm::MonitorsRead(serverConnection->fd)" assertions. | |
12 | ||
13 | * Protect Squid Client classes from new requests that compete with | |
14 | ongoing pinned connection use and | |
15 | * resume dealing with new requests when those Client classes are done | |
16 | using the pinned connection. | |
17 | ||
18 | Replaced primary ConnStateData::pinConnection() calls with a pair of | |
19 | pinBusyConnection() and notePinnedConnectionBecameIdle() calls, | |
20 | depending on the pinned connection state ("busy" or "idle"). | |
21 | ||
22 | Removed pinConnection() parameters that were not longer used or could be computed from the remaining parameters. | |
23 | ||
24 | Removed ConnStateData::httpsPeeked() code "hiding" the originating | |
25 | request and connection peer details while entering the first "idle" | |
26 | state. The old (trunk r11880.1.6) bump-server-first code used a pair of | |
27 | NULLs because "Intercepted connections do not have requests at the | |
28 | connection pinning stage", but that limitation no longer applicable | |
29 | because Squid always fakes (when intercepting) or parses (a CONNECT) | |
30 | request now, even during SslBump step1. | |
31 | ||
32 | The added XXX and TODOs are not directly related to this fix. They | |
33 | were added to document problems discovered while working on this fix. | |
34 | ||
35 | In v3.5 code, the same problems manifest as Read.cc | |
36 | "fd_table[conn->fd].halfClosedReader != NULL" assertions. | |
37 | ||
38 | This is a Measurement Factory project | |
39 | ------------------------------------------------------------ | |
40 | # Bazaar merge directive format 2 (Bazaar 0.90) | |
41 | # revision_id: squid3@treenet.co.nz-20170701120848-q2xznzfvxb4kwvb6 | |
42 | # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 | |
43 | # testament_sha1: e4f432eed8a845431d4bbbf023de04d682adeaff | |
44 | # timestamp: 2017-07-01 12:32:26 +0000 | |
45 | # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 | |
46 | # base_revision_id: squid3@treenet.co.nz-20170701095916-\ | |
47 | # wknqmneq2w0mxt6a | |
48 | # | |
49 | # Begin patch | |
50 | === modified file 'src/FwdState.cc' | |
51 | --- src/FwdState.cc 2017-01-01 00:16:45 +0000 | |
52 | +++ src/FwdState.cc 2017-07-01 12:08:48 +0000 | |
53 | @@ -246,7 +246,7 @@ | |
54 | #if USE_OPENSSL | |
55 | if (request->flags.sslPeek && request->clientConnectionManager.valid()) { | |
56 | CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData, | |
57 | - ConnStateData::httpsPeeked, Comm::ConnectionPointer(NULL)); | |
58 | + ConnStateData::httpsPeeked, ConnStateData::PinnedIdleContext(Comm::ConnectionPointer(nullptr), request)); | |
59 | } | |
60 | #endif | |
61 | } else { | |
62 | @@ -952,7 +952,7 @@ | |
63 | #if USE_OPENSSL | |
64 | if (request->flags.sslPeek) { | |
65 | CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData, | |
66 | - ConnStateData::httpsPeeked, serverConnection()); | |
67 | + ConnStateData::httpsPeeked, ConnStateData::PinnedIdleContext(serverConnection(), request)); | |
68 | unregister(serverConn); // async call owns it now | |
69 | complete(); // destroys us | |
70 | return; | |
71 | ||
72 | === modified file 'src/base/RefCount.h' | |
73 | --- src/base/RefCount.h 2017-01-01 00:16:45 +0000 | |
74 | +++ src/base/RefCount.h 2017-07-01 12:08:48 +0000 | |
75 | @@ -54,9 +54,7 @@ | |
76 | ||
77 | C & operator * () const {return *const_cast<C *>(p_); } | |
78 | ||
79 | - C const * getRaw() const {return p_; } | |
80 | - | |
81 | - C * getRaw() {return const_cast<C *>(p_); } | |
82 | + C * getRaw() const { return const_cast<C *>(p_); } | |
83 | ||
84 | bool operator == (const RefCount& p) const { | |
85 | return p.p_ == p_; | |
86 | ||
87 | === modified file 'src/client_side.cc' | |
88 | --- src/client_side.cc 2017-05-29 13:15:55 +0000 | |
89 | +++ src/client_side.cc 2017-07-01 12:08:48 +0000 | |
90 | @@ -836,6 +836,7 @@ | |
91 | assert(areAllContextsForThisConnection()); | |
92 | freeAllContexts(); | |
93 | ||
94 | + // XXX: Closing pinned conn is too harsh: The Client may want to continue! | |
95 | unpinConnection(true); | |
96 | ||
97 | if (Comm::IsConnOpen(clientConnection)) | |
98 | @@ -1559,6 +1560,13 @@ | |
99 | ||
100 | debugs(33, 3, HERE << "ConnnStateData(" << conn->clientConnection << "), Context(" << clientConnection << ")"); | |
101 | connIsFinished(); | |
102 | + conn->kick(); | |
103 | +} | |
104 | + | |
105 | +void | |
106 | +ConnStateData::kick() | |
107 | +{ | |
108 | + ConnStateData * conn = this; // XXX: Remove this diff minimization hack | |
109 | ||
110 | if (conn->pinning.pinned && !Comm::IsConnOpen(conn->pinning.serverConnection)) { | |
111 | debugs(33, 2, HERE << conn->clientConnection << " Connection was pinned but server side gone. Terminating client connection"); | |
112 | @@ -3240,6 +3248,13 @@ | |
113 | if (in.buf.isEmpty()) | |
114 | break; | |
115 | ||
116 | + // Prohibit concurrent requests when using a pinned to-server connection | |
117 | + // because our Client classes do not support request pipelining. | |
118 | + if (pinning.pinned && !pinning.readHandler) { | |
119 | + debugs(33, 3, clientConnection << " waits for busy " << pinning.serverConnection); | |
120 | + break; | |
121 | + } | |
122 | + | |
123 | /* Limit the number of concurrent requests */ | |
124 | if (concurrentRequestQueueFilled()) | |
125 | break; | |
126 | @@ -4434,22 +4449,19 @@ | |
127 | } | |
128 | ||
129 | void | |
130 | -ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection) | |
131 | +ConnStateData::httpsPeeked(PinnedIdleContext pic) | |
132 | { | |
133 | Must(sslServerBump != NULL); | |
134 | + Must(sslServerBump->request == pic.request); | |
135 | + Must(currentobject == NULL || currentobject->http == NULL || currentobject->http->request == pic.request.getRaw()); | |
136 | ||
137 | - if (Comm::IsConnOpen(serverConnection)) { | |
138 | - pinConnection(serverConnection, NULL, NULL, false); | |
139 | + if (Comm::IsConnOpen(pic.connection)) { | |
140 | + notePinnedConnectionBecameIdle(pic); | |
141 | ||
142 | debugs(33, 5, HERE << "bumped HTTPS server: " << sslConnectHostOrIp); | |
143 | - } else { | |
144 | + } else | |
145 | debugs(33, 5, HERE << "Error while bumping: " << sslConnectHostOrIp); | |
146 | ||
147 | - // copy error detail from bump-server-first request to CONNECT request | |
148 | - if (currentobject != NULL && currentobject->http != NULL && currentobject->http->request) | |
149 | - currentobject->http->request->detailError(sslServerBump->request->errType, sslServerBump->request->errDetail); | |
150 | - } | |
151 | - | |
152 | getSslContextStart(); | |
153 | } | |
154 | ||
155 | @@ -4952,19 +4964,35 @@ | |
156 | } | |
157 | ||
158 | void | |
159 | -ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth, bool monitor) | |
160 | -{ | |
161 | - if (!Comm::IsConnOpen(pinning.serverConnection) || | |
162 | - pinning.serverConnection->fd != pinServer->fd) | |
163 | - pinNewConnection(pinServer, request, aPeer, auth); | |
164 | - | |
165 | - if (monitor) | |
166 | - startPinnedConnectionMonitoring(); | |
167 | -} | |
168 | - | |
169 | -void | |
170 | -ConnStateData::pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth) | |
171 | -{ | |
172 | +ConnStateData::pinBusyConnection(const Comm::ConnectionPointer &pinServer, const HttpRequest::Pointer &request) | |
173 | +{ | |
174 | + pinConnection(pinServer, request); | |
175 | +} | |
176 | + | |
177 | +void | |
178 | +ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext pic) | |
179 | +{ | |
180 | + Must(pic.connection != NULL); | |
181 | + Must(pic.request != NULL); | |
182 | + pinConnection(pic.connection, pic.request); | |
183 | + | |
184 | + // monitor pinned server connection for remote-end closures. | |
185 | + startPinnedConnectionMonitoring(); | |
186 | + | |
187 | + if (!currentobject) | |
188 | + kick(); // in case clientParseRequests() was blocked by a busy pic.connection | |
189 | +} | |
190 | + | |
191 | +/// Forward future client requests using the given server connection. | |
192 | +void | |
193 | +ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, const HttpRequest::Pointer &request) | |
194 | +{ | |
195 | + if (Comm::IsConnOpen(pinning.serverConnection) && | |
196 | + pinning.serverConnection->fd == pinServer->fd) { | |
197 | + debugs(33, 3, "already pinned" << pinServer); | |
198 | + return; | |
199 | + } | |
200 | + | |
201 | unpinConnection(true); // closes pinned connection, if any, and resets fields | |
202 | ||
203 | pinning.serverConnection = pinServer; | |
204 | @@ -4973,23 +5001,21 @@ | |
205 | ||
206 | Must(pinning.serverConnection != NULL); | |
207 | ||
208 | - // when pinning an SSL bumped connection, the request may be NULL | |
209 | const char *pinnedHost = "[unknown]"; | |
210 | - if (request) { | |
211 | + if (request != NULL) { | |
212 | pinning.host = xstrdup(request->GetHost()); | |
213 | pinning.port = request->port; | |
214 | pinnedHost = pinning.host; | |
215 | + pinning.auth = request->flags.connectionAuth; | |
216 | } else { | |
217 | pinning.port = pinServer->remote.port(); | |
218 | } | |
219 | pinning.pinned = true; | |
220 | - if (aPeer) | |
221 | - pinning.peer = cbdataReference(aPeer); | |
222 | - pinning.auth = auth; | |
223 | + pinning.peer = cbdataReference(pinServer->getPeer()); | |
224 | char stmp[MAX_IPSTRLEN]; | |
225 | char desc[FD_DESC_SZ]; | |
226 | snprintf(desc, FD_DESC_SZ, "%s pinned connection for %s (%d)", | |
227 | - (auth || !aPeer) ? pinnedHost : aPeer->name, | |
228 | + (pinning.auth || !pinning.peer) ? pinnedHost : pinning.peer->name, | |
229 | clientConnection->remote.toUrl(stmp,MAX_IPSTRLEN), | |
230 | clientConnection->fd); | |
231 | fd_note(pinning.serverConnection->fd, desc); | |
232 | @@ -5164,3 +5190,8 @@ | |
233 | * connection has gone away */ | |
234 | } | |
235 | ||
236 | +std::ostream & | |
237 | +operator <<(std::ostream &os, const ConnStateData::PinnedIdleContext &pic) | |
238 | +{ | |
239 | + return os << pic.connection << ", request=" << pic.request; | |
240 | +} | |
241 | ||
242 | === modified file 'src/client_side.h' | |
243 | --- src/client_side.h 2017-01-01 00:16:45 +0000 | |
244 | +++ src/client_side.h 2017-07-01 12:08:48 +0000 | |
245 | @@ -26,6 +26,8 @@ | |
246 | #include "ssl/support.h" | |
247 | #endif | |
248 | ||
249 | +#include <iosfwd> | |
250 | + | |
251 | class ConnStateData; | |
252 | class ClientHttpRequest; | |
253 | class clientStreamNode; | |
254 | @@ -188,6 +190,11 @@ | |
255 | /// Traffic parsing | |
256 | bool clientParseRequests(); | |
257 | void readNextRequest(); | |
258 | + | |
259 | + // In v3.5, usually called via ClientSocketContext::keepaliveNextRequest(). | |
260 | + /// try to make progress on a transaction or read more I/O | |
261 | + void kick(); | |
262 | + | |
263 | ClientSocketContext::Pointer getCurrentContext() const; | |
264 | void addContextToQueue(ClientSocketContext * context); | |
265 | int getConcurrentRequestCount() const; | |
266 | @@ -287,9 +294,21 @@ | |
267 | bool handleReadData(); | |
268 | bool handleRequestBodyData(); | |
269 | ||
270 | - /// Forward future client requests using the given server connection. | |
271 | - /// Optionally, monitor pinned server connection for remote-end closures. | |
272 | - void pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth, bool monitor = true); | |
273 | + /// parameters for the async notePinnedConnectionBecameIdle() call | |
274 | + class PinnedIdleContext | |
275 | + { | |
276 | + public: | |
277 | + PinnedIdleContext(const Comm::ConnectionPointer &conn, const HttpRequest::Pointer &req): connection(conn), request(req) {} | |
278 | + | |
279 | + Comm::ConnectionPointer connection; ///< to-server connection to be pinned | |
280 | + HttpRequest::Pointer request; ///< to-server request that initiated serverConnection | |
281 | + }; | |
282 | + | |
283 | + /// Called when a pinned connection becomes available for forwarding the next request. | |
284 | + void notePinnedConnectionBecameIdle(PinnedIdleContext pic); | |
285 | + /// Forward future client requests using the given to-server connection. | |
286 | + /// The connection is still being used by the current client request. | |
287 | + void pinBusyConnection(const Comm::ConnectionPointer &pinServerConn, const HttpRequest::Pointer &request); | |
288 | /// Undo pinConnection() and, optionally, close the pinned connection. | |
289 | void unpinConnection(const bool andClose); | |
290 | /// Returns validated pinnned server connection (and stops its monitoring). | |
291 | @@ -345,7 +364,7 @@ | |
292 | /// generated | |
293 | void doPeekAndSpliceStep(); | |
294 | /// called by FwdState when it is done bumping the server | |
295 | - void httpsPeeked(Comm::ConnectionPointer serverConnection); | |
296 | + void httpsPeeked(PinnedIdleContext pic); | |
297 | ||
298 | /// Start to create dynamic SSL_CTX for host or uses static port SSL context. | |
299 | void getSslContextStart(); | |
300 | @@ -449,7 +468,7 @@ | |
301 | void clientAfterReadingRequests(); | |
302 | bool concurrentRequestQueueFilled() const; | |
303 | ||
304 | - void pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth); | |
305 | + void pinConnection(const Comm::ConnectionPointer &pinServerConn, const HttpRequest::Pointer &request); | |
306 | ||
307 | /* PROXY protocol functionality */ | |
308 | bool proxyProtocolValidateClient(); | |
309 | @@ -516,5 +535,7 @@ | |
310 | void clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *context, const HttpRequestMethod& method, Http::ProtocolVersion http_ver); | |
311 | void clientPostHttpsAccept(ConnStateData *connState); | |
312 | ||
313 | +std::ostream &operator <<(std::ostream &os, const ConnStateData::PinnedIdleContext &pic); | |
314 | + | |
315 | #endif /* SQUID_CLIENTSIDE_H */ | |
316 | ||
317 | ||
318 | === modified file 'src/clients/FtpRelay.cc' | |
319 | --- src/clients/FtpRelay.cc 2017-01-01 00:16:45 +0000 | |
320 | +++ src/clients/FtpRelay.cc 2017-07-01 12:08:48 +0000 | |
321 | @@ -210,9 +210,10 @@ | |
322 | mgr->unpinConnection(false); | |
323 | ctrl.close(); | |
324 | } else { | |
325 | - mgr->pinConnection(ctrl.conn, fwd->request, | |
326 | - ctrl.conn->getPeer(), | |
327 | - fwd->request->flags.connectionAuth); | |
328 | + CallJobHere1(9, 4, mgr, | |
329 | + ConnStateData, | |
330 | + notePinnedConnectionBecameIdle, | |
331 | + ConnStateData::PinnedIdleContext(ctrl.conn, fwd->request)); | |
332 | ctrl.forget(); | |
333 | } | |
334 | } | |
335 | ||
336 | === modified file 'src/http.cc' | |
337 | --- src/http.cc 2017-06-15 00:16:33 +0000 | |
338 | +++ src/http.cc 2017-07-01 12:08:48 +0000 | |
339 | @@ -1383,9 +1383,6 @@ | |
340 | void | |
341 | HttpStateData::processReplyBody() | |
342 | { | |
343 | - Ip::Address client_addr; | |
344 | - bool ispinned = false; | |
345 | - | |
346 | if (!flags.headers_parsed) { | |
347 | flags.do_next_read = true; | |
348 | maybeReadVirginBody(); | |
349 | @@ -1435,35 +1432,49 @@ | |
350 | } | |
351 | break; | |
352 | ||
353 | - case COMPLETE_PERSISTENT_MSG: | |
354 | + case COMPLETE_PERSISTENT_MSG: { | |
355 | debugs(11, 5, "processReplyBody: COMPLETE_PERSISTENT_MSG from " << serverConnection); | |
356 | - /* yes we have to clear all these! */ | |
357 | + | |
358 | + // TODO: Remove serverConnectionSaved but preserve exception safety. | |
359 | + | |
360 | commUnsetConnTimeout(serverConnection); | |
361 | flags.do_next_read = false; | |
362 | ||
363 | comm_remove_close_handler(serverConnection->fd, closeHandler); | |
364 | closeHandler = NULL; | |
365 | - fwd->unregister(serverConnection); | |
366 | ||
367 | + Ip::Address client_addr; // XXX: Remove as unused. Why was it added? | |
368 | if (request->flags.spoofClientIp) | |
369 | client_addr = request->client_addr; | |
370 | ||
371 | + Comm::ConnectionPointer serverConnectionSaved = serverConnection; | |
372 | + fwd->unregister(serverConnection); | |
373 | + serverConnection = nullptr; | |
374 | + | |
375 | + bool ispinned = false; // TODO: Rename to isOrShouldBePinned | |
376 | if (request->flags.pinned) { | |
377 | ispinned = true; | |
378 | } else if (request->flags.connectionAuth && request->flags.authSent) { | |
379 | ispinned = true; | |
380 | } | |
381 | ||
382 | - if (ispinned && request->clientConnectionManager.valid()) { | |
383 | - request->clientConnectionManager->pinConnection(serverConnection, request, _peer, | |
384 | - (request->flags.connectionAuth)); | |
385 | + if (ispinned) { | |
386 | + if (request->clientConnectionManager.valid()) { | |
387 | + CallJobHere1(11, 4, request->clientConnectionManager, | |
388 | + ConnStateData, | |
389 | + notePinnedConnectionBecameIdle, | |
390 | + ConnStateData::PinnedIdleContext(serverConnectionSaved, request)); | |
391 | + } else { | |
392 | + // must not pool/share ispinned connections, even orphaned ones | |
393 | + serverConnectionSaved->close(); | |
394 | + } | |
395 | } else { | |
396 | - fwd->pconnPush(serverConnection, request->GetHost()); | |
397 | + fwd->pconnPush(serverConnectionSaved, request->GetHost()); | |
398 | } | |
399 | ||
400 | - serverConnection = NULL; | |
401 | serverComplete(); | |
402 | return; | |
403 | + } | |
404 | ||
405 | case COMPLETE_NONPERSISTENT_MSG: | |
406 | debugs(11, 5, "processReplyBody: COMPLETE_NONPERSISTENT_MSG from " << serverConnection); | |
407 | ||
408 | === modified file 'src/servers/FtpServer.cc' | |
409 | --- src/servers/FtpServer.cc 2017-02-26 11:09:42 +0000 | |
410 | +++ src/servers/FtpServer.cc 2017-07-01 12:08:48 +0000 | |
411 | @@ -301,12 +301,8 @@ | |
412 | Must(http != NULL); | |
413 | HttpRequest *const request = http->request; | |
414 | Must(request != NULL); | |
415 | - | |
416 | - // this is not an idle connection, so we do not want I/O monitoring | |
417 | - const bool monitor = false; | |
418 | - | |
419 | // make FTP peer connection exclusive to our request | |
420 | - pinConnection(conn, request, conn->getPeer(), false, monitor); | |
421 | + pinBusyConnection(conn, request); | |
422 | } | |
423 | ||
424 | void | |
425 | ||
426 | === modified file 'src/tests/stub_client_side.cc' | |
427 | --- src/tests/stub_client_side.cc 2017-01-01 00:16:45 +0000 | |
428 | +++ src/tests/stub_client_side.cc 2017-07-01 12:08:48 +0000 | |
429 | @@ -60,7 +60,8 @@ | |
430 | void ConnStateData::noteBodyConsumerAborted(BodyPipe::Pointer) STUB | |
431 | bool ConnStateData::handleReadData() STUB_RETVAL(false) | |
432 | bool ConnStateData::handleRequestBodyData() STUB_RETVAL(false) | |
433 | -void ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth, bool monitor) STUB | |
434 | +void ConnStateData::pinBusyConnection(const Comm::ConnectionPointer &, const HttpRequest::Pointer &) STUB | |
435 | +void ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext) STUB | |
436 | void ConnStateData::unpinConnection(const bool andClose) STUB | |
437 | const Comm::ConnectionPointer ConnStateData::validatePinnedConnection(HttpRequest *request, const CachePeer *peer) STUB_RETVAL(NULL) | |
438 | void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) STUB | |
439 | @@ -70,7 +71,7 @@ | |
440 | void ConnStateData::swanSong() STUB | |
441 | void ConnStateData::quitAfterError(HttpRequest *request) STUB | |
442 | #if USE_OPENSSL | |
443 | -void ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection) STUB | |
444 | +void ConnStateData::httpsPeeked(PinnedIdleContext) STUB | |
445 | void ConnStateData::getSslContextStart() STUB | |
446 | void ConnStateData::getSslContextDone(SSL_CTX * sslContext, bool isNew) STUB | |
447 | void ConnStateData::sslCrtdHandleReplyWrapper(void *data, const Helper::Reply &reply) STUB | |
448 |