Re: segfault in clientSocketRecipient

On Fri, 2007-07-27 at 14:36 +1000, Robert Collins wrote:
> On Thu, 2007-07-26 at 22:26 -0600, Alex Rousskov wrote:
>
> > > revision 1.593
> > > date: 2002/09/24 10:46:43; author: robertc; state: Exp; lines: +783 -659
> > > Client side refactoring - no functionality changes
> >
> > Does anybody know whether we should resurrect support for the NULL rep
> > case in clientSocketRecipient? Or, at least, what does it mean that rep
> > is NULL there?
>
> If I remember correctly, the refactoring in question was to break out
> the different cases in to different callback handlers. So you would only
> be called in clientSocketRecipient once a reply does in fact exist,
> rather than it being a if/then on every callback.
>
> So it should never be NULL today, unless something is setting it as the
> callback routine in appropriately.

clientSocketRecipient is being called explicitly with a NULL rep pointer
by several clientReplyContext methods. In this particular case, it is
being called at the end of pushStreamData:

void
clientReplyContext::pushStreamData(StoreIOBuffer const &result, char *source)
{
    StoreIOBuffer tempBuffer;
    ...
    assert(result.offset - headers_sz == next()->readBuffer.offset);
    tempBuffer.offset = result.offset - headers_sz;
    tempBuffer.length = result.length;
    ...
    clientStreamCallback((clientStreamNode*)http->client_stream.head->data, http, NULL, tempBuffer);
}

As you can see, the rep parameter is hard-coded to NULL and the call to
clientStreamCallback is also hard-coded. This code has not changed since
you wrote it. The commit message (r1.31) was "Import of fix-ranges
branch" but there are no ranges in the test case causing the segfault.

FWIW, the whole chain of the unfortunate events starts with this
mysterious client-side code that is supposed to create an error response
from scratch:

    debugs(85,3, HERE << "ICAP REQMOD callout failed, responding with error");

    clientStreamNode *node = (clientStreamNode *)client_stream.tail->prev->data;
    clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
    assert(repContext);

    ConnStateData::Pointer c = getConn();
    repContext->setReplyToError(ERR_ICAP_FAILURE, HTTP_INTERNAL_SERVER_ERROR,
        request->method, NULL,
        (c != NULL ? &c->peer.sin_addr : &no_addr), request, NULL,
        (c != NULL && c->auth_user_request ?
            c->auth_user_request : request->auth_user_request));

    node = (clientStreamNode *)client_stream.tail->data;
    clientStreamRead(node, this, node->readBuffer);

I do not understand the node magic in the above code. Can you spot any
problems? Is this the right way to create an error response?

Thank you,

Alex.
Received on Fri Jul 27 2007 - 09:52:36 MDT