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

Fri Sep 9 17:21:30 UTC 2016

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:
>>> 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?

There is a "delete this" inside constructor, but it is never called.
The Coverity just does not know that it is not called.

I am not sure if it is problem or not. Someone can argue that even if it 
is not called may confuse developers in the future and make them to add 
a  "delete this" inside the GetRidOfRunner which will be called.

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

Yep.
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.

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

yep.

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

OK, I will try to prepare a patch on this base.

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