[squid-dev] [PATCH] SBuf conversion of vary_headers
Amos Jeffries
squid3 at treenet.co.nz
Mon Mar 21 07:47:16 UTC 2016
This is the major part of the planned long-term fix for the Vary header
issues with String that have been bugging us recently.
It converts the makeVaryMark() function to generate its output in an
SBuf instead of String, and removes one use of strListAdd.
It also converts the MemObject and HttpRequest vary_headers members from
char* to SBuf to reduce the amount of data copying and simplify memory
management done on the resulting variant key.
Future Work:
* The remaining part of this patch is to SBuf convert the
HttpHeaders::getList API to avoid the remaining String. That is still
waiting on conversion of the ICAP and mime header parse logics.
* An additional optimisation that could be made to strengthen against
the known attacks is to ensure Vary header list entries are unique in
the generated key. Ignoring repeated header names.
Amos
-------------- next part --------------
=== modified file 'src/HttpRequest.cc'
--- src/HttpRequest.cc 2016-03-17 03:28:14 +0000
+++ src/HttpRequest.cc 2016-03-20 22:36:15 +0000
@@ -89,7 +89,7 @@
peer_login = NULL; // not allocated/deallocated by this class
peer_domain = NULL; // not allocated/deallocated by this class
peer_host = NULL;
- vary_headers = NULL;
+ vary_headers = SBuf();
myportname = null_string;
tag = null_string;
#if USE_AUTH
@@ -121,8 +121,7 @@
#if USE_AUTH
auth_user_request = NULL;
#endif
- safe_free(vary_headers);
-
+ vary_headers.clear();
url.clear();
header.clean();
@@ -197,7 +196,7 @@
copy->lastmod = lastmod;
copy->etag = etag;
- copy->vary_headers = vary_headers ? xstrdup(vary_headers) : NULL;
+ copy->vary_headers = vary_headers;
// XXX: what to do with copy->peer_domain?
copy->tag = tag;
=== modified file 'src/HttpRequest.h'
--- src/HttpRequest.h 2016-03-17 03:28:14 +0000
+++ src/HttpRequest.h 2016-03-20 22:36:47 +0000
@@ -148,7 +148,8 @@
time_t lastmod; /* Used on refreshes */
- const char *vary_headers; /* Used when varying entities are detected. Changes how the store key is calculated */
+ ///< Used when varying entities are detected. Changes how the store key is calculated
+ SBuf vary_headers;
char *peer_domain; /* Configured peer forceddomain */
=== modified file 'src/MemObject.cc'
--- src/MemObject.cc 2016-01-01 00:12:18 +0000
+++ src/MemObject.cc 2016-03-20 22:33:37 +0000
@@ -101,11 +101,10 @@
ircb_data(nullptr),
id(0),
object_sz(-1),
- swap_hdr_sz(0),
+ swap_hdr_sz(0)
#if URL_CHECKSUM_DEBUG
- chksum(0),
+ ,chksum(0)
#endif
- vary_headers(nullptr)
{
debugs(20, 3, "new MemObject " << this);
memset(&start_ping, 0, sizeof(start_ping));
@@ -145,8 +144,6 @@
HTTPMSGUNLOCK(request);
ctx_exit(ctx); /* must exit before we free mem->url */
-
- safe_free(vary_headers);
}
void
@@ -230,8 +227,8 @@
MemObject::stat(MemBuf * mb) const
{
mb->appendf("\t" SQUIDSBUFPH " %s\n", SQUIDSBUFPRINT(method.image()), logUri());
- if (vary_headers)
- mb->appendf("\tvary_headers: %s\n", vary_headers);
+ if (!vary_headers.isEmpty())
+ mb->appendf("\tvary_headers: " SQUIDSBUFPH "\n", SQUIDSBUFPRINT(vary_headers));
mb->appendf("\tinmem_lo: %" PRId64 "\n", inmem_lo);
mb->appendf("\tinmem_hi: %" PRId64 "\n", data_hdr.endOffset());
mb->appendf("\tswapout: %" PRId64 " bytes queued\n", swapout.queue_offset);
=== modified file 'src/MemObject.h'
--- src/MemObject.h 2016-02-24 18:12:43 +0000
+++ src/MemObject.h 2016-03-20 22:34:30 +0000
@@ -13,6 +13,7 @@
#include "dlink.h"
#include "http/RequestMethod.h"
#include "RemovalPolicy.h"
+#include "sbuf/SBuf.h"
#include "SquidString.h"
#include "stmem.h"
#include "StoreIOBuffer.h"
@@ -170,7 +171,7 @@
unsigned int chksum;
#endif
- const char *vary_headers;
+ SBuf vary_headers;
void delayRead(DeferredRead const &);
void kickReads();
=== modified file 'src/MemStore.cc'
--- src/MemStore.cc 2016-03-11 18:00:51 +0000
+++ src/MemStore.cc 2016-03-20 22:37:00 +0000
@@ -618,7 +618,7 @@
assert(e.mem_obj);
- if (e.mem_obj->vary_headers) {
+ if (!e.mem_obj->vary_headers.isEmpty()) {
// XXX: We must store/load SerialisedMetaData to cache Vary in RAM
debugs(20, 5, "Vary not yet supported: " << e.mem_obj->vary_headers);
return false;
=== modified file 'src/StoreMetaVary.cc'
--- src/StoreMetaVary.cc 2016-01-01 00:12:18 +0000
+++ src/StoreMetaVary.cc 2016-03-21 00:30:43 +0000
@@ -18,14 +18,14 @@
{
assert (getType() == STORE_META_VARY_HEADERS);
- if (!e->mem_obj->vary_headers) {
+ if (e->mem_obj->vary_headers.isEmpty()) {
/* XXX separate this mutator from the query */
/* Assume the object is OK.. remember the vary request headers */
- e->mem_obj->vary_headers = xstrdup((char *)value);
+ e->mem_obj->vary_headers = static_cast<char *>(value);
return true;
}
- if (strcmp(e->mem_obj->vary_headers, (char *)value) != 0)
+ if (e->mem_obj->vary_headers.cmp(static_cast<char *>(value)) != 0)
return false;
return true;
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2016-03-10 06:55:17 +0000
+++ src/client_side.cc 2016-03-20 22:39:49 +0000
@@ -3570,7 +3570,7 @@
int
varyEvaluateMatch(StoreEntry * entry, HttpRequest * request)
{
- const char *vary = request->vary_headers;
+ SBuf vary(request->vary_headers);
int has_vary = entry->getReply()->header.has(Http::HdrType::VARY);
#if X_ACCELERATOR_VARY
@@ -3578,12 +3578,12 @@
entry->getReply()->header.has(Http::HdrType::HDR_X_ACCELERATOR_VARY);
#endif
- if (!has_vary || !entry->mem_obj->vary_headers) {
- if (vary) {
+ if (!has_vary || entry->mem_obj->vary_headers.isEmpty()) {
+ if (!vary.isEmpty()) {
/* Oops... something odd is going on here.. */
debugs(33, DBG_IMPORTANT, "varyEvaluateMatch: Oops. Not a Vary object on second attempt, '" <<
entry->mem_obj->urlXXX() << "' '" << vary << "'");
- safe_free(request->vary_headers);
+ request->vary_headers.clear();
return VARY_CANCEL;
}
@@ -3597,8 +3597,8 @@
*/
vary = httpMakeVaryMark(request, entry->getReply());
- if (vary) {
- request->vary_headers = xstrdup(vary);
+ if (!vary.isEmpty()) {
+ request->vary_headers = vary;
return VARY_OTHER;
} else {
/* Ouch.. we cannot handle this kind of variance */
@@ -3606,18 +3606,18 @@
return VARY_CANCEL;
}
} else {
- if (!vary) {
+ if (vary.isEmpty()) {
vary = httpMakeVaryMark(request, entry->getReply());
- if (vary)
- request->vary_headers = xstrdup(vary);
+ if (!vary.isEmpty())
+ request->vary_headers = vary;
}
- if (!vary) {
+ if (vary.isEmpty()) {
/* Ouch.. we cannot handle this kind of variance */
/* XXX This cannot really happen, but just to be complete */
return VARY_CANCEL;
- } else if (strcmp(vary, entry->mem_obj->vary_headers) == 0) {
+ } else if (vary == entry->mem_obj->vary_headers) {
return VARY_MATCH;
} else {
/* Oops.. we have already been here and still haven't
=== modified file 'src/client_side_reply.cc'
--- src/client_side_reply.cc 2016-03-11 18:00:51 +0000
+++ src/client_side_reply.cc 2016-03-20 22:44:41 +0000
@@ -1007,9 +1007,8 @@
}
/* And for Vary, release the base URI if none of the headers was included in the request */
-
- if (http->request->vary_headers
- && !strstr(http->request->vary_headers, "=")) {
+ if (!http->request->vary_headers.isEmpty()
+ && http->request->vary_headers.find('=') != SBuf::npos) {
// XXX: performance regression, c_str() reallocates
SBuf tmp(http->request->effectiveRequestUri());
StoreEntry *entry = storeGetPublic(tmp.c_str(), Http::METHOD_GET);
=== modified file 'src/http.cc'
--- src/http.cc 2016-03-11 18:00:51 +0000
+++ src/http.cc 2016-03-20 22:45:07 +0000
@@ -578,7 +578,7 @@
* virtual headers in the reply
* Returns false if the variance cannot be stored
*/
-const char *
+SBuf
httpMakeVaryMark(HttpRequest * request, HttpReply const * reply)
{
String vary, hdr;
@@ -586,20 +586,21 @@
const char *item;
const char *value;
int ilen;
- static String vstr;
+ SBuf vstr;
- vstr.clean();
vary = reply->header.getList(Http::HdrType::VARY);
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(), ',');
+ if (!vstr.isEmpty())
+ vstr.append(", ", 2);
+ vstr.append(name);
hdr = request->header.getByName(name);
value = hdr.termedBuf();
if (value) {
@@ -622,7 +623,9 @@
char *name = (char *)xmalloc(ilen + 1);
xstrncpy(name, item, ilen + 1);
Tolower(name);
- strListAdd(&vstr, name, ',');
+ if (!vstr.isEmpty())
+ vstr.append(", ", 2);
+ vstr.append(name);
hdr = request->header.getByName(name);
safe_free(name);
value = hdr.termedBuf();
@@ -640,8 +643,8 @@
vary.clean();
#endif
- debugs(11, 3, "httpMakeVaryMark: " << vstr);
- return vstr.termedBuf();
+ debugs(11, 3, vstr);
+ return vstr;
}
void
@@ -942,15 +945,15 @@
|| 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;
}
}
=== modified file 'src/http.h'
--- src/http.h 2016-01-25 17:54:50 +0000
+++ src/http.h 2016-03-21 00:06:34 +0000
@@ -13,6 +13,7 @@
#include "comm.h"
#include "http/forward.h"
#include "HttpStateFlags.h"
+#include "sbuf/SBuf.h"
class FwdState;
class HttpHeader;
@@ -132,7 +133,7 @@
int httpCachable(const HttpRequestMethod&);
void httpStart(FwdState *);
-const char *httpMakeVaryMark(HttpRequest * request, HttpReply const * reply);
+SBuf httpMakeVaryMark(HttpRequest * request, HttpReply const * reply);
#endif /* SQUID_HTTP_H */
=== modified file 'src/store.cc'
--- src/store.cc 2016-02-19 15:06:42 +0000
+++ src/store.cc 2016-03-20 22:48:37 +0000
@@ -658,31 +658,28 @@
if (mem_obj->request) {
HttpRequest *request = mem_obj->request;
- if (!mem_obj->vary_headers) {
+ if (mem_obj->vary_headers.isEmpty()) {
/* First handle the case where the object no longer varies */
- safe_free(request->vary_headers);
+ request->vary_headers.clear();
} else {
- if (request->vary_headers && strcmp(request->vary_headers, mem_obj->vary_headers) != 0) {
+ if (!request->vary_headers.isEmpty() && request->vary_headers != mem_obj->vary_headers) {
/* Oops.. the variance has changed. Kill the base object
* to record the new variance key
*/
- safe_free(request->vary_headers); /* free old "bad" variance key */
+ request->vary_headers.clear(); /* free old "bad" variance key */
if (StoreEntry *pe = storeGetPublic(mem_obj->storeId(), mem_obj->method))
pe->release();
}
/* Make sure the request knows the variance status */
- if (!request->vary_headers) {
- const char *vary = httpMakeVaryMark(request, mem_obj->getReply());
-
- if (vary)
- request->vary_headers = xstrdup(vary);
+ if (request->vary_headers.isEmpty()) {
+ request->vary_headers = httpMakeVaryMark(request, mem_obj->getReply());
}
}
// TODO: storeGetPublic() calls below may create unlocked entries.
// We should add/use storeHas() API or lock/unlock those entries.
- if (mem_obj->vary_headers && !storeGetPublic(mem_obj->storeId(), mem_obj->method)) {
+ if (!mem_obj->vary_headers.isEmpty() && !storeGetPublic(mem_obj->storeId(), mem_obj->method)) {
/* Create "vary" base object */
String vary;
StoreEntry *pe = storeCreateEntry(mem_obj->storeId(), mem_obj->logUri(), request->flags, request->method);
=== modified file 'src/store_key_md5.cc'
--- src/store_key_md5.cc 2016-01-01 00:12:18 +0000
+++ src/store_key_md5.cc 2016-03-20 22:49:17 +0000
@@ -124,8 +124,8 @@
SquidMD5Update(&M, &m, sizeof(m));
SquidMD5Update(&M, (unsigned char *) url.rawContent(), url.length());
- if (request->vary_headers) {
- SquidMD5Update(&M, (unsigned char *) request->vary_headers, strlen(request->vary_headers));
+ if (!request->vary_headers.isEmpty()) {
+ SquidMD5Update(&M, request->vary_headers.rawContent(), request->vary_headers.length());
debugs(20, 3, "updating public key by vary headers: " << request->vary_headers << " for: " << url);
}
=== modified file 'src/store_swapmeta.cc'
--- src/store_swapmeta.cc 2016-03-11 17:24:13 +0000
+++ src/store_swapmeta.cc 2016-03-20 22:51:48 +0000
@@ -39,7 +39,6 @@
{
tlv *TLV = NULL; /* we'll return this */
tlv **T = &TLV;
- const char *vary;
assert(e->mem_obj != NULL);
const int64_t objsize = e->mem_obj->expectedReplySize();
@@ -87,10 +86,11 @@
}
T = StoreMeta::Add(T, t);
- vary = e->mem_obj->vary_headers;
+ SBuf vary = e->mem_obj->vary_headers;
- if (vary) {
- t =StoreMeta::Factory(STORE_META_VARY_HEADERS, strlen(vary) + 1, vary);
+ if (!vary.isEmpty()) {
+ // XXX: do we still need +1 here?
+ t = StoreMeta::Factory(STORE_META_VARY_HEADERS, vary.length() + 1, vary.c_str());
if (!t) {
storeSwapTLVFree(TLV);
=== modified file 'src/tests/stub_MemObject.cc'
--- src/tests/stub_MemObject.cc 2016-01-01 00:12:18 +0000
+++ src/tests/stub_MemObject.cc 2016-03-20 22:35:17 +0000
@@ -38,7 +38,6 @@
id(0),
object_sz(-1),
swap_hdr_sz(0),
- vary_headers(NULL),
_reply(NULL)
{
memset(&clients, 0, sizeof(clients));
=== modified file 'src/tests/stub_http.cc'
--- src/tests/stub_http.cc 2016-01-01 00:12:18 +0000
+++ src/tests/stub_http.cc 2016-03-20 20:36:54 +0000
@@ -7,12 +7,12 @@
*/
#include "squid.h"
-
#include "HttpReply.h"
#include "HttpRequest.h"
+#include "SBuf.h"
#define STUB_API "http.cc"
#include "tests/STUB.h"
-const char * httpMakeVaryMark(HttpRequest * request, HttpReply const * reply) STUB_RETVAL(NULL)
+SBuf httpMakeVaryMark(HttpRequest * request, HttpReply const * reply) STUB_RETVAL(SBuf())
More information about the squid-dev
mailing list