improving detection of uninitialized arguments (the CallAndMessageChecker)

[Jordan Rose]

Hi, Per. Thanks again for working on this. I don't think we can rename the existing core.CallAndMessage, as much as we might want to. Names that aren't in "alpha" are API-ish, and we try not to break them without a strong reason.

We've also been leaving "Checker" off of the fully-qualified names (e.g. CallAndMessageUnInitRefArg instead of CallAndMessageUnInitRefArgChecker).

+  ChecksFilter filter;
+  CallAndMessageChecker()
+  : filter() {}

Since ChecksFilter has a non-trivial constructor, you can just leave out the explicit constructor for CallAndMessageChecker and the right thing will happen.

+                          OwningPtr<BugType> &BT, const ParmVarDecl* ParamDecl) const;

Two LLVM style nitpicks: please attach the * to the variable name, and please wrap to the next line to stay within 80 columns.

+  if (filter.check_CallAndMessageUnInitRefArgChecker) {

Suggestion: factor this whole section out into a helper function.


+    //ParamIsPointer
+    //ParamIsReference
+    if (ParamDecl->getType().getTypePtr()->isPointerType()) {
+      ParamIsPointerToConst = ParamDecl->getType().getTypePtr()
+                                     ->getPointeeType().isConstQualified();
+      Message = "Function call argument is a pointer to uninitialized value";
+    } else if (ParamDecl->getType().getTypePtr()->isReferenceType()) {
+      ParamIsReferenceToConst = ParamDecl->getType().getTypePtr()
+                                     ->getPointeeType().isConstQualified();
+      Message = "Function call argument is an uninitialized value";
+    }

A few things here:
- No need for these comments.
- QualType has an operator->, so you can drop the .getTypePtr()s without any change.
- It's probably very slightly nicer to factor out the common code (checking for const qualification on the pointee). I would do something like this:

if (ParamDecl->getType()->isPointerType())
	Message = "..."
// same for references

if (!Message)
	return;
if (!ParamDecl->getType()->getPointeeType().isConstQualified())
	return;


+            LazyInit_BT("Uninitialized argument value", BT);

This is duplicated below. It's a little unfortunate to have to keep the strings in sync. Maybe we can factor that out somehow? (Or maybe it's better to use different BugTypes. What do you think?)


+            if (argEx)
+              bugreporter::trackNullOrUndefValue(N, argEx, *R);

We definitely need tests for this. Are we actually going to track back to when this pointer first start pointing to an undefined region? If not, what path notes are we showing here? Does this work for both references and pointers? (You can see how to test path notes in test/Analysis/inlining/path-notes.c.)

If things aren't working perfectly here, it probably doesn't block the patch, but we should still know where we are today and what still needs to be done. And we don't want to print something that's straight-up incorrect.

For the test cases, how about some pointers to memory returned by malloc() as well? Also, please add "core" to the set of checkers run in each case...it's a more realistic testing environment to have the core checkers enabled.

Thank you!
Jordan


On Feb 26, 2014, at 5:29 , Per Viberg <Per.Viberg at evidente.se> wrote:

> 
> Hi,
> 
> I've extended the check that warns for uninitialized arguments to:
> - warn for pointer arguments that point to uninitialized local (stack) variables.  
> - warn for function calls with uninitialized arguments, when the corresponding function parameter is a const reference. 
> 
> void doStuff(const int *p);
> void doStuffRef(const int& c);
> 
> void f(void) {
>       int x;
>       doStuff(&x);  // warning
> }
> void g(void) {
>       int x;
>       doStuffRef(x); // warning
>     
> 
> I've also parameterize the CallAndMessageChecker in a similar way as CheckSecuritySyntaxOnly.
> 
> that is:
> // only check if arguments are uninitialized (the old functionality only)
> clang -cc1 -analyze -analyzer-checker=core.callandmessage uninit-const.cpp
> 
> // with extended functionality, i.e. "check if const pointer arguments are uninitialized" (also performs the old check).
> clang -cc1 -analyze -analyzer-checker=alpha.core.CallAndMessageUnInitRefArgChecker uninit-const.cpp
> 
> 
> /Per
> 
> .......................................................................................................................
> Per Viberg Senior Engineer
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> Phone:    +46 (0)8 402 79 00
> Mobile:    +46 (0)70 912 42 52
> E-mail:     Per.Viberg at evidente.se 
> 
> www.evidente.se
> This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.
> <refToUninitConst_rev1.diff>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140303/d7cb1da9/attachment.html>