[analyzer] NonNullParamChecker and CStringChecker parameter number in checker message

[bruntib]

Summary

There are some functions which can't be given a null pointer as parameter either because it has a nonnull attribute or it is declared to have undefined behavior (e.g. strcmp()). Sometimes it is hard to determine from the checker message which parameter is null at the invocation, so now this information is included in the message.

This commit fixes https://bugs.llvm.org/show_bug.cgi?id=39358

[whisperity]

clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

195–196 ↗

(On Diff #215539)

It seems to be as if now you're able to present which parameter is the [[nonnull]] one. Because of this, the output to the user sounds really bad and unfriendly, at least to me.

How about this:

"null pointer passed to nth parameter, but it's marked 'nonnull'"?
"null pointer passed to nth parameter expecting 'nonnull'"?

Something along these lines.

To a parameter, we're always passing arguments, so saying "as an argument" seems redundant.

And because we have the index always in our hands, we don't need to use the indefinite article.

[Szelethus]

I really like the change, thanks! Let's settle on the diagnostic message then.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

202 ↗

(On Diff #215539)

I think Optional<unsigned> would be nicer. Also, checkNonNull as a function name doesn't scream about why we need an index parameter -- could you rename it to IndexOfArg or something similar?

clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

195–196 ↗

(On Diff #215539)

Agreed.

[NoQ]

Thanks! I'm happy to accept this as soon as other reviewers are happy.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1548 ↗

(On Diff #215539)

You could also pass a description of the parameter (eg., "source" or "destination").

[bruntib]

The checker message has been changed according to the suggestion.
The last parameter of checkNonNull() doesn't require a default or optional value, so it has been fixed.
The parameter has been renamed from Idx to IdxOfArg.

[bruntib]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

202 ↗

(On Diff #215539)

The default value of IdxOfArg comes from my laziness. There were two places where I was not sure what index number should be given, but I added those too. This way no default value or Optional is needed. Omitting this information wouldn't even be reasonable, since there is always an ordinal number of the argument.

1548 ↗

(On Diff #215539)

Could you please give some hint, how to include this in the message? I don't know how to do it concisely.

clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

195–196 ↗

(On Diff #215539)

I used the original message and just extended it with a number. Are you sure that it is a good practice to change a checker message? I'm not against the modification, just a little afraid that somebody may rely on it. It's quite unlikely though, so I changed it :)

[Szelethus]

@whisperity, any objections to this?

[whisperity]

@Szelethus Nope.

[NoQ]

cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

295–297

It sounds like this code remained untested because tests don't match end-of-line.

I noticed it because it produces warnings that don't look like valid English, such as:

"Null pointer argument in call to memory set function 1st parameter"

Patches are welcome :)

[Charusso]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1548 ↗

(On Diff #215539)

I have made this for that purpose:

struct CallContext {
    CallContext(Optional<unsigned> DestinationPos,
                Optional<unsigned> SourcePos = None,
                Optional<unsigned> LengthPos = None)
        : DestinationPos(DestinationPos), SourcePos(SourcePos),
          LengthPos(LengthPos) {};
  
    Optional<unsigned> DestinationPos;
    Optional<unsigned> SourcePos;
    Optional<unsigned> LengthPos;
  };

// char *strcpy(char *dest, const char *src);
{{CDF_MaybeBuiltin, "stpcpy", 2}, {&CStringChecker::evalStpcpy, {0, 1}}},

May it helps.

[NoQ]

cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

295–297

D71321.

cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

public:
   static bool memsetAux(const Expr *DstBuffer, SVal CharE,
                         const Expr *Size, CheckerContext &C,
                         ProgramStateRef &State);
 
   // Re-usable checks
   ProgramStateRef checkNonNull(CheckerContext &C,
                                    ProgramStateRef state,
                                    const Expr *S,
-                                   SVal l) const;
+                                   SVal l,
+                                   unsigned IdxOfArg) const;
   ProgramStateRef CheckLocation(CheckerContext &C,
                                     ProgramStateRef state,
                                     const Expr *S,
                                     SVal l,
                                     const char *message = nullptr) const;
   ProgramStateRef CheckBufferAccess(CheckerContext &C,
                                         ProgramStateRef state,
                                         const Expr *Size,
CStringChecker::assumeZero(CheckerContext &C, ProgramStateRef state, SVal V,
 
   SValBuilder &svalBuilder = C.getSValBuilder();
   DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
   return state->assume(svalBuilder.evalEQ(state, *val, zero));
 }
 
 ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C,
                                             ProgramStateRef state,
-                                            const Expr *S, SVal l) const {
+                                            const Expr *S, SVal l,
+                                            unsigned IdxOfArg) const {
   // If a previous check has failed, propagate the failure.
   if (!state)
     return nullptr;
 
   ProgramStateRef stateNull, stateNonNull;
   std::tie(stateNull, stateNonNull) = assumeZero(C, state, l, S->getType());
 
   if (stateNull && !stateNonNull) {
     if (Filter.CheckCStringNullArg) {
       SmallString<80> buf;
-      llvm::raw_svector_ostream os(buf);
+      llvm::raw_svector_ostream OS(buf);
       assert(CurrentFunctionDescription);
-      os << "Null pointer argument in call to " << CurrentFunctionDescription;
+      OS << "Null pointer argument in call to " << CurrentFunctionDescription
+         << ' ' << IdxOfArg << llvm::getOrdinalSuffix(IdxOfArg)
+         << " parameter";
 
-      emitNullArgBug(C, stateNull, S, os.str());
+      emitNullArgBug(C, stateNull, S, OS.str());
     }
     return nullptr;
   }
 
   // From here on, assume that the value is non-null.
   assert(stateNonNull);
   return stateNonNull;
 }
ProgramStateRef CStringChecker::CheckBufferAccess(CheckerContext &C,
   ASTContext &Ctx = svalBuilder.getContext();
   const LocationContext *LCtx = C.getLocationContext();
 
   QualType sizeTy = Size->getType();
   QualType PtrTy = Ctx.getPointerType(Ctx.CharTy);
 
   // Check that the first buffer is non-null.
   SVal BufVal = C.getSVal(FirstBuf);
-  state = checkNonNull(C, state, FirstBuf, BufVal);
+  state = checkNonNull(C, state, FirstBuf, BufVal, 1);
   if (!state)
     return nullptr;
 
   // If out-of-bounds checking is turned off, skip the rest.
   if (!Filter.CheckCStringOutOfBounds)
     return state;
 
   // Get the access length and make sure it is known.
if (Optional<Loc> BufLoc = BufStart.getAs<Loc>()) {
     // If the buffer isn't large enough, abort.
     if (!state)
       return nullptr;
   }
 
   // If there's a second buffer, check it as well.
   if (SecondBuf) {
     BufVal = state->getSVal(SecondBuf, LCtx);
-    state = checkNonNull(C, state, SecondBuf, BufVal);
+    state = checkNonNull(C, state, SecondBuf, BufVal, 2);
     if (!state)
       return nullptr;
 
     BufStart = svalBuilder.evalCast(BufVal, PtrTy, SecondBuf->getType());
     if (Optional<Loc> BufLoc = BufStart.getAs<Loc>()) {
       const Expr *warningExpr = (WarnAboutSize ? Size : SecondBuf);
 
       SVal BufEnd = svalBuilder.evalBinOpLN(state, BO_Add, *BufLoc,
void CStringChecker::evalCopyCommon(CheckerContext &C,
   }
 
   // If the size can be nonzero, we have to check the other arguments.
   if (stateNonZeroSize) {
     state = stateNonZeroSize;
 
     // Ensure the destination is not null. If it is NULL there will be a
     // NULL pointer dereference.
-    state = checkNonNull(C, state, Dest, destVal);
+    state = checkNonNull(C, state, Dest, destVal, 1);
     if (!state)
       return;
 
     // Get the value of the Src.
     SVal srcVal = state->getSVal(Source, LCtx);
 
     // Ensure the source is not null. If it is NULL there will be a
     // NULL pointer dereference.
-    state = checkNonNull(C, state, Source, srcVal);
+    state = checkNonNull(C, state, Source, srcVal, 2);
     if (!state)
       return;
 
     // Ensure the accesses are valid and that the buffers do not overlap.
     const char * const writeWarning =
       "Memory copy function overflows destination buffer";
     state = CheckBufferAccess(C, state, Size, Dest, Source,
                               writeWarning, /* sourceWarning = */ nullptr);
if (IsStrnlen) {
     // Otherwise, record the assumption that the size is nonzero.
     state = stateNonZeroSize;
   }
 
   // Check that the string argument is non-null.
   const Expr *Arg = CE->getArg(0);
   SVal ArgVal = state->getSVal(Arg, LCtx);
 
-  state = checkNonNull(C, state, Arg, ArgVal);
+  state = checkNonNull(C, state, Arg, ArgVal, 1);
 
   if (!state)
     return;
 
   SVal strLength = getCStringLength(C, state, Arg, ArgVal);
 
   // If the argument isn't a valid C string, there's no valid state to
   // transition to.
void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
   CurrentFunctionDescription = "string copy function";
   ProgramStateRef state = C.getState();
   const LocationContext *LCtx = C.getLocationContext();
 
   // Check that the destination is non-null.
   const Expr *Dst = CE->getArg(0);
   SVal DstVal = state->getSVal(Dst, LCtx);
 
-  state = checkNonNull(C, state, Dst, DstVal);
+  state = checkNonNull(C, state, Dst, DstVal, 1);
   if (!state)
     return;
 
   // Check that the source is non-null.
   const Expr *srcExpr = CE->getArg(1);
   SVal srcVal = state->getSVal(srcExpr, LCtx);
-  state = checkNonNull(C, state, srcExpr, srcVal);
+  state = checkNonNull(C, state, srcExpr, srcVal, 2);
   if (!state)
     return;
 
   // Get the string length of the source.
   SVal strLength = getCStringLength(C, state, srcExpr, srcVal);
 
   // If the source isn't a valid C string, give up.
   if (strLength.isUndef())
void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
     bool isBounded, bool ignoreCase) const {
   CurrentFunctionDescription = "string comparison function";
   ProgramStateRef state = C.getState();
   const LocationContext *LCtx = C.getLocationContext();
 
   // Check that the first string is non-null
   const Expr *s1 = CE->getArg(0);
   SVal s1Val = state->getSVal(s1, LCtx);
-  state = checkNonNull(C, state, s1, s1Val);
+  state = checkNonNull(C, state, s1, s1Val, 1);
   if (!state)
     return;
 
   // Check that the second string is non-null.
   const Expr *s2 = CE->getArg(1);
   SVal s2Val = state->getSVal(s2, LCtx);
-  state = checkNonNull(C, state, s2, s2Val);
+  state = checkNonNull(C, state, s2, s2Val, 2);
   if (!state)
     return;
 
   // Get the string length of the first string or give up.
   SVal s1Length = getCStringLength(C, state, s1, s1Val);
   if (s1Length.isUndef())
     return;
 
void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const {
 
   CurrentFunctionDescription = "strsep()";
   ProgramStateRef State = C.getState();
   const LocationContext *LCtx = C.getLocationContext();
 
   // Check that the search string pointer is non-null (though it may point to
   // a null string).
   SVal SearchStrVal = State->getSVal(SearchStrPtr, LCtx);
-  State = checkNonNull(C, State, SearchStrPtr, SearchStrVal);
+  State = checkNonNull(C, State, SearchStrPtr, SearchStrVal, 1);
   if (!State)
     return;
 
   // Check that the delimiter string is non-null.
   const Expr *DelimStr = CE->getArg(1);
   SVal DelimStrVal = State->getSVal(DelimStr, LCtx);
-  State = checkNonNull(C, State, DelimStr, DelimStrVal);
+  State = checkNonNull(C, State, DelimStr, DelimStrVal, 2);
   if (!State)
     return;
 
   SValBuilder &SVB = C.getSValBuilder();
   SVal Result;
   if (Optional<Loc> SearchStrLoc = SearchStrVal.getAs<Loc>()) {
     // Get the current value of the search string pointer, as a char*.
     Result = State->getSVal(*SearchStrLoc, CharPtrTy);
void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const {
   if (StateZeroSize && !StateNonZeroSize) {
     StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, MemVal);
     C.addTransition(StateZeroSize);
     return;
   }
 
   // Ensure the memory area is not null.
   // If it is NULL there will be a NULL pointer dereference.
-  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
+  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal, 1);
   if (!State)
     return;
 
   State = CheckBufferAccess(C, State, Size, Mem);
   if (!State)
     return;
 
   // According to the values of the arguments, bind the value of the second
if (StateZeroSize && !StateNonZeroSize) {
     return;
   }
 
   // Get the value of the memory area.
   SVal MemVal = C.getSVal(Mem);
 
   // Ensure the memory area is not null.
   // If it is NULL there will be a NULL pointer dereference.
-  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
+  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal, 1);
   if (!State)
     return;
 
   State = CheckBufferAccess(C, State, Size, Mem);
   if (!State)
     return;
 
   if (!memsetAux(Mem, Zero, Size, C, State))

cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

class NonNullParamChecker
   mutable std::unique_ptr<BugType> BTAttrNonNull;
   mutable std::unique_ptr<BugType> BTNullRefArg;
 
 public:
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 
   std::unique_ptr<BugReport>
-  genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE) const;
+  genReportNullAttrNonNull(const ExplodedNode *ErrorN,
+                           const Expr *ArgE,
+                           unsigned IdxOfArg) const;
   std::unique_ptr<BugReport>
   genReportReferenceToNullPointer(const ExplodedNode *ErrorN,
                                   const Expr *ArgE) const;
 };
 } // end anonymous namespace
 
 /// \return Bitvector marking non-null attributes.
 static llvm::SmallBitVector getNonNullAttrs(const CallEvent &Call) {
for (unsigned idx = 0; idx < NumArgs; ++idx) {
 
     // Generate an error node.  Check for a null node in case
     // we cache out.
     if (stateNull && !stateNotNull) {
       if (ExplodedNode *errorNode = C.generateErrorNode(stateNull)) {
 
         std::unique_ptr<BugReport> R;
         if (haveAttrNonNull)
-          R = genReportNullAttrNonNull(errorNode, ArgE);
+          R = genReportNullAttrNonNull(errorNode, ArgE, idx + 1);
         else if (haveRefTypeParam)
           R = genReportReferenceToNullPointer(errorNode, ArgE);
 
         // Highlight the range of the argument that was null.
         R->addRange(Call.getArgSourceRange(idx));
 
         // Emit the bug report.
         C.emitReport(std::move(R));
void NonNullParamChecker::checkPreCall(const CallEvent &Call,
 
   // If we reach here all of the arguments passed the nonnull check.
   // If 'state' has been updated generated a new node.
   C.addTransition(state);
 }
 
 std::unique_ptr<BugReport>
 NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode,
-                                              const Expr *ArgE) const {
+                                              const Expr *ArgE,
+                                              unsigned IdxOfArg) const {
   // Lazily allocate the BugType object if it hasn't already been
   // created. Ownership is transferred to the BugReporter object once
   // the BugReport is passed to 'EmitWarning'.
   if (!BTAttrNonNull)
     BTAttrNonNull.reset(new BugType(
         this, "Argument with 'nonnull' attribute passed null", "API"));
 
-  auto R = std::make_unique<BugReport>(
-      *BTAttrNonNull,
-      "Null pointer passed as an argument to a 'nonnull' parameter", ErrorNode);
+  llvm::SmallString<256> SBuf;
+  llvm::raw_svector_ostream OS(SBuf);
+  OS << "Null pointer passed to "
+     << IdxOfArg << llvm::getOrdinalSuffix(IdxOfArg)
+     << " parameter expecting 'nonnull'";
+
+  auto R = llvm::make_unique<BugReport>(*BTAttrNonNull, SBuf, ErrorNode);
   if (ArgE)
     bugreporter::trackExpressionValue(ErrorNode, ArgE, *R);
 
   return R;
 }
 
 std::unique_ptr<BugReport> NonNullParamChecker::genReportReferenceToNullPointer(
     const ExplodedNode *ErrorNode, const Expr *ArgE) const {

cfe/trunk/test/Analysis/Inputs/expected-plists/edges-new.mm.plist

<array>
          <key>line</key><integer>394</integer>
          <key>col</key><integer>22</integer>
          <key>file</key><integer>0</integer>
         </dict>
        </array>
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
-     <string>Null pointer passed as an argument to a &apos;nonnull&apos; parameter</string>
+     <string>Null pointer passed to 1st parameter expecting &apos;nonnull&apos;</string>
      <key>message</key>
-     <string>Null pointer passed as an argument to a &apos;nonnull&apos; parameter</string>
+     <string>Null pointer passed to 1st parameter expecting &apos;nonnull&apos;</string>
     </dict>
    </array>
-   <key>description</key><string>Null pointer passed as an argument to a &apos;nonnull&apos; parameter</string>
+   <key>description</key><string>Null pointer passed to 1st parameter expecting &apos;nonnull&apos;</string>
    <key>category</key><string>API</string>
    <key>type</key><string>Argument with &apos;nonnull&apos; attribute passed null</string>
    <key>check_name</key><string>core.NonNullParamChecker</string>
    <!-- This hash is experimental and going to change! -->
    <key>issue_hash_content_of_line_in_context</key><string>c0b359a043c633f1b8d1581f68743361</string>
   <key>issue_context_kind</key><string>function</string>
   <key>issue_context</key><string>RDar13295437</string>
   <key>issue_hash_function_offset</key><string>3</string>

cfe/trunk/test/Analysis/Inputs/expected-plists/plist-output.m.plist

<array>
          <key>line</key><integer>184</integer>
          <key>col</key><integer>22</integer>
          <key>file</key><integer>0</integer>
         </dict>
        </array>
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
-     <string>Null pointer passed as an argument to a &apos;nonnull&apos; parameter</string>
+     <string>Null pointer passed to 1st parameter expecting &apos;nonnull&apos;</string>
      <key>message</key>
-     <string>Null pointer passed as an argument to a &apos;nonnull&apos; parameter</string>
+     <string>Null pointer passed to 1st parameter expecting &apos;nonnull&apos;</string>
     </dict>
    </array>
-   <key>description</key><string>Null pointer passed as an argument to a &apos;nonnull&apos; parameter</string>
+   <key>description</key><string>Null pointer passed to 1st parameter expecting &apos;nonnull&apos;</string>
    <key>category</key><string>API</string>
    <key>type</key><string>Argument with &apos;nonnull&apos; attribute passed null</string>
    <key>check_name</key><string>core.NonNullParamChecker</string>
    <!-- This hash is experimental and going to change! -->
    <key>issue_hash_content_of_line_in_context</key><string>c0b359a043c633f1b8d1581f68743361</string>
   <key>issue_context_kind</key><string>function</string>
   <key>issue_context</key><string>RDar13295437</string>
   <key>issue_hash_function_offset</key><string>3</string>

cfe/trunk/test/Analysis/misc-ps-region-store.m

 rdar_8642434_typeB __attribute__((transparent_union));
 
 __attribute__((visibility("default"))) __attribute__((__nonnull__)) __attribute__((__nothrow__))
 void rdar_8642434_funcA(rdar_8642434_typeB object);
 
 void rdar_8642434_funcB(struct rdar_8642434_typeA *x, struct rdar_8642434_typeA *y) {
   rdar_8642434_funcA(x);
   if (!y)
-    rdar_8642434_funcA(y); // expected-warning{{Null pointer passed as an argument to a 'nonnull' parameter}}
+    rdar_8642434_funcA(y); // expected-warning{{Null pointer passed to 1st parameter expecting 'nonnull'}}
 }
 
 // <rdar://problem/8848957> - Handle loads and stores from a symbolic index
 // into array without warning about an uninitialized value being returned.
 // While RegionStore can't fully reason about this example, it shouldn't
 // warn here either.
 typedef struct s_test_rdar8848957 {
   int x, y, z;

cfe/trunk/test/Analysis/null-deref-ps.c

int f5() {
   
   char *s = "hello world";
   return s[0]; // no-warning
 }
 
 int bar(int* p, int q) __attribute__((nonnull));
 
 int f6(int *p) { 
-  return !p ? bar(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+  return !p ? bar(p, 1) // expected-warning {{Null pointer passed to 1st parameter expecting 'nonnull'}}
          : bar(p, 0);   // no-warning
 }
 
 int bar2(int* p, int q) __attribute__((nonnull(1)));
 
 int f6b(int *p) { 
-  return !p ? bar2(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+  return !p ? bar2(p, 1) // expected-warning {{Null pointer passed to 1st parameter expecting 'nonnull'}}
          : bar2(p, 0);   // no-warning
 }
 
 int bar3(int*p, int q, int *r) __attribute__((nonnull(1,3)));
 
 int f6c(int *p, int *q) {
-   return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+   return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed to 3rd parameter expecting 'nonnull'}}
              : bar3(p, 2, q); // no-warning
 }
 
 void f6d(int *p) {
   bar(p, 0);
   // At this point, 'p' cannot be null.
   if (!p) {
     int *q = 0;