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

[Alex Rousskov]

On 09/09/2016 07:34 AM, Christos Tsantilas wrote:
> On 09/09/2016 02:21 PM, Amos Jeffries wrote:
>> These issues are caused by the new RegisterRunner() design using
>> GetRidOfRunner(rr) if shutdown has already begun. That can potentially
>> result in the constructor of a class inheriting from IndependentRunner
>> deleting 'this', then the new'd object being used.


> However the GetRidOfRunner will never delete an IndependedRunner, so
> there is not a real problem here.

To be more precise, there is a Coverity problem here, not a Squid
problem, right?


>> I think what we should be doing is using Must(RegisterRunner(this))
>> instead of just RegisterRunner(this) for children of IndependentRunner
>> so their constructors throw on errors.

That does not sound right: AFAICT, the code logic does not really
require successful registration of [independent] runners so it would be
inappropriate to throw when registration fails. In other words, a failed
independent runner registration does not indicate a bug in the code AFAICT.

IIRC, the current RR API does not guarantee emitting (and the code
should not rely on receiving) any RR events. If we want to make that
guarantee, we would probably have to supply a list of events (that a
specific RR must receive) to the registration function. I do not think
we need it, but please let me know if I missed any good use cases for
that functionality.


>> 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]


> However this is alone I think will not solve the problem.

Just to make sure we are on the same page: Which problem are you trying
to solve? A false USE_AFTER_FREE defect reported by Coverity?


> Maybe we need to re-implement  registerRunner() with something like:
> 
> IndependedRunner::registerRunner() {
>     RegisterRunnerIgnoreOnShutdown();
> }
> 
> The RegisterRunnerIgnoreOnShutdown() is similar to RegisterRunner but it
> will not call GetRidOfRunner().

To avoid code duplication, you will need something like this:

void
RegisterRunnerUnlessShutDown(rr)
{
    if (FindRunners())
        RegisterRunner_(rr);
    // else do nothing past finishShutdown
}

bool
RegisterOrGetRidOfRunner(rr)
{
    if (FindRunners()) {
        RegisterRunner_(rr);
        return true;
    } else {
        GetRidOfRunner(rr);
        return false; // past finishShutdown
    }
}

where RegisterRunner_() is a static void function that never calls
GetRidOfRunner() and throws if it cannot FindRunners().

One advantage of this more verbose/explicit solution is that it
minimizes fears about using dynamic_cast<> on a being-constructed
object. I think the current code is actually OK there, but avoiding that
question is a good thing so I support this change.


Thank you,

Alex.




>> On 9/09/2016 5:45 a.m., scan-admin wrote:
>>>
>>> ** CID 1372673:  Memory - illegal accesses  (USE_AFTER_FREE)
>>> /src/servers/FtpServer.cc: 55 in Ftp::Server::Server(const
>>> RefCount<MasterXaction> &)()
>>>
>>>
>>> ________________________________________________________________________________________________________
>>>
>>> *** CID 1372673:  Memory - illegal accesses  (USE_AFTER_FREE)
>>> /src/servers/FtpServer.cc: 55 in Ftp::Server::Server(const
>>> RefCount<MasterXaction> &)()
>>> 49     static bool CommandHasPathParameter(const SBuf &cmd);
>>> 50     };
>>> 51
>>> 52     Ftp::Server::Server(const MasterXaction::Pointer &xact):
>>> 53         AsyncJob("Ftp::Server"),
>>> 54         ConnStateData(xact),
>>>>>>     CID 1372673:  Memory - illegal accesses  (USE_AFTER_FREE)
>>>>>>     Dereferencing freed pointer "this".
>>> 55         master(new MasterState),
>>> 56         uri(),
>>> 57         host(),
>>> 58         gotEpsvAll(false),
>>> 59         onDataAcceptCall(),
>>> 60         dataListenConn(),
>>>
>>> ** CID 1372672:  Memory - illegal accesses  (USE_AFTER_FREE)
>>> /src/servers/Http1Server.cc: 27 in Http::One::Server::Server(const
>>> RefCount<MasterXaction> &, bool)()
>>>
>>>
>>> ________________________________________________________________________________________________________
>>>
>>> *** CID 1372672:  Memory - illegal accesses  (USE_AFTER_FREE)
>>> /src/servers/Http1Server.cc: 27 in Http::One::Server::Server(const
>>> RefCount<MasterXaction> &, bool)()
>>> 21     #include "servers/Http1Server.h"
>>> 22     #include "SquidConfig.h"
>>> 23     #include "Store.h"
>>> 24
>>> 25     CBDATA_NAMESPACED_CLASS_INIT(Http1, Server);
>>> 26
>>>>>>     CID 1372672:  Memory - illegal accesses  (USE_AFTER_FREE)
>>>>>>     Dereferencing freed pointer "this".
>>> 27     Http::One::Server::Server(const MasterXaction::Pointer &xact,
>>> bool beHttpsServer):
>>> 28         AsyncJob("Http1::Server"),
>>> 29         ConnStateData(xact),
>>> 30         isHttpsServer(beHttpsServer)
>>> 31     {
>>> 32     }
>>>
>>>
>>> ________________________________________________________________________________________________________
>>>
>>> To view the defects in Coverity Scan visit,
>>> https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRbvv5V1jRT-2FFTEh5SouD11PsnhHEJCA6aD7rB3cIxbLXQ-3D-3D_gndHAcXBoX6qDcYycKjMaKhQpd-2BDW-2BORg0izVeF8khSp2-2BSTODpQkV5I-2Fmydok7q79FMgS3x7g7GnwLNQ6LGBoh25NErdySWe-2FmGN-2Byw29L3E76sjeJMeKn74qRS8yQ07x6d-2Ba5gkubs9LPJj8j2O8-2B5-2FVzqEqPeXMnWlnFfh3X252jxFQIppsOaAa8iZzFwCFNfhmHLg1OqwRzwN-2FtsF8AlRD7-2B-2BejzeO-2FJpfrpEFs-3D
>>>
>>>
>>> To manage Coverity Scan email notifications for
>>> "noc at lists.squid-cache.org", click
>>> https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRbVDbis712qZDP-2FA8y06Nq4W76P2yZz75NEA4ckHihJ8hDUYC6WdPXELy5U35hjpH-2Bx0oGMlKYQxYZwu48zd34K0Fjksb1evIPVJe6QGymC0lD6Es5FNSogirJxAlrf7ao-3D_gndHAcXBoX6qDcYycKjMaKhQpd-2BDW-2BORg0izVeF8khSp2-2BSTODpQkV5I-2Fmydok7q79FMgS3x7g7GnwLNQ6LGBmkrenZtrNlpx1-2BVjUi4Qg1xsrieY0Pubzw8nl6tSWWV-2Fs2nSEb4qDzyeDJ9n6WRJBtiwZ74i6RhCXGJPf2SAmsQ-2BNYadNWqGwN4eFNzJPOthrQj3nlFSCY22YNeA1h5L-2BL43yXDsuegZsWQ-2BKWbTKU-3D
>>>