Re: /bzr/squid3/trunk/ r9845: Correct header limit checks. 64KB max is REQUIRED.

Alex Rousskov wrote:
> On 07/24/2009 02:31 PM, Henrik Nordstrom wrote:
>> fre 2009-07-24 klockan 14:24 -0600 skrev Alex Rousskov:
>>>> - if (hdr_len > Config.maxReplyHeaderSize || (hdr_len <= 0 && (size_t)buf->contentSize() > Config.maxReplyHeaderSize)) {
>>>> + if (hdr_len >= Config.maxReplyHeaderSize || (hdr_len <= 0 && (size_t)buf->contentSize() > Config.maxReplyHeaderSize)) {
>
>>> Does the comparison need to be ">="? On the surface, it seems like this
>>> change contradicts expected maxReplyHeaderSize semantics and
>>> documentation because the maximum header size should still be allowed.
>
>> It's because we do not want to go above 64KB-1 at this time.
>
> Should not the maximum allowed Config.maxReplyHeaderSize value be
> changed to (64KB-1) instead, then?

I took Henriks word for it last night on this change. The TODO: notes
and debugs are wrong either way. The bounds check MUST occur before
sanityChecks.

The state we have now is that:
  * the MemBuf is limited to 2 GB!!
  * headersEnd ignores everything except the end of real content so far
or \r\n. returning 0 or N. where N is the position of that final \n in
a zero-based array.
  * Config.maxReplyHeaderSize may be 64KB -1 _including_ final \n

Problem is whether at N=64KB and \n present _at_ 64KB any String header
conversion will drop the final \n during its \0 termination or assert
with grow(N+1) > N ?
   assert(len_ + len < 65536); // otherwise snew.len_ overflows below

First problem case hits with minimal valid request having
   N=strlen(". ")+64KB_url. So the answer there appears to be no. Any
header longer than 64KB will have a total size >64KB.

The (rather limited) sanity checks do ensure that 2 bytes minimum are
taken up with first-line syntax. Leaving <64KB-1 for header Strings and
URL where the problem limits start occurring again.

So to conclude; I think the strictness of the limit here was only
relevant when MemBuf capacity was possibly within range of the max
header limits. We have at least 2 bytes of leeway now.

Do you agree?

>
>> Not sure anyone is going to care or even notice that 1 byte difference.
>
> I do care, and I would not be surprised if the check becomes "wrong"
> (i.e., correct!) again during some future code polishing.

Good point. Might as well polish set correctly now.

The more I understand of this the more polish I'm seeing :(

>
> At the very least there should be a comment there saying that this has
> to be that way, but I think enforcing a lower limit on the
> Config.maxReplyHeaderSize setting is the right solution.

I can think of situations (REST filled stuff) where a 14 byte first-line
is the entire headers. I would not put it past the REST people to
enforce stuff like that on some of their reverse-proxies.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16
   Current Beta Squid 3.1.0.10 or 3.1.0.11