Re: [PATCH v1] unit-tests for request first-line parser.

On 08/29/2010 12:43 AM, Amos Jeffries wrote:
> One of our users uncovered a nasty bug in 3.1 today. Squid hangs on some
> simple requests.

FWIW, it is not clear to me what is so nasty about this bug: Squid waits
for more input. So what? A benign malformed request will eventually
timeout (if not, that is a true nasty bug!). A malicious request will be
constructed so that the beginning is valid anyway.

> On investigating I found that an update to make it return errors had
> used the wrong result code in a few places. Causing it to loop trying to
> read more data and complete the first line which was already complete.

* There are also many wrong comments in that function, including the
misleading TODO in the description. Perhaps out of scope for this bug
fix, but would be nice to polish now or later.

* FooInit() is a C version of a Foo class constructor. I would recommend
converting HttpParserInit() to a constructor since you are polishing
this code anyway, but if you leave init() in place, please call clear()
_after_ performing any initializations in init(). This approach makes a
future conversion much easier.

* From caller point of view, clear() should return the object to the
just-created object state. For example, some internal buffers, if any,
may be left larger than they were initially but any public state should
be restored. "state = 0;" in clear() violates this rule because init()
sets state to 1. This means that either the proposed clear() is buggy or
it should not be called clear(). More on clear() below.

* s/reset all fields to empty values./reset to initial state/

> The parser function also has no unit tests to verify correct operation.
> Included in this patch is a draft outline for some unit-tests.
>
> If anyone has suggestions or knowledge of other input cases please speak
> up; good, bad AND ugly examples wanted.

I disagree that such unit test cases are a good solution to the problem,
but it is difficult for me to object to adding them.

* It is sad that test cases prompted you to make the Parser class more
complex than required for the main code, adding the clear() method. Such
addition is wrong for testing (because the core code is not using
clear!) and bad for the main code itself (because the Parser class
becomes more complex). Please consider rewriting tests using
init()/constructor alone.

Thank you,

Alex.
Received on Sun Aug 29 2010 - 18:07:11 MDT