Bug 4563: duplicate code in httpMakeVaryMark

[thirdparty/squid.git] / src / http.cc

diff --git a/src/http.cc b/src/http.cc
index 8bc14a3f46e648ca4409246bd962e484871c094a..a0a30cb80b9fd034e43f5752ddf8572b08598bee 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -83,8 +83,6 @@ static const char *const crlf = "\r\n";
 static void httpMaybeRemovePublic(StoreEntry *, Http::StatusCode);
 static void copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, const String strConnection, const HttpRequest * request,
         HttpHeader * hdr_out, const int we_do_ranges, const HttpStateFlags &);
-//Declared in HttpHeaderTools.cc
-void httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPointer &al, HeaderWithAclList &headers_add);
 
 HttpStateData::HttpStateData(FwdState *theFwdState) :
     AsyncJob("HttpStateData"),
@@ -166,7 +164,8 @@ HttpStateData::httpTimeout(const CommTimeoutCbParams &)
         fwd->fail(new ErrorState(ERR_READ_TIMEOUT, Http::scGatewayTimeout, fwd->request));
     }
 
-    serverConnection->close();
+    closeServer();
+    mustStop("HttpStateData::httpTimeout");
 }
 
 /// Remove an existing public store entry if the incoming response (to be
@@ -572,35 +571,27 @@ HttpStateData::cacheableReply()
     /* NOTREACHED */
 }
 
-/*
- * For Vary, store the relevant request headers as
- * virtual headers in the reply
- * Returns false if the variance cannot be stored
- */
-const char *
-httpMakeVaryMark(HttpRequest * request, HttpReply const * reply)
+/// assemble a variant key (vary-mark) from the given Vary header and HTTP request
+static void
+assembleVaryKey(String &vary, SBuf &vstr, const HttpRequest &request)
 {
-    String vary, hdr;
-    const char *pos = NULL;
-    const char *item;
-    const char *value;
-    int ilen;
-    static String vstr;
-
-    vstr.clean();
-    vary = reply->header.getList(Http::HdrType::VARY);
+    static const SBuf asterisk("*");
+    const char *pos = nullptr;
+    const char *item = nullptr;
+    int ilen = 0;
 
     while (strListGetItem(&vary, ',', &item, &ilen, &pos)) {
-        static const SBuf asterisk("*");
         SBuf name(item, ilen);
         if (name == asterisk) {
-            vstr.clean();
+            vstr.clear();
             break;
         }
         name.toLower();
-        strListAdd(&vstr, name.c_str(), ',');
-        hdr = request->header.getByName(name);
-        value = hdr.termedBuf();
+        if (!vstr.isEmpty())
+            vstr.append(", ", 2);
+        vstr.append(name);
+        String hdr(request.header.getByName(name));
+        const char *value = hdr.termedBuf();
         if (value) {
             value = rfc1738_escape_part(value);
             vstr.append("=\"", 2);
@@ -610,37 +601,30 @@ httpMakeVaryMark(HttpRequest * request, HttpReply const * reply)
 
         hdr.clean();
     }
+}
 
-    vary.clean();
-#if X_ACCELERATOR_VARY
-
-    pos = NULL;
-    vary = reply->header.getList(Http::HdrType::HDR_X_ACCELERATOR_VARY);
-
-    while (strListGetItem(&vary, ',', &item, &ilen, &pos)) {
-        char *name = (char *)xmalloc(ilen + 1);
-        xstrncpy(name, item, ilen + 1);
-        Tolower(name);
-        strListAdd(&vstr, name, ',');
-        hdr = request->header.getByName(name);
-        safe_free(name);
-        value = hdr.termedBuf();
-
-        if (value) {
-            value = rfc1738_escape_part(value);
-            vstr.append("=\"", 2);
-            vstr.append(value);
-            vstr.append("\"", 1);
-        }
+/*
+ * For Vary, store the relevant request headers as
+ * virtual headers in the reply
+ * Returns an empty SBuf if the variance cannot be stored
+ */
+SBuf
+httpMakeVaryMark(HttpRequest * request, HttpReply const * reply)
+{
+    SBuf vstr;
+    String vary;
 
-        hdr.clean();
-    }
+    vary = reply->header.getList(Http::HdrType::VARY);
+    assembleVaryKey(vary, vstr, *request);
 
+#if X_ACCELERATOR_VARY
     vary.clean();
+    vary = reply->header.getList(Http::HdrType::HDR_X_ACCELERATOR_VARY);
+    assembleVaryKey(vary, vstr, *request);
 #endif
 
-    debugs(11, 3, "httpMakeVaryMark: " << vstr);
-    return vstr.termedBuf();
+    debugs(11, 3, vstr);
+    return vstr;
 }
 
 void
@@ -941,15 +925,15 @@ HttpStateData::haveParsedReplyHeaders()
             || rep->header.has(Http::HdrType::HDR_X_ACCELERATOR_VARY)
 #endif
        ) {
-        const char *vary = httpMakeVaryMark(request, rep);
+        const SBuf vary(httpMakeVaryMark(request, rep));
 
-        if (!vary) {
+        if (vary.isEmpty()) {
             entry->makePrivate();
             if (!fwd->reforwardableStatus(rep->sline.status()))
                 EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
             varyFailure = true;
         } else {
-            entry->mem_obj->vary_headers = xstrdup(vary);
+            entry->mem_obj->vary_headers = vary;
         }
     }
 
@@ -1234,8 +1218,8 @@ HttpStateData::readReply(const CommIoCbParams &io)
         err->xerrno = rd.xerrno;
         fwd->fail(err);
         flags.do_next_read = false;
-        io.conn->close();
-
+        closeServer();
+        mustStop("HttpStateData::readReply");
         return;
     }
 
@@ -1335,7 +1319,8 @@ HttpStateData::continueAfterParsingHeader()
     entry->reset();
     fwd->fail(new ErrorState(error, Http::scBadGateway, fwd->request));
     flags.do_next_read = false;
-    serverConnection->close();
+    closeServer();
+    mustStop("HttpStateData::continueAfterParsingHeader");
     return false; // quit on error
 }
 
@@ -1446,8 +1431,11 @@ HttpStateData::processReplyBody()
             writeReplyBody();
     }
 
+    // storing/sending methods like earlier adaptOrFinalizeReply() or
+    // above writeReplyBody() may release/abort the store entry.
     if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
-        // The above writeReplyBody() call may have aborted the store entry.
+        // TODO: In some cases (e.g., 304), we should keep persistent conn open.
+        // Detect end-of-reply (and, hence, pool our idle pconn) earlier (ASAP).
         abortTransaction("store entry aborted while storing reply");
         return;
     } else
@@ -1599,7 +1587,8 @@ HttpStateData::wroteLast(const CommIoCbParams &io)
         ErrorState *err = new ErrorState(ERR_WRITE_ERROR, Http::scBadGateway, fwd->request);
         err->xerrno = io.xerrno;
         fwd->fail(err);
-        serverConnection->close();
+        closeServer();
+        mustStop("HttpStateData::wroteLast");
         return;
     }
 
@@ -1627,7 +1616,6 @@ HttpStateData::sendComplete()
     request->hier.peer_http_request_sent = current_time;
 }
 
-// Close the HTTP server connection. Used by serverComplete().
 void
 HttpStateData::closeServer()
 {
@@ -1937,11 +1925,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
     }
 
     /* Now mangle the headers. */
-    if (Config2.onoff.mangle_request_headers)
-        httpHdrMangleList(hdr_out, request, ROR_REQUEST);
-
-    if (Config.request_header_add && !Config.request_header_add->empty())
-        httpHdrAdd(hdr_out, request, al, *Config.request_header_add);
+    httpHdrMangleList(hdr_out, request, al, ROR_REQUEST);
 
     strConnection.clean();
 }
@@ -2426,7 +2410,8 @@ HttpStateData::handleMoreRequestBodyAvailable()
             debugs(11, DBG_IMPORTANT, "http handleMoreRequestBodyAvailable: Likely proxy abuse detected '" << request->client_addr << "' -> '" << entry->url() << "'" );
 
             if (virginReply()->sline.status() == Http::scInvalidHeader) {
-                serverConnection->close();
+                closeServer();
+                mustStop("HttpStateData::handleMoreRequestBodyAvailable");
                 return;
             }
         }