Add temporary SBufToCstring() helper functions for SBuf transition

These functions provide safe replacement for xstrdup() and xstrncpy()
that guarantees 0-termination of the output c-string but do not have
any side effects or behaviour guarantees affecting the source SBuf
internal state.

This lack of side effects is important for the transitional period
where a lot of buffer contents will be copied out of SBuf but are
'read-only' and need to avoid overheads such as the reallocating
twice (or more) that would occur if using SBuf::c_str().

Effective immediately we have a ban on using the xstr*() group of
helper functions to copy data out of SBuf::raw*() accessors. The
xstr*() and all other common system str*() use c-string dependent
operations internally which on non-0-terminated SBuf internals can
result in nasty performance issues (ie. strlen() of 2 MB 'string').

diff --git a/src/SBuf.h b/src/SBuf.h
index 6f39f4bd17a7af1b706d69dbd7e101b161f829a2..31c51cdaf0268dc32493a460063b3d7daec77c3f 100644 (file)
--- a/src/SBuf.h
+++ b/src/SBuf.h
@@ -12,6 +12,7 @@
 #define SQUID_SBUF_H
 
 #include "base/InstanceId.h"
+#include "Debug.h"
 #include "MemBlob.h"
 #include "SBufExceptions.h"
 #include "SquidString.h"
@@ -722,6 +723,46 @@ ToLower(SBuf buf)
     return buf;
 }
 
+/**
+ * Copy an SBuf into a C-string.
+ *
+ * Guarantees that the output is a c-string of length
+ * no more than SBuf::length()+1 by appending a \0 byte
+ * to the C-string copy of the SBuf contents.
+ *
+ * \note The destination c-string memory MUST be of at least
+ *       length()+1 bytes.
+ *
+ * No protection is added to prevent \0 bytes within the string.
+ * Unexpectedly short strings are a problem for the receiver
+ * to deal with if it cares.
+ *
+ * Unlike SBuf::c_str() does not alter the SBuf in any way.
+ */
+inline void
+SBufToCstring(char *d, const SBuf &s)
+{
+    s.copy(d, s.length());
+    d[s.length()] = '\0'; // 0-terminate the destination
+    debugs(1, DBG_DATA, "built c-string '" << d << "' from " << s);
+}
+
+/**
+ * Copy an SBuf into a C-string.
+ *
+ * \see SBufToCstring(char *d, const SBuf &s)
+ *
+ * \returns A dynamically allocated c-string based on SBuf.
+ *          Use xfree() / safe_free() to release the c-string.
+ */
+inline char *
+SBufToCstring(const SBuf &s)
+{
+    char *d = static_cast<char*>(xmalloc(s.length()+1));
+    SBufToCstring(d, s);
+    return d;
+}
+
 inline
 SBufIterator::SBufIterator(const SBuf &s, size_type pos)
     : iter(s.rawContent()+pos)

diff --git a/src/acl/Url.cc b/src/acl/Url.cc
index 5a83c8cfb365d13e0cd679aba8f5bc01f726e88c..31213741d3de549812f2f6da1b1b9d676c030016 100644 (file)
--- a/src/acl/Url.cc
+++ b/src/acl/Url.cc
@@ -19,8 +19,7 @@
 int
 ACLUrlStrategy::match (ACLData<char const *> * &data, ACLFilledChecklist *checklist, ACLFlags &)
 {
-    const SBuf &tmp = checklist->request->effectiveRequestUri();
-    char *esc_buf = xstrndup(tmp.rawContent(), tmp.length()+1);
+    char *esc_buf = SBufToCstring(checklist->request->effectiveRequestUri());
     rfc1738_unescape(esc_buf);
     int result = data->match(esc_buf);
     xfree(esc_buf);

diff --git a/src/acl/UrlPath.cc b/src/acl/UrlPath.cc
index 513500467b40ff3585bb412799e71cdf403c14d6..75b9962458cc0ee67f2a11db661a04e712e276a7 100644 (file)
--- a/src/acl/UrlPath.cc
+++ b/src/acl/UrlPath.cc
@@ -21,8 +21,7 @@ ACLUrlPathStrategy::match (ACLData<char const *> * &data, ACLFilledChecklist *ch
     if (checklist->request->url.path().isEmpty())
         return -1;
 
-    SBuf tmp = checklist->request->url.path();
-    char *esc_buf = xstrndup(tmp.rawContent(), tmp.length()+1);
+    char *esc_buf = SBufToCstring(checklist->request->url.path());
     rfc1738_unescape(esc_buf);
     int result = data->match(esc_buf);
     xfree(esc_buf);

diff --git a/src/client_side.cc b/src/client_side.cc
index 87c7ad31a14e73321f1c8f47c20ef32578c4c149..5753c3e43b744d6846fc49038d1288ffe6aa62da 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2266,7 +2266,7 @@ parseHttpRequest(ConnStateData *csd, const Http1::RequestParserPointer &hp)
          * requested url. may be rewritten later, so make extra room */
         int url_sz = hp->requestUri().length() + Config.appendDomainLen + 5;
         http->uri = (char *)xcalloc(url_sz, 1);
-        xstrncpy(http->uri, hp->requestUri().rawContent(), hp->requestUri().length()+1);
+        SBufToCstring(http->uri, hp->requestUri());
     }
 
     result->flags.parsed_ok = 1;

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 866f0fc96d74f5a3d2d6bd142405b4c11f6f80fa..cdec22e5a93deb0057d6d8139dca8a292474a21a 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -833,9 +833,8 @@ ClientRequestContext::clientAccessCheckDone(const allow_t &answer)
     }
 
     /* ACCESS_ALLOWED continues here ... */
-    safe_free(http->uri);
-    const SBuf tmp(http->request->effectiveRequestUri());
-    http->uri = xstrndup(tmp.rawContent(), tmp.length()+1);
+    xfree(http->uri);
+    http->uri = SBufToCstring(http->request->effectiveRequestUri());
     http->doCallouts();
 }
 
@@ -1312,9 +1311,8 @@ ClientRequestContext::clientRedirectDone(const Helper::Reply &reply)
                     }
 
                     // update the current working ClientHttpRequest fields
-                    safe_free(http->uri);
-                    const SBuf tmp(new_request->effectiveRequestUri());
-                    http->uri = xstrndup(tmp.rawContent(), tmp.length()+1);
+                    xfree(http->uri);
+                    http->uri = SBufToCstring(new_request->effectiveRequestUri());
                     HTTPMSGUNLOCK(old_request);
                     http->request = new_request;
                     HTTPMSGLOCK(http->request);
@@ -1919,8 +1917,7 @@ ClientHttpRequest::handleAdaptedHeader(HttpMsg *msg)
          * Store the new URI for logging
          */
         xfree(uri);
-        const SBuf tmp(request->effectiveRequestUri());
-        uri = xstrndup(tmp.rawContent(), tmp.length()+1);
+        uri = SBufToCstring(request->effectiveRequestUri());
         setLogUri(this, urlCanonicalClean(request));
         assert(request->method.id());
     } else if (HttpReply *new_rep = dynamic_cast<HttpReply*>(msg)) {

diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index 2a8c80aec56401fd956f2a51802993c8dd73c673..1c5e5fc403d20b232750aea0292c38fd96eaa5c2 100644 (file)
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -1418,8 +1418,7 @@ ftpReadType(Ftp::Gateway * ftpState)
     debugs(9, 3, HERE << "code=" << code);
 
     if (code == 200) {
-        const SBuf tmp = ftpState->request->url.path();
-        p = path = xstrndup(tmp.rawContent(),tmp.length()+1);
+        p = path = SBufToCstring(ftpState->request->url.path());
 
         if (*p == '/')
             ++p;
@@ -2369,9 +2368,7 @@ ftpTrySlashHack(Ftp::Gateway * ftpState)
     safe_free(ftpState->filepath);
 
     /* Build the new path (urlpath begins with /) */
-    const SBuf tmp = ftpState->request->url.path();
-    path = xstrndup(tmp.rawContent(), tmp.length()+1);
-    path[tmp.length()] = '\0';
+    path = SBufToCstring(ftpState->request->url.path());
 
     rfc1738_unescape(path);
 

diff --git a/src/gopher.cc b/src/gopher.cc
index 27e569ea5438e6c75fdbe31c5f1f83c5328be4ec..9c4ecc043fabc97fc1e76160c185b6811501c1d3 100644 (file)
--- a/src/gopher.cc
+++ b/src/gopher.cc
@@ -273,8 +273,7 @@ gopher_request_parse(const HttpRequest * req, char *type_id, char *request)
     *type_id = typeId[0];
 
     if (request) {
-        SBuf path = tok.remaining().substr(0, MAX_URL-1);
-        xstrncpy(request, path.rawContent(), path.length()+1);
+        SBufToCstring(request, tok.remaining().substr(0, MAX_URL-1));
         /* convert %xx to char */
         rfc1738_unescape(request);
     }

diff --git a/src/http/one/Parser.cc b/src/http/one/Parser.cc
index dca2c768102114a27ae4c26b1fbfb02bd23b4efe..df0b068221e6223a6768e6eb1a58b8706b7a32d5 100644 (file)
--- a/src/http/one/Parser.cc
+++ b/src/http/one/Parser.cc
@@ -134,7 +134,7 @@ Http::One::Parser::getHeaderField(const char *name)
         p.chop(0, sizeof(header)-1);
 
         // return the header field-value
-        xstrncpy(header, p.rawContent(), p.length()+1);
+        SBufToCstring(header, p);
         debugs(25, 5, "returning " << header);
         return header;
     }

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 3aef1437301b94626a58b3406bab21bc3004c9d4..b5467a9242cfe113e5fbb38c3429bfbf44929486 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -1197,7 +1197,7 @@ switchToTunnel(HttpRequest *request, Comm::ConnectionPointer &clientConn, Comm::
     ++statCounter.server.other.requests;
 
     tunnelState = new TunnelStateData;
-    tunnelState->url = xstrndup(url.rawContent(), url.length()+1);
+    tunnelState->url = SBufToCstring(url);
     tunnelState->request = request;
     tunnelState->server.size_ptr = NULL; //Set later if ClientSocketContext is available
 

diff --git a/src/url.cc b/src/url.cc
index 3812e0c9b75847015d2df3f98abdee3fe31ec897..0e8e58c9816f4d0045bce36addf4d7caa7f6542a 100644 (file)
--- a/src/url.cc
+++ b/src/url.cc
@@ -654,7 +654,7 @@ urlMakeAbsolute(const HttpRequest * req, const char *relUrl)
                 // XXX: crops bits in the middle of the combined URL.
                 lastSlashPos = MAX_URL - urllen - 1;
             }
-            xstrncpy(&urlbuf[urllen], path.rawContent(), lastSlashPos);
+            SBufToCstring(&urlbuf[urllen], path.substr(0,lastSlashPos));
             urllen += lastSlashPos;
             if (urllen + 1 < MAX_URL) {
                 xstrncpy(&urlbuf[urllen], relUrl, MAX_URL - urllen - 1);

diff --git a/src/urn.cc b/src/urn.cc
index 4e39a289ebd3c470f041c5e88e2ccec040e5f20f..c9f5c91c9990b3bf0c4be3be2f46955d12b30889 100644 (file)
--- a/src/urn.cc
+++ b/src/urn.cc
@@ -127,17 +127,13 @@ urnFindMinRtt(url_entry * urls, const HttpRequestMethod &, int *rtt_ret)
 char *
 UrnState::getHost(const SBuf &urlpath)
 {
-    char * result;
-    size_t p;
-
     /** FIXME: this appears to be parsing the URL. *very* badly. */
     /*   a proper encapsulated URI/URL type needs to clear this up. */
-    if ((p = urlpath.find(':')) != SBuf::npos) {
-        result = xstrndup(urlpath.rawContent(), (p-1) /*but xstrndup truncates*/+1 );
-    } else {
-        result = xstrndup(urlpath.rawContent(), urlpath.length()+1);
-    }
-    return result;
+    size_t p;
+    if ((p = urlpath.find(':')) != SBuf::npos)
+        return SBufToCstring(urlpath.substr(0, p-1));
+
+    return SBufToCstring(urlpath);
 }
 
 void