[static analyzer] Emit buffer overflow warning in strcpy fucntion when uninitialized source array of known length(> dest length) is used

[mayurp]

Summary

Enable static analyzer to throw warnings when uninitialized source array of known length is given as the argument to strcpy function, where dest size < source size.
char x[3] = "abc";
char y[4];
strcpy(x,y); // emit buffer overflow warning

[• arthur.j.odwyer]

Your test cases and commit message look wrong to me.

char x[3] = "abc";
char y[4] = "ab";
strcpy(x,y);  // This should not warn, or at least should give a suppressible diagnostic,
              // since no overflow occurs: "ab" fits into x just fine

char x[3] = "abc";
char y[4];
strcpy(x,y);  // This should give a use-before-def diagnostic for y

char x[3] = "abc";
char y[100];
strcpy(y, x);  // This should give the "overflow" diagnostic, since it definitely attempts to strcpy an array of char that is not null-terminated

[mayurp]

Hi Arthur,

Thanks for reviewing the patch and providing valuable comments. Actually what I meant by uninitialized source array was an source array which does not contain proper string or is not properly null terminated, so probably need to change the commit message. The testcase that would appropriately test the patch would be :

char x[10] = "abcd";
char y[100] ;
memset(y,'a',100);
strcpy(x,y); // string overflow warning

when we execute the same code we get segmentation fault:
$ cat strcpy3.c
#include<string.h>

int main ()
{

char x[10] = "abcd";
char y[100] ;
memset(y,'a',100);
strcpy(x,y);
return 0;

}
$ clang strcpy3.c
$ ./a.out
Segmentation fault (core dumped)
$

And the behaviour in test cases you mentioned would be:

char x[3] = "abc";
char y[4] = "ab";
strcpy(x,y); // this will not throw warning as it fits finely into x

char x[3] = "abc";
char y[4];
strcpy(x,y); // as you pointed correctly, this would throw use-before-def for y (i had not enabled alpha checker earlier so i was getting overflow warning)

char x[3] = "abc";
char y[100];
strcpy(y, x); // the patch does not handle this as per your comments. On checking the behaviour with clang this does not seem to be buffer-overflow, I might be wrong though.

please review

Thanks,
Mayur

[mayurp]

ping

[• arthur.j.odwyer]

Mayur, you wrote:

The testcase that would appropriately test the patch would be ...

but you didn't actually revise your patch.
Now, I'm pretty sure that this patch is just entirely wrong (e.g. you wrote it without being aware that char a[3]="xyz" is not a null-terminated string), but certainly it's bad form to "ping" before you've addressed the previous round's comments.

[mayurp]

Hi Arthur,

Sorry for not updating the patch last time. I was waiting for your comments before updating it. Updated the patch now.
And for this case:

char x[3] = "abc";
char y[100];
strcpy(y, x); 

I had mentioned that this does not seem to be buffer-overflow, as when i checked the same with clang, strcpy is inserting a null terminator after copying the contents of source array.

$ cat strcpy.c
#include<string.h>
#include<stdio.h>
int main ()
{

char x[3]; // non-null terminated array
x[0] = 'a';
x[1] = 'b';
x[2] = 'c';
char y[100] ;
memset(y,'a',100);
strcpy(y,x);
printf("%s \n",y);
printf("%c\n",y[0]);
printf("%c\n",y[1]);
printf("%c\n",y[2]);
printf("%c\n",y[3]);
printf("%c\n",y[4]);
return 0;

}
$ clang strcpy.c
$ ./a.out
abc
a
b
c

a

In this example it is seen that clang is inserting null terminator and hence even while copying non-null terminated string to another array, buffer-overflow is not caused.
Please provide comments on whether this analysis is correct or not.

Thanks,
Mayur

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

       BT_NotCString, BT_AdditionOverflow;
 
   mutable const char *CurrentFunctionDescription;
+  static bool isSrcGarbage;
 
 public:
   /// The filter is used to filter out the diagnostics which are not enabled by

                                             NonLoc right) const;
 };
 
+bool CStringChecker::isSrcGarbage = 0;
+
 } //end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(CStringLength, const MemRegion *, SVal)

     const SVal *Recorded = state->get<CStringLength>(MR);
     if (Recorded)
       return *Recorded;
+    else {
+      // set isSrcGarbage, so that we will try to evaluate size of src for strcpy fn
+      isSrcGarbage = 1;
+    } 
   }
 
   // Otherwise, get a new symbol and update the state.

   QualType cmpTy = svalBuilder.getConditionType();
   QualType sizeTy = svalBuilder.getContext().getSizeType();
 
+  // If we could not record proper string length, try to get the size of src buffer
+  if (isSrcGarbage && !isAppending && !isBounded && !returnEnd) {
+    isSrcGarbage = 0;
+    const MemRegion *R = srcVal.getAsRegion();
+    if (!R)
+      return;
+
+    const ElementRegion *ER = dyn_cast<ElementRegion>(R);
+    SVal retsize = UnknownVal();
+    if (ER) {
+      assert(ER->getValueType() == C.getASTContext().CharTy &&
+      "Should only be called with char* ElementRegions");
+
+      // Get the size of the array.
+      const SubRegion *superReg = cast<SubRegion>(ER->getSuperRegion());
+      SVal Extent = svalBuilder.convertToArrayIndex(superReg->getExtent(svalBuilder));
+      DefinedOrUnknownSVal strSize = cast<DefinedOrUnknownSVal>(Extent);
+      retsize = strSize;
+
+      NonLoc One = svalBuilder.makeIntVal(1, sizeTy).castAs<NonLoc>();
+      NonLoc *knownStrSize = cast<NonLoc>(&retsize);
+      retsize = (svalBuilder.evalBinOpNN(state, BO_Sub, *knownStrSize, One, sizeTy));
+
+      if (!(retsize.isUnknown()) && !(retsize.isUndef())) {
+        strLength = retsize;
+      }
+    }
+  }
+
   // These two values allow checking two kinds of errors:
   // - actual overflows caused by a source that doesn't fit in the destination
   // - potential overflows caused by a bound that could exceed the destination

test/Analysis/string.c

     strcpy(x, y); // no-warning
 }
 
+extern void * memset(void *, int , unsigned long );
+void strcpy_src_garbage_overflow1() {
+  char x[4] = "abc";
+  char y[5];
+  memset(y,'a',5);
+  strcpy(x,y);  // expected-warning{{String copy function overflows destination buffer}}
+}
+
 //===----------------------------------------------------------------------===
 // stpcpy()
 //===----------------------------------------------------------------------===