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
=== modified file 'src/helper.cc'
--- src/helper.cc 2012-11-24 14:30:02 +0000
+++ src/helper.cc 2012-12-04 23:17:03 +0000
@@ -848,9 +848,6 @@
{
helper_request *r = srv->requests[request_number];
if (r) {
-// TODO: parse the reply into new helper reply object
-// pass that to the callback instead of msg
-
HLPCB *callback = r->callback;
srv->requests[request_number] = NULL;
@@ -883,15 +880,12 @@
request_number << " from " << hlp->id_name << " #" <<
srv->index + 1 <<
" '" << srv->rbuf << "'");
}
- srv->roffset -= (msg_end - srv->rbuf);
- memmove(srv->rbuf, msg_end, srv->roffset + 1);
if (!srv->flags.shutdown) {
helperKickQueue(hlp);
} else if (!srv->flags.closing && !srv->stats.pending) {
srv->flags.closing=1;
srv->writePipe->close();
- return;
}
}
@@ -936,10 +930,16 @@
/* end of reply found */
char *msg = srv->rbuf;
int i = 0;
+ int skip = 1;
debugs(84, 3, "helperHandleRead: end of reply found");
- if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n')
- t[-1] = '\0';
+ if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n') {
+ t = '\0';
+ // rewind to the \r octet which is the real terminal now
+ // and remember that we have to skip forward 2 places now.
+ skip = 2;
+ --t;
+ }
*t = '\0';
@@ -951,8 +951,8 @@
}
helperReturnBuffer(i, srv, hlp, msg, t);
- // only skip off the \0 _after_ passing its location to
helperReturnBuffer
- ++t;
+ srv->roffset -= (t - srv->rbuf) + skip;
+ memmove(srv->rbuf, t, srv->roffset);
}
if (Comm::IsConnOpen(srv->readPipe)) {
@@ -1049,6 +1049,12 @@
t += skip;
srv->flags.busy = 0;
+ /**
+ * BUG: the below assumes that only one response per read() was
received and discards any octets remaining.
+ * Doing this prohibits concurrency support with multiple replies
per read().
+ * TODO: check that read() setup on these buffers pays attention to
roffest!=0
+ * TODO: check that replies bigger than the buffer are discarded and
do not to affect future replies
+ */
srv->roffset = 0;
helperStatefulRequestFree(r);
srv->request = NULL;
