Helpers parse response bug

[Tsantilas Christos]

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



=== modified file 'src/helper.cc'
--- src/helper.cc	2012-11-24 14:30:02 +0000
+++ src/helper.cc	2012-12-04 14:17:59 +0000
@@ -866,42 +866,42 @@
         -- srv->stats.pending;
         ++ srv->stats.replies;
 
         ++ hlp->stats.replies;
 
         srv->answer_time = current_time;
 
         srv->dispatch_time = r->dispatch_time;
 
         hlp->stats.avg_svc_time =
             Math::intAverage(hlp->stats.avg_svc_time,
                              tvSubMsec(r->dispatch_time, current_time),
                              hlp->stats.replies, REDIRECT_AV_FACTOR);
 
         helperRequestFree(r);
     } else {
         debugs(84, DBG_IMPORTANT, "helperHandleRead: unexpected reply on channel " <<
                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);
+    srv->roffset -= (msg_end - srv->rbuf + 1);
+    memmove(srv->rbuf, msg_end, srv->roffset);
 
     if (!srv->flags.shutdown) {
         helperKickQueue(hlp);
     } else if (!srv->flags.closing && !srv->stats.pending) {
         srv->flags.closing=1;
         srv->writePipe->close();
         return;
     }
 }
 
 static void
 helperHandleRead(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm_err_t flag, int xerrno, void *data)
 {
     char *t = NULL;
     helper_server *srv = (helper_server *)data;
     helper *hlp = srv->parent;
     assert(cbdataReferenceValid(data));
 
     /* Bail out early on COMM_ERR_CLOSING - close handlers will tidy up for us */
 


=== modified file 'src/helper.cc'
--- src/helper.cc	2012-11-24 14:30:02 +0000
+++ src/helper.cc	2012-12-04 14:09:50 +0000
@@ -915,43 +915,44 @@
 
     if (flag != COMM_OK || len == 0) {
         srv->closePipesSafely();
         return;
     }
 
     srv->roffset += len;
     srv->rbuf[srv->roffset] = '\0';
     debugs(84, 9, "helperHandleRead: '" << srv->rbuf << "'");
 
     if (!srv->stats.pending) {
         /* someone spoke without being spoken to */
         debugs(84, DBG_IMPORTANT, "helperHandleRead: unexpected read from " <<
                hlp->id_name << " #" << srv->index + 1 << ", " << (int)len <<
                " bytes '" << srv->rbuf << "'");
 
         srv->roffset = 0;
         srv->rbuf[0] = '\0';
     }
 
-    while ((t = strchr(srv->rbuf, hlp->eom))) {
+    char *msg = srv->rbuf;
+    while(*msg == '\0' && (msg - srv->rbuf) < srv->roffset) msg++;
+    while ((t = strchr(msg, hlp->eom))) {
         /* end of reply found */
-        char *msg = srv->rbuf;
         int i = 0;
         debugs(84, 3, "helperHandleRead: end of reply found");
 
         if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n')
             t[-1] = '\0';
 
         *t = '\0';
 
         if (hlp->childs.concurrency) {
             i = strtol(msg, &msg, 10);
 
             while (*msg && xisspace(*msg))
                 ++msg;
         }
 
         helperReturnBuffer(i, srv, hlp, msg, t);
         // only skip off the \0 _after_ passing its location to helperReturnBuffer
         ++t;
     }