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

On Sun, 29 Aug 2010 12:07:02 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> 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.

Yes looks like detritus from several rounds of prior changes.
My plan is adding unit-tests to find out what bugs are lying dormant and
ensure changes don't break anything. Success so far, but more ideas for
actual things to test would be good.

Then submitting the bug fix + unit-tests to all 3.x releases.

ETA wednesday.

Then polishing HttpParser to a trickle-parser which can also be reset at
will and used by the current non-trickling code. Reducing the global
functions into methods. Collapsing sanity checks inline with the parse loop
and generating HTTP status codes as it goes. Using StringNG SBuf for safer
buffer pointers.

And finally work over the current parse loops outside of HttpParser to do
much faster trickle-parsing with one HttpParser object per request which
transfers its results on success into an HttpRequest or HttpReply

ETA sometime 2011.

>
> * 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.
>

Um, no.

 init() sets it to a state where data is available for parsing and about
to begin.

 clear() sets it to a state where there is no data available and parsing
is invalid.

HttpParser is (ab)used in the current code by being re-used for multiple
requests and responses *without clearing previous offset memory*. It has no
buffers of its own, it merely points at the callers buffer space and hold
offsets. Also, that callers buffer may at any time be re-allocated. Current
code gets around this by re-init().

Okay, for the basic fix+tests I'll remove clear() again. Leaving it and
your comments to the polish stage.
NOTE: we then cannot test for incorrect setting of supposedly unset
fields.

Amos
Received on Sun Aug 29 2010 - 23:10:03 MDT