[analyzer] Fix use of length in CStringChecker

[vabridgers]

Summary

CStringChecker is using getByteLength to get the length of a string
literal. For targets where a "char" is 8-bits, getByteLength() and
getLength() will be equal for a C string, but for targets where a "char"
is 16-bits getByteLength() returns the size in octets.

This is verified in our downstream target, but we have no way to add a
test case for this case since there is no target supporting 16-bit
"char" upstream. Since this cannot have a test case, I'm asserted this
change is "correct by construction", and visually inspected to be
correct by way of the following example where this was found.

The case that shows this fails using a target with 16-bit chars is here.
getByteLength() for the string literal returns 4, which fails when
checked against "char x[4]". With the change, the string literal is
evaluated to a size of 2 which is a correct number of "char"'s for a
16-bit target.

void strcpy_no_overflow_2(char *y) {
  char x[4];
  strcpy(x, "12"); // with getByteLength(), returns 4 using 16-bit chars
}

This change exposed that embedded nulls within the string are not
handled. This is documented as a FIXME for a future fix.

void strcpy_no_overflow_3(char *y) {
  char x[3];
  strcpy(x, "12\0");
}

[steakhal]

Actually, there is one more bug there. If the string has an embedded null-terminator; a wrong length will be returned.
Please add tests to demonstrate the wrong behavior. Try different target triples for having irregular char sizes and pin that.

[vabridgers]

Thanks Balazs, you mean something like this correct?

void strcpy_no_overflow_2(char *y) {
  char x[3];
  strcpy(x, "12\0"); // this produces a warning, but should not. 
}

If I understand correctly, we'd want to iterate through the string literal by "CodeUnit" size, checking each character for NULL. Something like this (terribly hackish ...)

for (countx = 0; 
      countx < strLit->getLength() && (strLit->getCodeUnit(countx) != 0); 
      countx++);

Is that what you're referring too? Nice eye by the way, to have seen that.

Best

[vabridgers]

a proposal to handle embedded null case caught by @steakhal

[martong]

Thanks Vince for the patch! I think the implementation is correct. But I am worried that it might do sub-optimal computations on the string literals. I mean, getting the length would require O(N) steps each time, and that could be problematic with long literals, especially in projects that have many literals.

So, I'd rather keep your first implementation with getLength with the new test case which fails (and the failure is documented). I think, this is what @steakhal was also suggesting.

Then, if your time allows, in a second patch, we could have the loop based implementation for catching embedded null terminators. However, that change should have it's own measurement and maybe even it's own command line option to enable.

[vabridgers]

Thanks @martong , I understand. I'll make the changes and resubmit. Best!

[vabridgers]

update per comments from @martong

[martong]

LGTM

clang/docs/analyzer/checkers.rst

  }
 
 .. _alpha-unix-cstring-OutOfBounds:
 
 alpha.unix.cstring.OutOfBounds (C)
 """"""""""""""""""""""""""""""""""
 Check for out-of-bounds access in string functions; applies to:`` strncopy, strncat``.
 
+This check also applies to string literals, except there is a known bug in that
+the analyzer cannot detect embedded NULL characters.
+
 .. code-block:: c
 
  void test() {
    int y = strlen((char *)&test); // warn
  }
 
 .. _alpha-unix-cstring-UninitializedRead:
 

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state,
 
   switch (MR->getKind()) {
   case MemRegion::StringRegionKind: {
     // Modifying the contents of string regions is undefined [C99 6.4.5p6],
     // so we can assume that the byte length is the correct C string length.
     SValBuilder &svalBuilder = C.getSValBuilder();
     QualType sizeTy = svalBuilder.getContext().getSizeType();
     const StringLiteral *strLit = cast<StringRegion>(MR)->getStringLiteral();
-    return svalBuilder.makeIntVal(strLit->getByteLength(), sizeTy);
+    return svalBuilder.makeIntVal(strLit->getLength(), sizeTy);
   }
   case MemRegion::SymbolicRegionKind:
   case MemRegion::AllocaRegionKind:
   case MemRegion::NonParamVarRegionKind:
   case MemRegion::ParamVarRegionKind:
   case MemRegion::FieldRegionKind:
   case MemRegion::ObjCIvarRegionKind:
     return getCStringLengthForRegion(C, state, Ex, MR, hypothetical);

clang/test/Analysis/string.c

void memset29_plain_int_zero(void) {
   clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 }
 
 void test_memset_chk(void) {
   int x;
   __builtin___memset_chk(&x, 0, sizeof(x), __builtin_object_size(&x, 0));
   clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 }
+
+#ifndef SUPPRESS_OUT_OF_BOUND
+void strcpy_no_overflow_2(char *y) {
+  char x[3];
+  // FIXME: string literal modeling does not account for embedded NULLs.
+  //        This case should not elicit a warning, but does.
+  //        See discussion at https://reviews.llvm.org/D129269
+  strcpy(x, "12\0"); // expected-warning{{String copy function overflows the destination buffer}}
+}
+#else
+void strcpy_no_overflow_2(char *y) {
+  char x[3];
+  strcpy(x, "12\0");
+}
+#endif