[cfe-commits] strncpy checker

Argyrios Kyrtzidis kyrtzidis at apple.com
Wed Dec 15 17:44:41 PST 2010


Hi Lenny,

On Dec 15, 2010, at 8:51 AM, Leonard Maiorani wrote:
> Attached is my first foray into Clang Static Analyzer contributions. Please review for style and correctness. Take punches at it if you will, I am tough enough. Unit tests are included.
> 
> strncpy() was not being checked at all, so this should solve that problem by ensuring that the 'n' parameter to strncpy() is not larger than the size of the buffer passed in because that would indicate a logic error and potential bus error.


    BuiltinBug *BT;
    if (IsDestination) {
      if (!BT_BoundsWrite) {
-        BT_BoundsWrite = new BuiltinBug("Out-of-bound array access",
-          "Byte string function overflows destination buffer");
+        if (IsDefinedLength) {
+          BT_BoundsWrite = new BuiltinBug("Out-of-bound array access",
+            "Byte string function length parameter overflows destination buffer");
+        } else {
+          BT_BoundsWrite = new BuiltinBug("Out-of-bound array access",
+            "Byte string function overflows destination buffer");
+        }
      }
      BT = static_cast<BuiltinBug*>(BT_BoundsWrite);
    } else {
      if (!BT_Bounds) {
-        BT_Bounds = new BuiltinBug("Out-of-bound array access",
-          "Byte string function accesses out-of-bound array element");
+        if (IsDefinedLength) {
+          BT_BoundsWrite = new BuiltinBug("Out-of-bound array access",
+            "Byte string function length parameter accesses out-of-bound element");
+        } else {
+          BT_Bounds = new BuiltinBug("Out-of-bound array access",
+            "Byte string function accesses out-of-bound array element");
+        }
      }
      BT = static_cast<BuiltinBug*>(BT_Bounds);
    }

BT_BoundsWrite/BT_Bounds is created once and reused, so this change sets the bug string for all strcpy and strncpy bugs in the same function,
depending on what occured first.
You probably meant to use new BuiltinBugs (e.g. BT_BoundsWrite_strncpy) but I think it's adequate to leave it as is and use the same message;
then the IsDefinedLength parameter won't be needed.


void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
-                                      bool returnEnd) {
+                                      bool returnEnd, bool specifiedLen) {
  const GRState *state = C.getState();

  // Check that the destination is non-null
@@ -828,6 +847,12 @@
  // Get the string length of the source.
  SVal strLength = getCStringLength(C, state, srcExpr, srcVal);

+  if (specifiedLen) {
+    // Check if the number of bytes to copy is less than the size of the src
+    const Expr *lenExpr = CE->getArg(2);
+    strLength = state->getSVal(lenExpr);
+  }
+

Maybe rename specifiedLen to isStrncpy to make it more clear that this affects what kind of CallExpr we got ?

Other than that, it looks good, thanks for working in it.

As an aside, the CStringChecker probably needs some refactoring and improvement to warn for non null-terminated strings,
but this would require some infrastructure changes.

-Argiris

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


More information about the cfe-commits mailing list