[cfe-commits] r105602 - in /cfe/trunk: lib/Checker/CMakeLists.txt lib/Checker/GRExprEngineExperimentalChecks.cpp lib/Checker/GRExprEngineInternalChecks.h lib/Checker/StackAddrLeakChecker.cpp test/Analysis/stackaddrleak.c

[Ted Kremenek]

On Jun 8, 2010, at 3:00 AM, Zhongxing Xu wrote:

> Author: zhongxingxu
> Date: Tue Jun  8 05:00:00 2010
> New Revision: 105602
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=105602&view=rev
> Log:
> Add a checker check if a global variable holds a local variable's address after
> the function call is left where the local variable is declared.

Awesome Zhongxing.  A few comments:

> 
> +        R = R->getBaseRegion();
> +        if (const VarRegion *VR = dyn_cast<VarRegion>(R)) {
> +          const VarDecl *VD = VR->getDecl();
> +          const DeclContext *DC = VD->getDeclContext();

We can probably be more general and handle all stack allocated memory, including those returned from alloca().  I think checking the memory space of the region as being from the stack should be sufficient.

> +          // Get the function where the variable is declared.
> +          if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(DC)) {
> +            // Check if the function is the function we are leaving.
> +            if (FD == LCtx->getDecl()) {

This will leave out Objective-C methods and blocks (closures).  Don't we have a more general way of checking this by looking at the location context?  (e.g., StackFrameContext).  This is precisely what StackFrameContext was created for.  That way you can handle any stack frame, regardless of what created it (C function, Objective-C method, block, etc).

> +              // The variable is declared in the function scope which we are 
> +              // leaving. Keeping this variable's address in a global variable
> +              // is dangerous.
> +              // FIXME: Currently VarRegion does not carry context information.
> +              // So we cannot tell if the local variable instance is in the
> +              // current stack frame.

I don't think this is true.  I think you can look at the memory space of the VarRegion.  Is that not the case?

Incidentally, instead of making this a separate checker, should this just be merged in with ReturnStackAddressChecker.cpp?  They are both two sides of the same kind of error, and probably could share most of their logic.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100608/cee826f9/attachment.html>