[analyzer] Fix crash in Stream checker when using void pointers

[steakhal]

We can get zero type size (thus div by zero crash) if the region is for a 'void*' pointer.
In this patch, let's just override the void type with a char type to avoid the crash.

Fixes
#93408 (comment)

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balazs Benics (steakhal)

Changes

We can get zero type size (thus div by zero crash) if the region is for a 'void*' pointer.
In this patch, let's just override the void type with a char type to avoid the crash.

Fixes
#93408 (comment)

---

Full diff: https://github.com/llvm/llvm-project/pull/97199.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+25-14)
• (modified) clang/test/Analysis/stream.c (+13)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 9aee7f952ad2d..0d1d50d19f4c4 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1034,16 +1034,19 @@ void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent &Call,
   C.addTransition(State);
 }
 
-static std::optional<QualType> getPointeeType(const MemRegion *R) {
+static QualType getPointeeType(const MemRegion *R) {
   if (!R)
-    return std::nullopt;
-  if (const auto *ER = dyn_cast<ElementRegion>(R))
-    return ER->getElementType();
-  if (const auto *TR = dyn_cast<TypedValueRegion>(R))
-    return TR->getValueType();
-  if (const auto *SR = dyn_cast<SymbolicRegion>(R))
-    return SR->getPointeeStaticType();
-  return std::nullopt;
+    return {};
+  QualType Ty = [R] {
+    if (const auto *ER = dyn_cast<ElementRegion>(R))
+      return ER->getElementType();
+    if (const auto *TR = dyn_cast<TypedValueRegion>(R))
+      return TR->getValueType();
+    if (const auto *SR = dyn_cast<SymbolicRegion>(R))
+      return SR->getPointeeStaticType();
+    return QualType{};
+  }();
+  return !Ty.isNull() ? Ty->getCanonicalTypeUnqualified() : QualType{};
 }
 
 static std::optional<NonLoc> getStartIndex(SValBuilder &SVB,
@@ -1073,7 +1076,16 @@ tryToInvalidateFReadBufferByElements(ProgramStateRef State, CheckerContext &C,
   const auto *Buffer =
       dyn_cast_or_null<SubRegion>(Call.getArgSVal(0).getAsRegion());
 
-  std::optional<QualType> ElemTy = getPointeeType(Buffer);
+  const ASTContext &Ctx = C.getASTContext();
+
+  QualType ElemTy = getPointeeType(Buffer);
+
+  // Consider pointer to void as a pointer to char buffer such that it has a
+  // non-zero type size.
+  if (!ElemTy.isNull() && ElemTy == Ctx.VoidTy) {
+    ElemTy = Ctx.CharTy;
+  }
+
   std::optional<SVal> StartElementIndex =
       getStartIndex(C.getSValBuilder(), Buffer);
 
@@ -1086,10 +1098,9 @@ tryToInvalidateFReadBufferByElements(ProgramStateRef State, CheckerContext &C,
   std::optional<int64_t> StartIndexVal =
       getKnownValue(State, StartElementIndex.value_or(UnknownVal()));
 
-  if (ElemTy && CountVal && Size && StartIndexVal) {
+  if (!ElemTy.isNull() && CountVal && Size && StartIndexVal) {
     int64_t NumBytesRead = Size.value() * CountVal.value();
-    int64_t ElemSizeInChars =
-        C.getASTContext().getTypeSizeInChars(*ElemTy).getQuantity();
+    int64_t ElemSizeInChars = Ctx.getTypeSizeInChars(ElemTy).getQuantity();
     bool IncompleteLastElement = (NumBytesRead % ElemSizeInChars) != 0;
     int64_t NumCompleteOrIncompleteElementsRead =
         NumBytesRead / ElemSizeInChars + IncompleteLastElement;
@@ -1097,7 +1108,7 @@ tryToInvalidateFReadBufferByElements(ProgramStateRef State, CheckerContext &C,
     constexpr int MaxInvalidatedElementsLimit = 64;
     if (NumCompleteOrIncompleteElementsRead <= MaxInvalidatedElementsLimit) {
       return escapeByStartIndexAndCount(State, Call, C.blockCount(), Buffer,
-                                        *ElemTy, *StartIndexVal,
+                                        ElemTy, *StartIndexVal,
                                         NumCompleteOrIncompleteElementsRead);
     }
   }
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index db03d90cd8af4..0869b1b325172 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -453,3 +453,16 @@ void getline_buffer_size_negative() {
   free(buffer);
   fclose(file);
 }
+
+void gh_93408_regression(void *buffer) {
+  FILE *f = fopen("/tmp/foo.txt", "r");
+  fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash: void is treated as char.
+  fclose(f);
+}
+
+typedef void VOID;
+void gh_93408_regression_typedef(VOID *buffer) {
+  FILE *f = fopen("/tmp/foo.txt", "r");
+  fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash: VOID is treated as char.
+  fclose(f);
+}

[NagyDonat]

Unfortunately this PR is not a full solution, because e.g. the following test code still triggers the crash (if it is appended to the test file stream.c):

struct zerosized {
    int foo[0];
};

void fread_zerosized(struct zerosized *buffer) {
  FILE *f = fopen("/tmp/foo.txt", "r");
  fread(buffer, 1, 1, f);
  fclose(f);
}

(I know that zero-sized arrays are not standard-compliant, but they are a widespread extension: e.g. both clang and gcc accept this struct zerosized with the default settings.)

Overall, I'd say that it's futile to try to recognize zero-sized types with a "canonical type equal to" check, so you should just check whether ElemSizeInChars is zero and do something based on that. (Either an early return, or you can say ElemSizeInChars = 1 at that point if you think that that's the logically correct solution.)

<bikeshedding>This way you could also avoid the immediately invoked lambda in getPointeeType which is really ugly in my opinion.</bikeshedding>

[NagyDonat]

I only noticed that this PR was already merged after posting the review. There is no need to revert the commit -- it's better than nothing -- but I'd be happy if you created a followup change that also handles the testcase that I mentioned.

[steakhal]

Overall, I'd say that it's futile to try to recognize zero-sized types with a "canonical type equal to" check, so you should just check whether ElemSizeInChars is zero and do something based on that. (Either an early return, or you can say ElemSizeInChars = 1 at that point if you think that that's the logically correct solution.)

Yes :/

<bikeshedding>This way you could also avoid the immediately invoked lambda in getPointeeType which is really ugly in my opinion.</bikeshedding>

Alright. Duplicated the call I wanted to avoid, but I agree it looks better now.

I only noticed that this PR was already merged after posting the review. There is no need to revert the commit -- it's better than nothing -- but I'd be happy if you created a followup change that also handles the testcase that I mentioned.

I'm not sure what you refer to. This PR is not approved, hence not merged.
Please continue with the review.

[NagyDonat]

Thanks for the updates! I added few minor comments, but the PR is already good enough to be merged.

I only noticed that this PR was already merged after posting the review. There is no need to revert the commit -- it's better than nothing -- but I'd be happy if you created a followup change that also handles the testcase that I mentioned.

I'm not sure what you refer to. This PR is not approved, hence not merged. Please continue with the review.

Oops, sorry -- I was probably confused by the purple "merged" icon in

...but the real reason is that I got up at 4 AM today 😴

[NagyDonat]

It's a bit strange that here you return {}, while at the end of the function QualType{} is explicitly specified.

[NagyDonat]

Consider moving the ->getCanonicalTypeUnqualified() calls into escapeByStartIndexAndCount() because that's the only place where it will be relevant. (This would eliminate the code duplication.)

[steakhal]

Actually, now that I don't compare the type against a concrete alternative, it's no longer relevant. I'll just drop it. Thanks for spotting.