From: Amos Jeffries Date: Wed, 29 Jul 2015 00:41:57 +0000 (-0700) Subject: Add temporary SBufToCstring() helper functions for SBuf transition X-Git-Tag: merge-candidate-3-v1~24^2~4 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3f0e38d60af2b79103a5d7ff8f93943f4ee2f7e0;p=thirdparty%2Fsquid.git 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 6f39f4bd17..31c51cdaf0 100644 --- 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(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 5a83c8cfb3..31213741d3 100644 --- a/src/acl/Url.cc +++ b/src/acl/Url.cc @@ -19,8 +19,7 @@ int ACLUrlStrategy::match (ACLData * &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 513500467b..75b9962458 100644 --- a/src/acl/UrlPath.cc +++ b/src/acl/UrlPath.cc @@ -21,8 +21,7 @@ ACLUrlPathStrategy::match (ACLData * &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 87c7ad31a1..5753c3e43b 100644 --- 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 866f0fc96d..cdec22e5a9 100644 --- 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(msg)) { diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc index 2a8c80aec5..1c5e5fc403 100644 --- 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 27e569ea54..9c4ecc043f 100644 --- 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 dca2c76810..df0b068221 100644 --- 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 3aef143730..b5467a9242 100644 --- 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 3812e0c9b7..0e8e58c981 100644 --- 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 4e39a289eb..c9f5c91c99 100644 --- 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