Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Alex Rousskov]

On 08/23/2016 09:17 AM, Amos Jeffries wrote:
> On 24/08/2016 12:07 a.m., Eduard Bagdasaryan wrote:
>> 2016-08-21 15:58 GMT+03:00 Amos Jeffries :
>>> To change anything between those markers we have to do a full cache
>>> versioning and up/down-grade compatibility dance.

>> Could you please clarify what versioning problems you are talking
>> about?

> I was referring to that block of members being read and written
> directly to cache files. Either swap.state or the object metadata section.
> I dont think they are referenced individully, but as a raw-pointer to
> the first members address cast to some other identically laid out
> StoreMeta* type (yucky).


Yes, this happens in at least two places:

> bzr grep timestamp src | fgrep \&
...
> src/store_rebuild.cc:memcpy(>timestamp, x.value, 
> STORE_HDR_METASIZE);
> src/store_swapmeta.cc:t = 
> StoreMeta::Factory(STORE_META_STD_LFS,STORE_HDR_METASIZE,>timestamp)


> The point of those comments AFAIK is to make it obvious that kind of
> this is/was being done with them and prevent us doing what you did.

Agreed. Since improving this code is way outside this simple fix scope,
I recommend returning the renamed lastmod data member into that "keep
them together" section, with a comment:

time_t lastmod_; ///< received Last-Modified value or -1; treat as private

Please note that "received Last-Modified value or -1" is just my guess
what lastmod_ is -- use the right description if my guess is wrong.

Renaming lastmod (and temporary making it private) already forced you to
check and, if needed, adjust most of the old code using that data
member. Keeping lastmod_ public is not ideal, but it does not make
things worse and allows you to stay focused. Fixing related problems is
only a good idea when those fixes are simple and do not stray you off
course too much.


HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Coding standards

[Alex Rousskov]

On 08/23/2016 05:48 AM, Adam Majer wrote:

> What are the coding standards for Squid?

Just to add to Kinkie's correct response: We do not have a comprehensive
standard, unfortunately, but you can find a few requirements at

  http://wiki.squid-cache.org/SquidCodingGuidelines

(which should be updated to answer the C++11 question).


HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Incorrect processing of long URIs

[Alex Rousskov]

On 08/23/2016 08:08 AM, Amos Jeffries wrote:
> A followup patch can be done to give skipDelimiter a 'const char* which'
> parameter that takes a description/name for the delimiter to improve
> that debug output.
> 
> so:
>   skipDelimiter(blah, "method")
> 
> produces:
>  invalid request-line: missing method delimiter

Good idea! I would

   s/which/where/
   s/missing  delimiter/missing delimiter /

because the URI field has two associated delimiters (left and right),
and a "where" parameter would be able to detail the missing delimiter
better:

  invalid request-line: missing delimiter after method
  invalid request-line: missing delimiter before "HTTP/1"

Same for the "too many delimiters" error, of course.


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Incorrect processing of long URIs

[Alex Rousskov]

On 08/23/2016 03:26 AM, Eduard Bagdasaryan wrote:
> 2016-08-23 3:08 GMT+03:00 Alex Rousskov:
>> I do not understand why you decided to use maxMethodLength in
>> parseRequestFirstLine(). AFAICT, parseMethodField() already does
>> everything we need: It logs an error message and sets parseStatusCode
>> accordingly.

> Yes, it does partly what we need but it does not parse the delimiter(
> and inform about possible failure). 

... which is unrelated to the maxMethodLength move I questioned above.


> skipDelimiter() does this with message:
> 
>> invalid request-line: missing delimiter
> 
> which does not hint that our case is a 'bad' method 

If the method parses OK but the delimiter is missing, then our case can
be considered to be a case of a "missing delimiter" rather than of a
"bad method". It is all relative, of course: If a request line starts
with a few token characters, then it is impossible to say whether the
method suffix is invalid or the following delimiter -- it takes two to
tango.


> and less informative than:
> 
>> invalid request-line: method exceeds 32-byte limit

How is a "missing delimiter" error is less informative than "method
exceeds 32-byte limit", especially when the method does not actually
exceed that limit? [Rhetorical]


> I followed your sketch and refreshed the patch.

> +debugs(74, ErrorLevel(), "invalid request-line exceeds " <<
> +Config.maxRequestHeaderSize << "-byte limit");

bzr grep "invalid request-line" src

s/request-line/request-line: URI/ for consistency and clarity sake.


> +const CharacterSet  = DelimiterCharacters();

I wonder whether we should make this variable static to avoid repeated
function calls on a performance-sensitive code path. Same for the old
"delimiters" variable left inside parseRequestFirstLine(), of course.


These polishing touches can be done during commit.

+1

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Incorrect processing of long URIs

[Amos Jeffries]

On 23/08/2016 9:26 p.m., Eduard Bagdasaryan wrote:
> 2016-08-23 3:08 GMT+03:00 Alex Rousskov:
> 
>> I do not understand why you decided to use maxMethodLength in
>> parseRequestFirstLine(). AFAICT, parseMethodField() already does
>> everything we need: It logs an error message and sets parseStatusCode
>> accordingly.
> 
> Yes, it does partly what we need but it does not parse the delimiter(
> and inform about possible failure). skipDelimiter() does this with
> message:
> 
>> invalid request-line: missing delimiter
> 
> which does not hint that our case is a 'bad' method and
> less informative than:
> 
>> invalid request-line: method exceeds 32-byte limit
> 
> Probably this does not make much sense, so I followed your
> sketch and refreshed the patch.

A followup patch can be done to give skipDelimiter a 'const char* which'
parameter that takes a description/name for the delimiter to improve
that debug output.

so:
  skipDelimiter(blah, "method")

produces:
 invalid request-line: missing method delimiter


Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Eduard Bagdasaryan]

2016-08-21 15:58 GMT+03:00 Amos Jeffries :

> To change anything between those markers we have to do a full cache
> versioning and up/down-grade compatibility dance.

Could you please clarify what versioning problems you are talking about?  It
seems that StoreEntry's STORE_META_STD fields are not used directly while
storing/restoring metainformation. I found only places where field-by-field
assignment is performed (e.g., InitStoreEntry::operator()), so making
StoreEntry::lastmod 'private' should not do any harm there.  So probably we
could mark with  "START OF ON-DISK STORE_META_STD TLV field" (or 
similar) the

StoreEntry::lastmod, which is now private.

> lastModifiedDelta() returning -1 when the actual delta is any -N value
> is wrong conceptually. A delta should be the actual difference value 
-N < 0 < +N.


Since we need and use only the positive outcome of this method inside
refreshStaleness(), probably rename it to ageWhenCached() (or similar)?
Returning '-1' would mean that it is impossible to calculate the age 
('lastmod'

is absent or greater than timestamp).


Eduard.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Coding standards

[Kinkie]

Hi Adam!
  You're absolutely right; Squid 4.0 is c++11; squid 3.5 is not getting new
features right now.

On Tue, Aug 23, 2016 at 12:48 PM, Adam Majer  wrote:

> Hello,
>
> What are the coding standards for Squid? More specifically, I would like
> to know if C++11 (or later?) is allowed in Squid 4.0. I'm assuming that
> Squid 3.5 is still on the old C++03.
>
> Thanks,
> Adam
>
> ___
> squid-dev mailing list
> squid-dev@lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>



-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] Coding standards

[Adam Majer]

Hello,

What are the coding standards for Squid? More specifically, I would like 
to know if C++11 (or later?) is allowed in Squid 4.0. I'm assuming that 
Squid 3.5 is still on the old C++03.


Thanks,
Adam

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Incorrect processing of long URIs

[Eduard Bagdasaryan]

2016-08-23 3:08 GMT+03:00 Alex Rousskov :

> I do not understand why you decided to use maxMethodLength in
> parseRequestFirstLine(). AFAICT, parseMethodField() already does
> everything we need: It logs an error message and sets parseStatusCode
> accordingly.

Yes, it does partly what we need but it does not parse the delimiter(
and inform about possible failure). skipDelimiter() does this with
message:

> invalid request-line: missing delimiter

which does not hint that our case is a 'bad' method and
less informative than:

> invalid request-line: method exceeds 32-byte limit

Probably this does not make much sense, so I followed your
sketch and refreshed the patch.


Eduard.
MUST respond with 414 (URI Too Long) when request target exceeds limits.

Before the fix, Squid simply closed client connection after receiving a
huge URI (or a huge request-line), violating the RFC 7230 MUST. This
happened because a high-level Must(have buffer space) check in
ConnStateData::clientParseRequests() would throw an exception. Now these
problems are detected inside the low-level RequestParser code, where we
can distinguish huge URIs from huge methods.

=== modified file 'src/http/one/RequestParser.cc'
--- src/http/one/RequestParser.cc	2016-05-20 08:28:33 +
+++ src/http/one/RequestParser.cc	2016-08-23 09:01:24 +
@@ -58,60 +58,65 @@
 while (!buf_.isEmpty() && (buf_[0] == '\n' || (buf_[0] == '\r' && buf_[1] == '\n'))) {
 buf_.consume(1);
 }
 }
 }
 
 /**
  * Attempt to parse the method field out of an HTTP message request-line.
  *
  * Governed by:
  *  RFC 1945 section 5.1
  *  RFC 7230 section 2.6, 3.1 and 3.5
  */
 bool
 Http::One::RequestParser::parseMethodField(Http1::Tokenizer )
 {
 // method field is a sequence of TCHAR.
 // Limit to 32 characters to prevent overly long sequences of non-HTTP
 // being sucked in before mismatch is detected. 32 is itself annoyingly
 // big but there are methods registered by IANA that reach 17 bytes:
 //  http://www.iana.org/assignments/http-methods
 static const size_t maxMethodLength = 32; // TODO: make this configurable?
 
 SBuf methodFound;
 if (!tok.prefix(methodFound, CharacterSet::TCHAR, maxMethodLength)) {
 debugs(33, ErrorLevel(), "invalid request-line: missing or malformed method");
 parseStatusCode = Http::scBadRequest;
 return false;
 }
 method_ = HttpRequestMethod(methodFound);
+
+const CharacterSet  = DelimiterCharacters();
+if (!skipDelimiter(tok.skipAll(delimiters)))
+return false;
+
 return true;
 }
 
 /// the characters which truly are valid within URI
 static const CharacterSet &
 UriValidCharacters()
 {
 /* RFC 3986 section 2:
  * "
  *   A URI is composed from a limited set of characters consisting of
  *   digits, letters, and a few graphic symbols.
  * "
  */
 static const CharacterSet UriChars =
 CharacterSet("URI-Chars","") +
 // RFC 3986 section 2.2 - reserved characters
 CharacterSet("gen-delims", ":/?#[]@") +
 CharacterSet("sub-delims", "!$&'()*+,;=") +
 // RFC 3986 section 2.3 - unreserved characters
 CharacterSet::ALPHA +
 CharacterSet::DIGIT +
 CharacterSet("unreserved", "-._~") +
 // RFC 3986 section 2.1 - percent encoding "%" HEXDIG
 CharacterSet("pct-encoded", "%") +
 CharacterSet::HEXDIG;
 
 return UriChars;
 }
 
 /// characters which Squid will accept in the HTTP request-target (URI)
@@ -243,73 +248,83 @@
 return false;
 }
 }
 return true;
 }
 
 /**
  * Attempt to parse the first line of a new request message.
  *
  * Governed by:
  *  RFC 1945 section 5.1
  *  RFC 7230 section 2.6, 3.1 and 3.5
  *
  * \retval -1  an error occurred. parseStatusCode indicates HTTP status result.
  * \retval  1  successful parse. member fields contain the request-line items
  * \retval  0  more data is needed to complete the parse
  */
 int
 Http::One::RequestParser::parseRequestFirstLine()
 {
 debugs(74, 5, "parsing possible request: buf.length=" << buf_.length());
 debugs(74, DBG_DATA, buf_);
 
 SBuf line;
 
 // Earlier, skipGarbageLines() took care of any leading LFs (if allowed).
 // Now, the request line has to end at the first LF.
 static const CharacterSet lineChars = CharacterSet::LF.complement("notLF");
 ::Parser::Tokenizer lineTok(buf_);
 if (!lineTok.prefix(line, lineChars) || !lineTok.skip('\n')) {
+if (buf_.length() >= Config.maxRequestHeaderSize) {
+/* who should we blame for our failure to parse this line? */
+
+Http1::Tokenizer methodTok(buf_);
+if (!parseMethodField(methodTok))
+return -1; // blame a bad method (or its delimiter)
+
+// assume it is the URI
+debugs(74, ErrorLevel(), "invalid request-line exceeds " <<
+