Re: [Fwd: [squid-users] [PATCH] Squid 3.0.STABLE17 assertion(!eof) fix]

Amos Jeffries schrieb:
>
> How certain are about that?
>
> I've been contemplating something similar. The problem appears to be
> that the eof is already set in Squid sometimes before the real EOF
> occurs on the wire link.
>
> Amos

Hello Amos, thank you for answering. Here are the steps, which finally
lead to the creation of this patch.

Found your "Security Update Advisory SQUID-2009:2".
Built a new package for my Slackware box.
Upgraded from STABLE13 to STABLE17.

cache.log:
2009/07/28 16:46:10| assertion failed: http.cc:738: "!eof"
2009/07/28 16:48:05| assertion failed: http.cc:738: "!eof"
2009/07/28 16:48:14| assertion failed: http.cc:738: "!eof"
2009/07/28 17:37:17| assertion failed: http.cc:738: "!eof"
2009/07/28 18:00:53| assertion failed: http.cc:738: "!eof"
2009/07/28 18:28:29| assertion failed: http.cc:738: "!eof"
2009/07/28 19:01:29| assertion failed: http.cc:738: "!eof"
2009/07/28 19:02:11| assertion failed: http.cc:738: "!eof"
2009/07/29 07:30:40| assertion failed: http.cc:738: "!eof"
2009/07/29 08:05:04| assertion failed: http.cc:738: "!eof"
2009/07/29 08:22:44| assertion failed: http.cc:738: "!eof"
2009/07/29 10:05:56| assertion failed: http.cc:738: "!eof"
2009/07/29 10:12:36| assertion failed: http.cc:738: "!eof"
2009/07/29 10:27:04| assertion failed: http.cc:738: "!eof"
2009/07/29 10:36:59| assertion failed: http.cc:738: "!eof"
2009/07/29 10:39:30| assertion failed: http.cc:738: "!eof"
2009/07/29 11:19:39| assertion failed: http.cc:738: "!eof"
2009/07/29 11:25:03| assertion failed: http.cc:738: "!eof"

Found and applied your "missing bits" patch. But no change!

Ok, looked closer inside your "missing bits" patch.
Realized, that the patch did not address the problem at all.

At that time I actually was very annoyed about the bad quality of
STABLE17. Sorry about that, but I had to say this somewhere.

Ok, tried to take another route to fix the "Security problem".

Build a new STABLE13 modified with the patches b9070.patch, b9074.patch
and b9075.patch. Now I *also* got the assertions in STABLE13!

cache.log:
2009/07/29 14:05:17| assertion failed: http.cc:726: "!eof"
2009/07/29 14:50:16| assertion failed: http.cc:726: "!eof"
2009/07/29 15:12:15| assertion failed: http.cc:726: "!eof"
2009/07/29 15:45:18| assertion failed: http.cc:726: "!eof"
2009/07/29 16:01:27| assertion failed: http.cc:726: "!eof"
2009/07/29 16:35:08| assertion failed: http.cc:726: "!eof"
2009/07/29 16:43:12| assertion failed: http.cc:726: "!eof"
2009/07/29 17:54:11| assertion failed: http.cc:726: "!eof"
2009/07/29 18:08:15| assertion failed: http.cc:726: "!eof"
2009/07/29 18:10:38| assertion failed: http.cc:726: "!eof"
2009/07/30 08:00:44| assertion failed: http.cc:726: "!eof"
2009/07/30 08:02:31| assertion failed: http.cc:726: "!eof"
2009/07/30 08:09:52| assertion failed: http.cc:726: "!eof"
2009/07/30 08:16:36| assertion failed: http.cc:726: "!eof"
2009/07/30 08:45:49| assertion failed: http.cc:726: "!eof"
2009/07/30 08:48:20| assertion failed: http.cc:726: "!eof"
2009/07/30 09:03:41| assertion failed: http.cc:726: "!eof"
2009/07/30 10:03:08| assertion failed: http.cc:726: "!eof"
2009/07/30 10:59:13| assertion failed: http.cc:726: "!eof"
2009/07/30 11:13:29| assertion failed: http.cc:726: "!eof"
2009/07/30 11:14:39| assertion failed: http.cc:726: "!eof"
2009/07/30 11:31:31| assertion failed: http.cc:726: "!eof"
2009/07/30 12:02:04| assertion failed: http.cc:726: "!eof"
2009/07/30 12:26:49| assertion failed: http.cc:726: "!eof"
2009/07/30 14:01:49| assertion failed: http.cc:726: "!eof"

So, I was assured they are caused by changes made in the patches.

<http.cc:724>
Looking inside the code of STABLE17 in processReplyHeader() the
assertion is obviously triggered by (!parsed && error == HTTP_STATUS_NONE).

<HttpMsg.cc:144>
The call of sanityCheckStartLine() is obviously moved in front of the
following check:

  if (hdr_len <= 0) {
   debugs(58, 3, ...);
   if (eof) // iff we have seen the end, this is an error
    *error = HTTP_INVALID_HEADER;
   return false;
  }

A consequence of this change is that (hdr_len <= 0) is now possible in
sanityCheckStartLine().

 From this check I see that eof together with (!parsed && error ==
HTTP_STATUS_NONE) simply is not a valid state.

<HttpReply.cc:444>
Looking at the first check introduced by the security patch:

  if ( buf->contentSize() < (protoPrefix.size() + 4) ) {
   if (hdr_len > 0)
    *error = HTTP_INVALID_HEADER;
   return false;
  }

If hdr_len <= 0 then we come to the the state (!parsed && error ==
HTTP_STATUS_NONE).

In this case eof is very likely to also be true, which is invalid and
therefore the assertion is triggered!

My fix detects the case (!parsed && error == HTTP_STATUS_NONE) and
avoids the illegal situation if eof also happens to be true.

Hope I could explain the working of my fix a little bit :-)

FYI: Since yesterday no Squid warnings and assertions of STABLE17 with
your "missing bits" patch and my fix applied !!!

Martin
Received on Fri Jul 31 2009 - 18:53:58 MDT