Re: Segmentation fault in FwdState::serverClosed

On 05/08/2014 09:35 PM, squid3_at_treenet.co.nz wrote:
> I think moving the stats is a good idea.

OK, I will work on that.

> Ive not yet checked your commit but at all,9 debug i also hit the debugs
> array dereference before noteUses

I now added protection for the debugs() call as well (r13406).

Thank you,

Alex.

> ------------------------------------------------------------------------
> From : Alex Rousskov
> To : squid-dev_at_squid-cache.org;
> Subject : Re: Segmentation fault in FwdState::serverClosed
>
>
> On 05/08/2014 11:49 AM, Amos Jeffries wrote: >>> Something is still
> calling serverConn->close() directly In your particular trace,
> HttpStateData::continueAfterParsingHeader() does that, but there are
> many other methods doing the same thing. AFAICT, the idea is that the
> close handlers will clean up during connection closure, including the
> FwdState close handler. While the whole FwdState/Server integration is
> an ugly mess, that close() calling code in ServerStateData and its kids
> is not at fault here. > instead of via the FwdState::closeServer()
> method. I am guessing you are thinking about
> FwdState::closeServerConnection(). It is a private method encapsulating
> repeated code internal to FwdState. It was not meant to be, cannot, and
> should not be used outside FwdState itself "as is". >> Do you know how a
> connection close callback (fwdServerClosedWrapper) can >> be called with
> fd set to -1? It feels like there is a shared Connection >> object that
> two different jobs are updating/closing independently, >> stepping on
> each other's toes. Actually, I bet CommCloseCbParams.fd is just never
> set. commCallCloseHandlers() is supposed to set it, but it does not.
> This is a bug, but I suspect most close handlers do not care about the
> descriptor value so they continue to work fine. r13388 (standy
> connections) was not that lucky. I added the following check to update
> the pconn stats for the closing connection: > if
> (serverConnection()->fd == fd) // should be, but not critical to assert
>> fwdPconnPool->noteUses(fd_table[fd].pconn.uses); That new
> check does not work as intended because both fds are -1 in this case:
> the first one is reset by http.cc code that shares Connection object
> with FwdState and the second is never set by Comm. The above noteUses()
> call was moved from Comm (where it did not belong in the first place and
> caused other troubles after we added non-static pconn pools). Here is
> that old Comm code calling noteUses() for static pconn pools: >
> commCallCloseHandlers(fd); > > - if (F->pconn.uses &&
> F->pconn.pool) > - F->pconn.pool->noteUses(F->pconn.uses); I have
> committed a short-term fix for this bug (r13404). Here is updated
> FwdState::serverClosed(): > - if (serverConnection()->fd == fd) //
> should be, but not critical to assert > + if (fd >= 0 &&
> serverConnection()->fd == fd) // XXX: fd is often -1 here >
> fwdPconnPool->noteUses(fd_table[fd].pconn.uses); This change
> should prevent serverClosed() segfaults, but it is wrong. For the
> correct solution, I am considering moving pconn.uses from fd_table to
> Connection. What do you think? Thank you, Alex.
>
Received on Fri May 09 2014 - 04:20:40 MDT