Cppcheck: Dereferencing 'pDef' after it is deallocated / released (basic module)

[David Tardon]

Hi,

On Thu, Feb 05, 2015 at 12:24:33PM -0700, julien2412 wrote:
> Hello,
> 
> Cppcheck reported this:
> [basic/source/comp/dim.cxx:1239]: (error) Dereferencing 'pDef' after it is
> deallocated / released
> We have this:
>    1213         pProc = pOld->GetProcDef();
>    1214         if( !pProc )
>    1215         {
>    1216             // Declared as a variable
>    1217             Error( SbERR_BAD_DECLARATION, pDef->GetName() );
>    1218             delete pDef;
>    1219             pProc = NULL;
>    1220             bError_ = true;
>    1221         }
>    1222         // #100027: Multiple declaration -> Error
>    1223         // #112787: Not for setup, REMOVE for 8
>    1224         else if( pProc->IsUsedForProcDecl() )
>    1225         {
>    1226             PropertyMode ePropMode = pDef->getPropertyMode();
>    1227             if( ePropMode == PROPERTY_MODE_NONE || ePropMode ==
> pProc->getPropertyMode() )
>    1228             {
>    1229                 Error( SbERR_PROC_DEFINED, pDef->GetName() );
>    1230                 delete pDef;
>    1231                 pProc = NULL;
>    1232                 bError_ = true;
>    1233             }
>    1234         }
>    1235 
>    1236         if( !bError_ )
>    1237         {
>    1238             pDef->Match( pProc );
>    1239             pProc = pDef;
>    1240         }
> See http://opengrok.libreoffice.org/xref/core/basic/source/comp/dim.cxx#1209
> So it seems a false positive since when pDef is deleted, bError = true and
> we don't enter the if.

Yes, it is a false positive.

> But isn't it weird to have each time
> delete pDef;
> pProc = NULL;
> 
> Shouldn't it be one of these :
> 1)
> delete pProc;
> pProc = NULL;
> 
> 2)
> delete pDef;
> pDef = NULL;
> 
> ?

No, it should not. pDef is the currently processed definition of a
procedure, which has been dynamically allocated by call to ProcDecl. If
there is error, it is not used, so it must be deleted. pProc is the
final one, which is set to a possible already known definition on line
1213. And that would already be deleted at some later time. The only
nitpick is that setting pProc to NULL on line 1219 is unnecessary, as it
is already NULL.

D.