Re: [MERGE] Address Alex and Amos' comments.
On Wed, 2008-09-03 at 14:44 +1000, Benno Rice wrote: > Address Alex and Amos' comments. > > - Use bool instead of int for urlIsRelative. > - Document what leads to a NULL return in urlMakeAbsolute and mention the > responsibility of the caller to free the result in the non-NULL case. > - Declare variables closer to where they're used. > - Fix indentation. I have no more serious objections but cannot vote "approve" since I did not review the internals of urlMakeAbsolute. Is that a valid reason to bb:abstain? Thank you for your patience and especially the continued stream of improving patches. I am looking forward to seeing this committed. Alex.
Re: [MERGE] Further cleanup of urlAbsolute and friends.
On Wed, 2008-09-03 at 16:30 +1200, Amos Jeffries wrote: > >> +char *urlbuf; > >> +const char *path, *last_slash; > >> +size_t urllen, pathlen; > > > > Declaring variables as needed rather than in advance makes code > > maintenance easier and might help the compiler to optimize. Not a big > > deal though. > > But it does needlessly break the oldest-possible-compiler policy. PoV #1: Compilers that are so old that they cannot support as-needed variables are too old to work correctly for an increasing number of other reasons and, hence, are most likely irrelevant. Pov #2: I doubt the questionable no-as-needed-vars policy has ever existed, but even if it has, it has been broken so many times since, that it can be considered obsolete. I mean, do you really expect anybody to go through the code and make it worse in many places just to support some outdated compiler that is going to break on something else anyway? Cheers, Alex.
Re: [MERGE] Address Alex and Amos' comments.
Bundle Buggy has detected this merge request. For details, see: http://bundlebuggy.aaronbentley.com/project/squid/request/%3C200809030444.m834iIKI048580%40harfy.jeamland.net%3E Project: Squid
[MERGE] Address Alex and Amos' comments.
Address Alex and Amos' comments.
- Use bool instead of int for urlIsRelative.
- Document what leads to a NULL return in urlMakeAbsolute and mention the
responsibility of the caller to free the result in the non-NULL case.
- Declare variables closer to where they're used.
- Fix indentation.
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: [EMAIL PROTECTED]
# target_branch: http://www.squid-cache.org/bzr/squid3/trunk/
# testament_sha1: 623f171d6e06626c28aa1dd5e02cf8b18daff6ca
# timestamp: 2008-09-03 14:43:27 +1000
# base_revision_id: [EMAIL PROTECTED]
# ejhnqdep2qm6qz0h
#
# Begin patch
=== modified file 'src/HttpRequestMethod.cc'
--- src/HttpRequestMethod.cc 2008-06-20 04:43:01 +
+++ src/HttpRequestMethod.cc 2008-08-29 00:56:35 +
@@ -243,11 +243,16 @@
case METHOD_PURGE:
return true;
-/* all others */
+/*
+ * RFC 2616 sayeth, in section 13.10, final paragraph:
+ * A cache that passes through requests for methods it does not
+ * understand SHOULD invalidate any entities referred to by the
+ * Request-URI.
+ */
case METHOD_OTHER:
default:
-return false; // be conservative: we do not know some methods specs
+return true;
}
-return false; // not reached
+return true; // not reached, but just in case
}
=== modified file 'src/Server.cc'
--- src/Server.cc 2008-07-22 12:33:41 +
+++ src/Server.cc 2008-09-03 01:00:39 +
@@ -402,11 +402,35 @@
// purges entries that match the value of a given HTTP [response] header
static void
-purgeEntriesByHeader(const char *reqUrl, HttpMsg *rep, http_hdr_type hdr)
+purgeEntriesByHeader(const HttpRequest *req, const char *reqUrl, HttpMsg *rep, http_hdr_type hdr)
{
-if (const char *url = rep->header.getStr(hdr))
-if (sameUrlHosts(reqUrl, url)) // prevent purging DoS, per RFC 2616
-purgeEntriesByUrl(url);
+const char *hdrUrl, *absUrl;
+
+absUrl = NULL;
+hdrUrl = rep->header.getStr(hdr);
+if (hdrUrl == NULL) {
+return;
+}
+
+/*
+ * If the URL is relative, make it absolute so we can find it.
+ * If it's absolute, make sure the host parts match to avoid DOS attacks
+ * as per RFC 2616 13.10.
+ */
+if (urlIsRelative(hdrUrl)) {
+absUrl = urlMakeAbsolute(req, hdrUrl);
+if (absUrl != NULL) {
+hdrUrl = absUrl;
+}
+} else if (!sameUrlHosts(reqUrl, hdrUrl)) {
+return;
+}
+
+purgeEntriesByUrl(hdrUrl);
+
+if (absUrl != NULL) {
+safe_free(absUrl);
+}
}
// some HTTP methods should purge matching cache entries
@@ -425,8 +449,8 @@
const char *reqUrl = urlCanonical(request);
debugs(88, 5, "maybe purging due to " << RequestMethodStr(request->method) << ' ' << reqUrl);
purgeEntriesByUrl(reqUrl);
- purgeEntriesByHeader(reqUrl, theFinalReply, HDR_LOCATION);
- purgeEntriesByHeader(reqUrl, theFinalReply, HDR_CONTENT_LOCATION);
+ purgeEntriesByHeader(request, reqUrl, theFinalReply, HDR_LOCATION);
+ purgeEntriesByHeader(request, reqUrl, theFinalReply, HDR_CONTENT_LOCATION);
}
// called (usually by kids) when we have final (possibly adapted) reply headers
=== modified file 'src/protos.h'
--- src/protos.h 2008-07-17 12:38:06 +
+++ src/protos.h 2008-09-03 04:40:31 +
@@ -638,6 +638,8 @@
SQUIDCEXTERN void urlInitialize(void);
SQUIDCEXTERN HttpRequest *urlParse(const HttpRequestMethod&, char *, HttpRequest *request = NULL);
SQUIDCEXTERN const char *urlCanonical(HttpRequest *);
+SQUIDCEXTERN bool urlIsRelative(const char *);
+SQUIDCEXTERN char *urlMakeAbsolute(const HttpRequest *, const char *);
SQUIDCEXTERN char *urlRInternal(const char *host, u_short port, const char *dir, const char *name);
SQUIDCEXTERN char *urlInternal(const char *dir, const char *name);
SQUIDCEXTERN int matchDomainName(const char *host, const char *domain);
=== modified file 'src/url.cc'
--- src/url.cc 2008-04-10 11:29:39 +
+++ src/url.cc 2008-09-03 04:42:27 +
@@ -533,6 +533,105 @@
}
/*
+ * Test if a URL is relative.
+ *
+ * RFC 2396, Section 5 (Page 17) implies that in a relative URL, a '/' will
+ * appear before a ':'.
+ */
+bool
+urlIsRelative(const char *url)
+{
+const char *p;
+
+if (url == NULL) {
+return (false);
+}
+if (*url == '\0') {
+return (false);
+}
+
+for (p = url; *p != '\0' && *p != ':' && *p != '/'; p++);
+
+if (*p == ':') {
+return (false);
+}
+return (true);
+}
+
+/*
+ * Convert a relative URL to an absolute URL using the context of a given
+ * request.
+ *
+ * It is assumed that you have already ensured that the URL is relative.
+ *
+ * If NULL is returned it is an indication that the method in use in the
+ * request does not distinguish between relative and absolute and you should
+ * use the url unchanged.
+ *
+ * If non-NULL is returned, it is up to the caller to free the res
Re: [MERGE] Further cleanup of urlAbsolute and friends.
> On Wed, 2008-09-03 at 11:04 +1000, Benno Rice wrote:
>> Ok, hopefully this meets with everybody's approval. Changes in this
>> version:
>>
>> - Split urlAbsolute into urlIsRelative and urlMakeAbsolute.
>> - Make urlIsRelative compliant with the RFC as to what defines a
>> relative URL.
>> - Use a malloc'ed buffer instead of a stack-allocated array and xstrdup
>> in
>> urlMakeAbsolute.
>> - Rework purgeEntriesByHeader to be a little easier on the eyes.
>
> Thank you for fixing and polishing the code! I only found one serious
> problem related to "const char *" return type of urlMakeAbsolute. Please
> feel free to ignore other comments.
>
>> int
>> urlIsRelative(const char *url)
>
> The above should return bool and use true/false constants as appropriate.
>
>
>> * It is assumed that you have already ensured that the URL is relative.
>> * If NULL is returned, you should use the original URL unchanged.
>
> This does not document what a NULL return value means (only what we
> should do with it, which is calling context dependent). The code seems
> to imply that a NULL return value is a special CONNECT method case.
> Perhaps that case should be handled by urlIsRelative (you will need to
> add the req argument to it)? After all, the CONNECT Request URI is
> "absolute" by the CONNECT method definition.
>
> Also, it is not documented whether/how the returned value should be
> freed.
Um, thats a big one.
>
>
>> const char *
>> +urlMakeAbsolute(const HttpRequest * req, const char *relUrl)
>
> If this is returning "const char*", we should not free (or modify) the
> result. Did you mean to use "char *" as the return value?
>
> BTW, safe_free (i.e., xxfree) prototype misleads that you can free a
> constant. That was probably inherited from Squid2/C, with an ugly
> const-removing cast inside the xxfree implementation.
>
>> +}
>> +
>> + if (req->port != urlDefaultPort(req->protocol)) {
>
> It could be my mailer, but the indentation above seems wrong.
>
>
>> +char *urlbuf;
>> +const char *path, *last_slash;
>> +size_t urllen, pathlen;
>
> Declaring variables as needed rather than in advance makes code
> maintenance easier and might help the compiler to optimize. Not a big
> deal though.
But it does needlessly break the oldest-possible-compiler policy.
Amos
Re: [MERGE] Further cleanup of urlAbsolute and friends.
On Wed, 2008-09-03 at 11:04 +1000, Benno Rice wrote:
> Ok, hopefully this meets with everybody's approval. Changes in this version:
>
> - Split urlAbsolute into urlIsRelative and urlMakeAbsolute.
> - Make urlIsRelative compliant with the RFC as to what defines a relative URL.
> - Use a malloc'ed buffer instead of a stack-allocated array and xstrdup in
> urlMakeAbsolute.
> - Rework purgeEntriesByHeader to be a little easier on the eyes.
Thank you for fixing and polishing the code! I only found one serious
problem related to "const char *" return type of urlMakeAbsolute. Please
feel free to ignore other comments.
> int
> urlIsRelative(const char *url)
The above should return bool and use true/false constants as appropriate.
> * It is assumed that you have already ensured that the URL is relative.
> * If NULL is returned, you should use the original URL unchanged.
This does not document what a NULL return value means (only what we
should do with it, which is calling context dependent). The code seems
to imply that a NULL return value is a special CONNECT method case.
Perhaps that case should be handled by urlIsRelative (you will need to
add the req argument to it)? After all, the CONNECT Request URI is
"absolute" by the CONNECT method definition.
Also, it is not documented whether/how the returned value should be
freed.
> const char *
> +urlMakeAbsolute(const HttpRequest * req, const char *relUrl)
If this is returning "const char*", we should not free (or modify) the
result. Did you mean to use "char *" as the return value?
BTW, safe_free (i.e., xxfree) prototype misleads that you can free a
constant. That was probably inherited from Squid2/C, with an ugly
const-removing cast inside the xxfree implementation.
> +}
> +
> + if (req->port != urlDefaultPort(req->protocol)) {
It could be my mailer, but the indentation above seems wrong.
> +char *urlbuf;
> +const char *path, *last_slash;
> +size_t urllen, pathlen;
Declaring variables as needed rather than in advance makes code
maintenance easier and might help the compiler to optimize. Not a big
deal though.
HTH,
Alex.
P.S. I have not verified the correctness of urlMakeAbsolute internals.
Re: [squid-users] state of gzip/transfer-encoding?
2008/9/3 Amos Jeffries <[EMAIL PROTECTED]>: > The existing experimental patch for 3.0.pre2 adds a ClientStreams handler > for de/encoding as needed. Right. > It's really only a matter of caching things properly (ETag from server of > squid-generated), and fiddling the headers slightly as they transit Squid > in both directions. Should not matter which HTTP/1.x version is used at > the header level, since both can handle compressed data objects. > > 3.1 already does TE decoding. But we can't do the TE encoding until 1.1, > only Content-Type re-coding. So its not specifically TE, its just content-encoded fiddling as appropriate? OK, that makes much more sense. Adrian
Re: [squid-users] state of gzip/transfer-encoding?
> So how much is needed exactly to support this when we currently don't > support HTTP/1.1? > The existing experimental patch for 3.0.pre2 adds a ClientStreams handler for de/encoding as needed. It's really only a matter of caching things properly (ETag from server of squid-generated), and fiddling the headers slightly as they transit Squid in both directions. Should not matter which HTTP/1.x version is used at the header level, since both can handle compressed data objects. 3.1 already does TE decoding. But we can't do the TE encoding until 1.1, only Content-Type re-coding. Amos > > Adrian > > 2008/9/2 Amos Jeffries <[EMAIL PROTECTED]>: >>> Chris Woodfield wrote: Squid does not do transfer encoding of objects on its own; >>> >>> >>> Just be curious, does Squid have the plan to develop the feature of >>> objects compression like Apache's mod_deflate? Thanks. >>> >> >> Yes. As an eCAP module. It should be out shortly after eCAP support is >> stabilized. >> >> Amos >> >> >
[MERGE] Further cleanup of urlAbsolute and friends.
Ok, hopefully this meets with everybody's approval. Changes in this version:
- Split urlAbsolute into urlIsRelative and urlMakeAbsolute.
- Make urlIsRelative compliant with the RFC as to what defines a relative URL.
- Use a malloc'ed buffer instead of a stack-allocated array and xstrdup in
urlMakeAbsolute.
- Rework purgeEntriesByHeader to be a little easier on the eyes.
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: [EMAIL PROTECTED]
# target_branch: file:///home/benno/squid-work/squid3-repo/trunk/
# testament_sha1: bc594f5388639662dda35328f972e5eb73bdc3d4
# timestamp: 2008-09-03 11:03:59 +1000
# base_revision_id: [EMAIL PROTECTED]
# ejhnqdep2qm6qz0h
#
# Begin patch
=== modified file 'src/HttpRequestMethod.cc'
--- src/HttpRequestMethod.cc 2008-06-20 04:43:01 +
+++ src/HttpRequestMethod.cc 2008-08-29 00:56:35 +
@@ -243,11 +243,16 @@
case METHOD_PURGE:
return true;
-/* all others */
+/*
+ * RFC 2616 sayeth, in section 13.10, final paragraph:
+ * A cache that passes through requests for methods it does not
+ * understand SHOULD invalidate any entities referred to by the
+ * Request-URI.
+ */
case METHOD_OTHER:
default:
-return false; // be conservative: we do not know some methods specs
+return true;
}
-return false; // not reached
+return true; // not reached, but just in case
}
=== modified file 'src/Server.cc'
--- src/Server.cc 2008-07-22 12:33:41 +
+++ src/Server.cc 2008-09-03 01:00:39 +
@@ -402,11 +402,35 @@
// purges entries that match the value of a given HTTP [response] header
static void
-purgeEntriesByHeader(const char *reqUrl, HttpMsg *rep, http_hdr_type hdr)
+purgeEntriesByHeader(const HttpRequest *req, const char *reqUrl, HttpMsg *rep, http_hdr_type hdr)
{
-if (const char *url = rep->header.getStr(hdr))
-if (sameUrlHosts(reqUrl, url)) // prevent purging DoS, per RFC 2616
-purgeEntriesByUrl(url);
+const char *hdrUrl, *absUrl;
+
+absUrl = NULL;
+hdrUrl = rep->header.getStr(hdr);
+if (hdrUrl == NULL) {
+return;
+}
+
+/*
+ * If the URL is relative, make it absolute so we can find it.
+ * If it's absolute, make sure the host parts match to avoid DOS attacks
+ * as per RFC 2616 13.10.
+ */
+if (urlIsRelative(hdrUrl)) {
+absUrl = urlMakeAbsolute(req, hdrUrl);
+if (absUrl != NULL) {
+hdrUrl = absUrl;
+}
+} else if (!sameUrlHosts(reqUrl, hdrUrl)) {
+return;
+}
+
+purgeEntriesByUrl(hdrUrl);
+
+if (absUrl != NULL) {
+safe_free(absUrl);
+}
}
// some HTTP methods should purge matching cache entries
@@ -425,8 +449,8 @@
const char *reqUrl = urlCanonical(request);
debugs(88, 5, "maybe purging due to " << RequestMethodStr(request->method) << ' ' << reqUrl);
purgeEntriesByUrl(reqUrl);
- purgeEntriesByHeader(reqUrl, theFinalReply, HDR_LOCATION);
- purgeEntriesByHeader(reqUrl, theFinalReply, HDR_CONTENT_LOCATION);
+ purgeEntriesByHeader(request, reqUrl, theFinalReply, HDR_LOCATION);
+ purgeEntriesByHeader(request, reqUrl, theFinalReply, HDR_CONTENT_LOCATION);
}
// called (usually by kids) when we have final (possibly adapted) reply headers
=== modified file 'src/protos.h'
--- src/protos.h 2008-07-17 12:38:06 +
+++ src/protos.h 2008-09-02 11:27:02 +
@@ -638,6 +638,8 @@
SQUIDCEXTERN void urlInitialize(void);
SQUIDCEXTERN HttpRequest *urlParse(const HttpRequestMethod&, char *, HttpRequest *request = NULL);
SQUIDCEXTERN const char *urlCanonical(HttpRequest *);
+SQUIDCEXTERN int urlIsRelative(const char *);
+SQUIDCEXTERN const char *urlMakeAbsolute(const HttpRequest *, const char *);
SQUIDCEXTERN char *urlRInternal(const char *host, u_short port, const char *dir, const char *name);
SQUIDCEXTERN char *urlInternal(const char *dir, const char *name);
SQUIDCEXTERN int matchDomainName(const char *host, const char *domain);
=== modified file 'src/url.cc'
--- src/url.cc 2008-04-10 11:29:39 +
+++ src/url.cc 2008-09-03 01:00:39 +
@@ -533,6 +533,100 @@
}
/*
+ * Test if a URL is relative.
+ *
+ * RFC 2396, Section 5 (Page 17) implies that in a relative URL, a '/' will
+ * appear before a ':'.
+ */
+int
+urlIsRelative(const char *url)
+{
+const char *p;
+
+if (url == NULL) {
+return (0);
+}
+if (*url == '\0') {
+return (0);
+}
+
+for (p = url; *p != '\0' && *p != ':' && *p != '/'; p++);
+
+if (*p == ':') {
+return (0);
+}
+return (1);
+}
+
+/*
+ * Convert a relative URL to an absolute URL using the context of a given
+ * request.
+ *
+ * It is assumed that you have already ensured that the URL is relative.
+ *
+ * If NULL is returned, you should use the original URL unchanged.
+ */
+const char *
+urlMakeAbsolute(const HttpRequest * req, const char *relUrl)
+{
+char *urlbuf;
+
Re: Introducing myself
On Mon, September 1, 2008 9:20 pm, S L wrote: > Hi, it's restricted via mume-type? If that is, then me did introduced it > already. But it doesn't approved yet. > If don't undertand. Do you have a link to your patch? I want to select whether cache or not an object based on reply mime-type. I think we can do it using cached headers. I'm working on it right nigh, will post results in a few days. regards, Diego -- Diego Woitasen XTECH - Soluciones Linux para empresas (54) 011 5219-0678
squid-dev@squid-cache.org
2008/9/2 Kinkie <[EMAIL PROTECTED]>:
> Read: might be useful for the HTTP parser.
> Stuff like:
>
> KBuf reqhdr = (whatever gets in from comm)
> Kbuf hdrline=reqhdr->nextToken("\n");
> if (hdrline[hdrline.len()-1]=='\r')
>hrline.truncate(hdrline.len()-1); //chop \r off
Ok, so what would the underlying manipulations be?
Adrian
squid-dev@squid-cache.org
On Tue, Sep 2, 2008 at 6:38 AM, Alex Rousskov
<[EMAIL PROTECTED]> wrote:
> On Tue, 2008-09-02 at 15:37 +1200, Amos Jeffries wrote:
>
>> There are two versions of this needed;
>>
>> One: const char ...() const; for read-only access to a char. With tests
>> returning NULL char or other safe abortion for out-of-range requests.
>>
>> The other: char ...(); for write-access, with full tests for string
>> expansion if necessary to store the new data.
>
> you probably meant "char &()" for the second one, but please be extra
> careful there: There are C++ gurus that consider write character access
> in a string via a char reference nearly impossible to implement safely.
It could probably be done, but honestly I'm not planning to implement that.
> My recommendation is to drop character access, especially the writeable
> character access via a reference from any buffer- or string-related
> class.
Write: agreed.
Read: might be useful for the HTTP parser.
Stuff like:
KBuf reqhdr = (whatever gets in from comm)
Kbuf hdrline=reqhdr->nextToken("\n");
if (hdrline[hdrline.len()-1]=='\r')
hrline.truncate(hdrline.len()-1); //chop \r off
Syntax for the last two lines might be a little handier, such as:
if (hdrline.charAt(1,FROM_END)=='\r')
hdrline.chop(1);
--
/kinkie
