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.
