[PATCH] Re: Helpers parse response bug

On 05.12.2012 03:24, Tsantilas Christos wrote:
> Hi all,
>
> I think I found a bug in the current helpers response parsing code:
> One
> byte remains in helpers io buffer after parsing the response. This is
> will cause problems when the next response from helper will enter
> squid.
>
> The bug exist in helperHandleRead and helperReturnBuffer functions
> exist
> in src/helper.cc file.
> After the srv->rbuf parsed, the srv->rbuf still contains on byte (a
> '\0' char) and the srv->roffset is 1.
>
> I am posting a patch which fixes the problem.
> Also a second patch (helpers-fix-alternate.patch) to help you
> understand
> the problem.
>
> Regards,
> Christos

Aha! thank you.

There are another slightly related bug here as well which I think we
should clean out immediately. Attached patch fixes both of the below
(1a,b) issues.

  1a) a layering issue between helperHandleRead() and
helperReturnBuffer(). helperReturnBuffer is only dealing with the
message sub-string, but has been made to memmove() the buffer contents
which means it has to become aware of these problematic terminal \0
octet(s). However helperHandleRead() is where the termination is being
done and the buffer information is all being handled.
  -> need to do the memmove() in helperHandleRead()

  1b) helpers which return \r\n are still passed the location of the \n
as endpoint to workaround (1a) even though the \r is also replaced with
\0 and shortens the msg portion by one octet. This affects the
HelperReply parser length checks on responses without kv-pair.
   -> need to pass the first of the termination \0 octets not the last
to helperReturnBuffer(). This is made possible after fixing (1a).

Also,
  2) helperStatefulHandleRead() is not shuffling the buffer remainders
forward for future handling, but assuming only one response line is sent
per read and dropping anything else.
  -> we need to do this shuffling. Documented but omitted from this
patch, since it is not causing broken behaviour right now and I still
have to track down the read() handlers and large response handling that
needs to be checked with that change.

Amos