[squid-dev] New Defects reported by Coverity Scan for Squid after IndependentRunner

[Amos Jeffries]

On 10/09/2016 7:26 a.m., Alex Rousskov wrote:
> On 09/09/2016 11:21 AM, Christos Tsantilas wrote:
>> On 09/09/2016 07:00 PM, Alex Rousskov wrote:
>>> On 09/09/2016 07:34 AM, Christos Tsantilas wrote:
>>>> On 09/09/2016 02:21 PM, Amos Jeffries wrote:
>>>>> Also the IndependentRunner::registerRunner() method is not used
>>>>> anywhere. Was it supposed to be called by the child classes ?
>>>>>  (IdleConnList and ConnStateData)
> 
>>>> Well, the registerRunner() method should be used instead of
>>>> RegisterRunner for ConnStateData and IDleConnList. But this change
>>>> somewhere lost while I was playing with the patches.
> 
>>> [rant] Which means our testing was inadequate again. Accidents happen,
>>> but such a major bug should not have went unnoticed if proper testing
>>> was in place.[/rant]
> 
>> But it is not exactly bug. The registereRunner() method just calls
>> "RegisterRunner(this)", it does not do anything else.
>> Just we did not replace RegisterRunner(this); with the
>> IndependedRunner::registerRunner call.
> 
> Ah, this is minor, and no functionality testing could expose this. I
> incorrectly thought that we are not registering independent runners at all.
> 

Well it is rare-ish, but I think it can happen with ConnStateData if a
connection is accepted after shutdown starts during the grace period
witing for client connections to finish up.


> BTW, you can probably move the sketched RegisterRunnerUnlessShutdown()
> function into the IndependedRunner::registerRunner() itself and also add
> a Must(not an independent runner) check to the sketched
> RegisterOrGetRidOfRunner().

What I was thinking was to have this:

 IndependedRunner::registerRunner() {
    Must(RegisterRunner(this));
 }

constructors are allowed to throw, so if it is not caught by the
Call/callback handler from accept() thats a separate issue to fix.

Amos