[clang] Implement -Walloc-size diagnostic option

[vvuksanovic]

Warns about calls to functions decorated with attribute alloc_size that specify insufficient size for the type they are cast to. Matches the behavior of the GCC option of the same name.

Closes #138973

[llvmbot]

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

@llvm/pr-subscribers-clang

Author: Vladimir Vuksanovic (vvuksanovic)

Changes

Warns about calls to functions decorated with attribute alloc_size that specify insufficient size for the type they are cast to. Matches the behavior of the GCC option of the same name.

Closes #138973

---

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

7 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+4)
• (modified) clang/include/clang/AST/Expr.h (+26-17)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+6)
• (modified) clang/lib/AST/Expr.cpp (+53)
• (modified) clang/lib/AST/ExprConstant.cpp (+3-63)
• (modified) clang/lib/Sema/SemaExpr.cpp (+36)
• (added) clang/test/Sema/warn-alloc-size.c (+42)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 46a77673919d3..bf4615129f357 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -717,6 +717,10 @@ Improvements to Clang's diagnostics
   Added a new warning in this group for the case where the attribute is missing/implicit on
   an override of a virtual method.
 
+- A new warning ``-Walloc-size`` has been added to detect calls to functions
+  decorated with the ``alloc_size`` attribute don't allocate enough space for
+  the target pointer type.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 523c0326d47ef..8bdd0d45dcbb1 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -40,23 +40,24 @@
 #include <optional>
 
 namespace clang {
-  class APValue;
-  class ASTContext;
-  class BlockDecl;
-  class CXXBaseSpecifier;
-  class CXXMemberCallExpr;
-  class CXXOperatorCallExpr;
-  class CastExpr;
-  class Decl;
-  class IdentifierInfo;
-  class MaterializeTemporaryExpr;
-  class NamedDecl;
-  class ObjCPropertyRefExpr;
-  class OpaqueValueExpr;
-  class ParmVarDecl;
-  class StringLiteral;
-  class TargetInfo;
-  class ValueDecl;
+class AllocSizeAttr;
+class APValue;
+class ASTContext;
+class BlockDecl;
+class CXXBaseSpecifier;
+class CXXMemberCallExpr;
+class CXXOperatorCallExpr;
+class CastExpr;
+class Decl;
+class IdentifierInfo;
+class MaterializeTemporaryExpr;
+class NamedDecl;
+class ObjCPropertyRefExpr;
+class OpaqueValueExpr;
+class ParmVarDecl;
+class StringLiteral;
+class TargetInfo;
+class ValueDecl;
 
 /// A simple array of base specifiers.
 typedef SmallVector<CXXBaseSpecifier*, 4> CXXCastPath;
@@ -3256,6 +3257,14 @@ class CallExpr : public Expr {
     setDependence(getDependence() | ExprDependence::TypeValueInstantiation);
   }
 
+  /// Try to get the alloc_size attribute of the callee. May return null.
+  const AllocSizeAttr *getAllocSizeAttr() const;
+
+  /// Get the total size in bytes allocated by calling a function decorated with
+  /// alloc_size. Returns true if the the result was successfully evaluated.
+  bool getBytesReturnedByAllocSizeCall(const ASTContext &Ctx,
+                                       llvm::APInt &Result) const;
+
   bool isCallToStdMove() const;
 
   static bool classof(const Stmt *T) {
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b2ea65ae111be..67bd44f1e5dc4 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3684,6 +3684,12 @@ def warn_alloca_align_alignof : Warning<
   "second argument to __builtin_alloca_with_align is supposed to be in bits">,
   InGroup<DiagGroup<"alloca-with-align-alignof">>;
 
+def warn_alloc_size
+    : Warning<
+          "allocation of insufficient size '%0' for type %1 with size '%2'">,
+      InGroup<DiagGroup<"alloc-size">>,
+      DefaultIgnore;
+
 def err_alignment_too_small : Error<
   "requested alignment must be %0 or greater">;
 def err_alignment_too_big : Error<
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 2e1a9a3d9ad63..eb5f5875fcc76 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3542,6 +3542,59 @@ bool CallExpr::isBuiltinAssumeFalse(const ASTContext &Ctx) const {
          Arg->EvaluateAsBooleanCondition(ArgVal, Ctx) && !ArgVal;
 }
 
+const AllocSizeAttr *CallExpr::getAllocSizeAttr() const {
+  if (const FunctionDecl *DirectCallee = getDirectCallee())
+    return DirectCallee->getAttr<AllocSizeAttr>();
+  if (const Decl *IndirectCallee = getCalleeDecl())
+    return IndirectCallee->getAttr<AllocSizeAttr>();
+  return nullptr;
+}
+
+bool CallExpr::getBytesReturnedByAllocSizeCall(const ASTContext &Ctx,
+                                               llvm::APInt &Result) const {
+  const AllocSizeAttr *AllocSize = getAllocSizeAttr();
+
+  assert(AllocSize && AllocSize->getElemSizeParam().isValid());
+  unsigned SizeArgNo = AllocSize->getElemSizeParam().getASTIndex();
+  unsigned BitsInSizeT = Ctx.getTypeSize(Ctx.getSizeType());
+  if (getNumArgs() <= SizeArgNo)
+    return false;
+
+  auto EvaluateAsSizeT = [&](const Expr *E, llvm::APSInt &Into) {
+    Expr::EvalResult ExprResult;
+    if (E->isValueDependent() ||
+        !E->EvaluateAsInt(ExprResult, Ctx, Expr::SE_AllowSideEffects))
+      return false;
+    Into = ExprResult.Val.getInt();
+    if (Into.isNegative() || !Into.isIntN(BitsInSizeT))
+      return false;
+    Into = Into.zext(BitsInSizeT);
+    return true;
+  };
+
+  llvm::APSInt SizeOfElem;
+  if (!EvaluateAsSizeT(getArg(SizeArgNo), SizeOfElem))
+    return false;
+
+  if (!AllocSize->getNumElemsParam().isValid()) {
+    Result = std::move(SizeOfElem);
+    return true;
+  }
+
+  llvm::APSInt NumberOfElems;
+  unsigned NumArgNo = AllocSize->getNumElemsParam().getASTIndex();
+  if (!EvaluateAsSizeT(getArg(NumArgNo), NumberOfElems))
+    return false;
+
+  bool Overflow;
+  llvm::APInt BytesAvailable = SizeOfElem.umul_ov(NumberOfElems, Overflow);
+  if (Overflow)
+    return false;
+
+  Result = std::move(BytesAvailable);
+  return true;
+}
+
 bool CallExpr::isCallToStdMove() const {
   return getBuiltinCallee() == Builtin::BImove;
 }
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 0d12161756467..a295d5e38b332 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -114,15 +114,6 @@ namespace {
     return Ctx.getLValueReferenceType(E->getType());
   }
 
-  /// Given a CallExpr, try to get the alloc_size attribute. May return null.
-  static const AllocSizeAttr *getAllocSizeAttr(const CallExpr *CE) {
-    if (const FunctionDecl *DirectCallee = CE->getDirectCallee())
-      return DirectCallee->getAttr<AllocSizeAttr>();
-    if (const Decl *IndirectCallee = CE->getCalleeDecl())
-      return IndirectCallee->getAttr<AllocSizeAttr>();
-    return nullptr;
-  }
-
   /// Attempts to unwrap a CallExpr (with an alloc_size attribute) from an Expr.
   /// This will look through a single cast.
   ///
@@ -142,7 +133,7 @@ namespace {
       E = Cast->getSubExpr()->IgnoreParens();
 
     if (const auto *CE = dyn_cast<CallExpr>(E))
-      return getAllocSizeAttr(CE) ? CE : nullptr;
+      return CE->getAllocSizeAttr() ? CE : nullptr;
     return nullptr;
   }
 
@@ -9439,57 +9430,6 @@ bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) {
 // Pointer Evaluation
 //===----------------------------------------------------------------------===//
 
-/// Attempts to compute the number of bytes available at the pointer
-/// returned by a function with the alloc_size attribute. Returns true if we
-/// were successful. Places an unsigned number into `Result`.
-///
-/// This expects the given CallExpr to be a call to a function with an
-/// alloc_size attribute.
-static bool getBytesReturnedByAllocSizeCall(const ASTContext &Ctx,
-                                            const CallExpr *Call,
-                                            llvm::APInt &Result) {
-  const AllocSizeAttr *AllocSize = getAllocSizeAttr(Call);
-
-  assert(AllocSize && AllocSize->getElemSizeParam().isValid());
-  unsigned SizeArgNo = AllocSize->getElemSizeParam().getASTIndex();
-  unsigned BitsInSizeT = Ctx.getTypeSize(Ctx.getSizeType());
-  if (Call->getNumArgs() <= SizeArgNo)
-    return false;
-
-  auto EvaluateAsSizeT = [&](const Expr *E, APSInt &Into) {
-    Expr::EvalResult ExprResult;
-    if (!E->EvaluateAsInt(ExprResult, Ctx, Expr::SE_AllowSideEffects))
-      return false;
-    Into = ExprResult.Val.getInt();
-    if (Into.isNegative() || !Into.isIntN(BitsInSizeT))
-      return false;
-    Into = Into.zext(BitsInSizeT);
-    return true;
-  };
-
-  APSInt SizeOfElem;
-  if (!EvaluateAsSizeT(Call->getArg(SizeArgNo), SizeOfElem))
-    return false;
-
-  if (!AllocSize->getNumElemsParam().isValid()) {
-    Result = std::move(SizeOfElem);
-    return true;
-  }
-
-  APSInt NumberOfElems;
-  unsigned NumArgNo = AllocSize->getNumElemsParam().getASTIndex();
-  if (!EvaluateAsSizeT(Call->getArg(NumArgNo), NumberOfElems))
-    return false;
-
-  bool Overflow;
-  llvm::APInt BytesAvailable = SizeOfElem.umul_ov(NumberOfElems, Overflow);
-  if (Overflow)
-    return false;
-
-  Result = std::move(BytesAvailable);
-  return true;
-}
-
 /// Convenience function. LVal's base must be a call to an alloc_size
 /// function.
 static bool getBytesReturnedByAllocSizeCall(const ASTContext &Ctx,
@@ -9499,7 +9439,7 @@ static bool getBytesReturnedByAllocSizeCall(const ASTContext &Ctx,
          "Can't get the size of a non alloc_size function");
   const auto *Base = LVal.getLValueBase().get<const Expr *>();
   const CallExpr *CE = tryUnwrapAllocSizeCall(Base);
-  return getBytesReturnedByAllocSizeCall(Ctx, CE, Result);
+  return CE->getBytesReturnedByAllocSizeCall(Ctx, Result);
 }
 
 /// Attempts to evaluate the given LValueBase as the result of a call to
@@ -9977,7 +9917,7 @@ bool PointerExprEvaluator::visitNonBuiltinCallExpr(const CallExpr *E) {
   if (ExprEvaluatorBaseTy::VisitCallExpr(E))
     return true;
 
-  if (!(InvalidBaseOK && getAllocSizeAttr(E)))
+  if (!(InvalidBaseOK && E->getAllocSizeAttr()))
     return false;
 
   Result.setInvalid(E);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 45c7178c6965d..24088a7ca640c 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -18,6 +18,7 @@
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/ASTMutationListener.h"
+#include "clang/AST/Attrs.inc"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
@@ -7818,6 +7819,33 @@ ExprResult Sema::CheckExtVectorCast(SourceRange R, QualType DestTy,
   return prepareVectorSplat(DestTy, CastExpr);
 }
 
+/// Check that a call to alloc_size function specifies sufficient space for the
+/// destination type.
+static void CheckSufficientAllocSize(Sema &S, QualType DestType,
+                                     const Expr *E) {
+  QualType SourceType = E->getType();
+  if (!DestType->isPointerType() || !SourceType->isPointerType() ||
+      DestType == SourceType)
+    return;
+
+  const auto *CE = dyn_cast<CallExpr>(E->IgnoreCasts());
+  if (!CE)
+    return;
+
+  // Find the total size allocated by the function call.
+  llvm::APInt AllocSize;
+  if (!CE->getAllocSizeAttr() ||
+      !CE->getBytesReturnedByAllocSizeCall(S.Context, AllocSize))
+    return;
+  auto Size = CharUnits::fromQuantity(AllocSize.getZExtValue());
+
+  QualType TargetType = DestType->getPointeeType();
+  auto LhsSize = S.Context.getTypeSizeInCharsIfKnown(TargetType);
+  if (LhsSize && Size < LhsSize)
+    S.Diag(E->getExprLoc(), diag::warn_alloc_size)
+        << Size.getQuantity() << TargetType << LhsSize->getQuantity();
+}
+
 ExprResult
 Sema::ActOnCastExpr(Scope *S, SourceLocation LParenLoc,
                     Declarator &D, ParsedType &Ty,
@@ -7883,6 +7911,8 @@ Sema::ActOnCastExpr(Scope *S, SourceLocation LParenLoc,
 
   DiscardMisalignedMemberAddress(castType.getTypePtr(), CastExpr);
 
+  CheckSufficientAllocSize(*this, castType, CastExpr);
+
   return BuildCStyleCastExpr(LParenLoc, castTInfo, RParenLoc, CastExpr);
 }
 
@@ -9887,6 +9917,12 @@ AssignConvertType Sema::CheckSingleAssignmentConstraints(QualType LHSType,
   AssignConvertType result =
       CheckAssignmentConstraints(LHSType, RHS, Kind, ConvertRHS);
 
+  // If assigning a void * created by an allocation function call to some other
+  // type, check that the allocated size is sufficient for that type.
+  if (result != AssignConvertType::Incompatible &&
+      RHS.get()->getType()->isVoidPointerType())
+    CheckSufficientAllocSize(*this, LHSType, RHS.get());
+
   // C99 6.5.16.1p2: The value of the right operand is converted to the
   // type of the assignment expression.
   // CheckAssignmentConstraints allows the left-hand side to be a reference,
diff --git a/clang/test/Sema/warn-alloc-size.c b/clang/test/Sema/warn-alloc-size.c
new file mode 100644
index 0000000000000..e1ce341b79678
--- /dev/null
+++ b/clang/test/Sema/warn-alloc-size.c
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloc-size %s
+struct Foo { int x[10]; };
+
+void *malloc(unsigned long) __attribute__((alloc_size(1)));
+void *alloca(unsigned long) __attribute__((alloc_size(1)));
+void *calloc(unsigned long, unsigned long) __attribute__((alloc_size(2, 1)));
+
+void foo_consumer(struct Foo* p);
+
+void alloc_foo(void) {
+  struct Foo *ptr1 = malloc(sizeof(struct Foo));
+  struct Foo *ptr2 = malloc(sizeof(*ptr2));
+  struct Foo *ptr3 = calloc(1, sizeof(*ptr3));
+  struct Foo *ptr4 = calloc(sizeof(*ptr4), 1);
+  struct Foo (*ptr5)[5] = malloc(sizeof(*ptr5));
+  void *ptr6 = malloc(4);
+
+  // Test insufficient size with different allocation functions.
+  struct Foo *ptr7 = malloc(sizeof(ptr7));      // expected-warning {{allocation of insufficient size '8' for type 'struct Foo' with size '40'}}
+  struct Foo *ptr8 = alloca(sizeof(ptr8));      // expected-warning {{allocation of insufficient size '8' for type 'struct Foo' with size '40'}}
+  struct Foo *ptr9 = calloc(1, sizeof(ptr9));   // expected-warning {{allocation of insufficient size '8' for type 'struct Foo' with size '40'}}
+  struct Foo *ptr10 = calloc(sizeof(ptr10), 1); // expected-warning {{allocation of insufficient size '8' for type 'struct Foo' with size '40'}}
+
+  // Test function arguments.
+  foo_consumer(malloc(4)); // expected-warning {{allocation of insufficient size '4' for type 'struct Foo' with size '40'}}
+
+  // Test explicit cast.
+  struct Foo *ptr11 = (struct Foo *)malloc(sizeof(*ptr11));
+  struct Foo *ptr12 = (struct Foo *)malloc(sizeof(ptr12));    // expected-warning {{allocation of insufficient size '8' for type 'struct Foo' with size '40'}}
+  struct Foo *ptr13 = (struct Foo *)alloca(sizeof(ptr13));    // expected-warning {{allocation of insufficient size '8' for type 'struct Foo' with size '40'}}
+  struct Foo *ptr14 = (struct Foo *)calloc(1, sizeof(ptr14)); // expected-warning {{allocation of insufficient size '8' for type 'struct Foo' with size '40'}}
+  struct Foo *ptr15 = (struct Foo *)malloc(4);                // expected-warning {{allocation of insufficient size '4' for type 'struct Foo' with size '40'}}
+  void *ptr16 = (struct Foo *)malloc(4);                      // expected-warning {{allocation of insufficient size '4' for type 'struct Foo' with size '40'}}
+
+  struct Foo *ptr17 = (void *)(struct Foo *)malloc(4); // expected-warning 2 {{allocation of insufficient size '4' for type 'struct Foo' with size '40'}}
+  int *ptr18 = (unsigned *)(void *)(int *)malloc(1);   // expected-warning {{initializing 'int *' with an expression of type 'unsigned int *' converts between pointers to integer types with different sign}}
+                                                       // expected-warning@-1 {{allocation of insufficient size '1' for type 'int' with size '4'}}
+                                                       // expected-warning@-2 {{allocation of insufficient size '1' for type 'unsigned int' with size '4'}}
+  int *ptr19 = (void *)(int *)malloc(1);               // expected-warning {{allocation of insufficient size '1' for type 'int' with size '4'}}
+                                                       // expected-warning@-1 {{allocation of insufficient size '1' for type 'int' with size '4'}}
+  (void)(int *)malloc(1);                              // expected-warning {{allocation of insufficient size '1' for type 'int' with size '4'}}
+}

[AaronBallman]

Then there's one more test I'd like to see added:

void *ptr = (void (*)(int))malloc(0); // Should diagnose
void *ptr = (void (*)(int))malloc(1); // Should not diagnose

[AaronBallman]

LGTM! Please give @erichkeane a chance to sign off before landing, though

[erichkeane]

1 have 1 concern with how we're getting the value of alloc-size, and think we probably need a little tweaking, else this is a LGTM.

[erichkeane]

Does this need to be BitsInSizeT-1? In the case where the size_t is signed, but the expression is unsigned-same-bits-as-size_t?

That would make the EvaluateAsSizeT result in a negative value.

[vvuksanovic]

I'm not sure I understand, isn't size_t guaranteed to be unsigned?

Also, this code isn't new, it was just moved here.

[erichkeane]

Yep, you're right about size_t, no idea what I was thinking of. Also didn't notice this was a 'move'.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp,c -- clang/test/Sema/warn-alloc-size.c clang/include/clang/AST/Expr.h clang/lib/AST/Expr.cpp clang/lib/AST/ExprConstant.cpp clang/lib/Sema/SemaExpr.cpp clang/test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp clang/test/Analysis/Malloc+MismatchedDeallocator_intersections.cpp clang/test/Analysis/NewDelete-checker-test.cpp clang/test/Analysis/castsize.c clang/test/Analysis/malloc-annotations.c clang/test/Analysis/malloc-sizeof.c clang/test/Analysis/malloc.c clang/test/Analysis/unix-fns.c clang/test/Sema/implicit-void-ptr-cast.c

View the diff from clang-format here.

diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index d2ffe9c2e..662395095 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -40,25 +40,25 @@
 #include <optional>
 
 namespace clang {
-  class AllocSizeAttr;
-  class APValue;
-  class ASTContext;
-  class BlockDecl;
-  class CXXBaseSpecifier;
-  class CXXMemberCallExpr;
-  class CXXOperatorCallExpr;
-  class CastExpr;
-  class Decl;
-  class IdentifierInfo;
-  class MaterializeTemporaryExpr;
-  class NamedDecl;
-  class ObjCPropertyRefExpr;
-  class OpaqueValueExpr;
-  class ParmVarDecl;
-  class StringLiteral;
-  class TargetInfo;
-  class ValueDecl;
-  class WarnUnusedResultAttr;
+class AllocSizeAttr;
+class APValue;
+class ASTContext;
+class BlockDecl;
+class CXXBaseSpecifier;
+class CXXMemberCallExpr;
+class CXXOperatorCallExpr;
+class CastExpr;
+class Decl;
+class IdentifierInfo;
+class MaterializeTemporaryExpr;
+class NamedDecl;
+class ObjCPropertyRefExpr;
+class OpaqueValueExpr;
+class ParmVarDecl;
+class StringLiteral;
+class TargetInfo;
+class ValueDecl;
+class WarnUnusedResultAttr;
 
 /// A simple array of base specifiers.
 typedef SmallVector<CXXBaseSpecifier*, 4> CXXCastPath;

[vvuksanovic]

Thanks for the review. Can someone merge this for me when the CI finishes? The formatting error is due to the indentation of existing code in Expr.h.

[vvuksanovic]

Updated to fix test on windows. Can you merge? CI failure seems to be unrelated to this PR.

[alexfh]

Hi @vvuksanovic, the new diagnostic produces some arguable warnings for a number of open-source projects. A few examples:

• https://android.googlesource.com/kernel/msm/+/refs/heads/android-msm-crosshatch-4.9-android10/tools/perf/builtin-top.c#313 - malloc(0) assigned to a char* along with a conditional subsequent realloc. In this particular case NULL could have been used instead of malloc(0), but char* is rather special in the sense that it's unlikely someone mistakenly allocates less than a byte of memory.
• https://github.com/storpipfugl/pykdtree/blob/master/pykdtree/_kdtree_core.c#L596 - this code intentionally allocates less memory for leaf nodes of a tree.
• https://projects.blender.org/blender/blender/src/branch/main/source/blender/blenkernel/intern/rigidbody.cc#L1606 - this code intentionally reallocates memory for zero elements.

There's more, but I didn't go through all of them. I have found some questionable code (i.e. it could be written more clearly and without triggering this diagnostic), but no real bugs so far.

My question is what could be done here to increase the signal-to-noise ratio? Maybe provide some ways to suppress this (without using pragmas)?

[vvuksanovic]

I think I will add an exception for zero size allocations. That is likely done on purpose and that change should eliminate a lot of the noise.

For the pykdtree case, that code is a bit questionable as it relies on not using a part of a structure. I think that emitting a warning there makes sense.

I am open to suggestions for ways to suppress the warning. Maybe we can only check implicit casts, but that might miss some actual problems. If there is a lot of noise, gating this diagnostic behind -Wextra like gcc does is an option.

[cmtice]

I seem to have found an issue with this warning. I have some code roughly like this:

if (var1 < 0)
return;

var2 = malloc(sizeof(...) * var1).

I'm getting warnings complaining that the malloc is assigning -8 instead of 8. The only way it could be -8 would be if var1 was negative. But if var1 is negative the malloc never gets executed (because of the return above). So it's warning in a case that can't happen.

[cmtice]

It also appears to have issues when the thing receiving the malloc is defined as a union, and the malloc allocates only the size of one of the smaller union options.

[vvuksanovic]

I seem to have found an issue with this warning. I have some code roughly like this:

if (var1 < 0) return;

var2 = malloc(sizeof(...) * var1).

I'm getting warnings complaining that the malloc is assigning -8 instead of 8. The only way it could be -8 would be if var1 was negative. But if var1 is negative the malloc never gets executed (because of the return above). So it's warning in a case that can't happen.

Do you have a concrete example of this? The size shouldn't get evaluated unless it is constant. The following example does show the warning:

const int x = -1;
if (x < 0)
    return;
int *ptr = malloc(sizeof(int) * x);

But only when x is marked const in which case the malloc size will always evaluate to a negative value (though never actually get executed).

It also appears to have issues when the thing receiving the malloc is defined as a union, and the malloc allocates only the size of one of the smaller union options.

I am not sure if that is defined behavior or not and I can't find any information about that. To me, warning in that case makes sense because dereferencing a larger field would access unallocated memory.

[NagyDonat]

This is a clear example for a bug type where the simple AST matching that can be done in a diagnostic warning is insufficient and the path-sensitive analysis of the clang static analyzer provides much better reports.

The AST pattern matching does not follow the control flow and cannot deduce things like "when the value is negative, this never gets executed" -- while the symbolic execution process of static analyzer is designed exactly for these considerations. (We could introduce with hacks like "if there is an if between these two code fragments, then inspect its condition and ...", but these would just "reinvent the wheel" and quickly become impossible to manage.)

Unfortunately the analyzer checkers are more costly (in terms of runtime) and less accessible than diagnostic warnings, but I still feel that it would be better to replace this diagnostic option with a static analyzer checker that can do the job properly. (In fact, the static analyzer already has the unix.Malloc checker which reports various bugs related to Malloc. If I recall correctly, zero-sized allocations are already modeled and reported when they are illegal; while negative-sized allocations are not currently covered but would be easy to handle.)

---

It also appears to have issues when the thing receiving the malloc is defined as a union, and the malloc allocates only the size of one of the smaller union options.

I am not sure if that is defined behavior or not and I can't find any information about that. To me, warning in that case makes sense because dereferencing a larger field would access unallocated memory.

Casts between pointer to union and pointer to a member of the union have well-defined behavior (second paragraph in linked section), so I'm sure that the following indirect approach doesn't trigger UB:

union u {
  short small;
  long big;
};
void f() {
  union u *p = (union u*)(short *)malloc(sizeof(short));
  p->small = 42;
  do_something_with(p);
  free(p);
}

Omitting the (short *) cast would clearly work in practice and I would presume that it's well-defined according to the standard, but I'm not entirely sure about this.

On the other hand, accessing a member of the union which is different from the one that was used to create it produces unspecified results. In your "dereferencing a larger field would access unallocated memory" example I would say that the problematic step is the one where the union is accessed through a member that is different from the first one. (It is fair to say that casting the small allocated memory to the union type is a suspicious step. However I would guess that this would only happen rarely and only in situations when this suspicious solution is intentional and needed, so reporting it as a compiler warning isn't helpful for the developer.)

[steakhal]

[...] (In fact, the static analyzer already has the unix.Malloc checker which reports various bugs related to Malloc. If I recall correctly, zero-sized allocations are already modeled and reported when they are illegal; while negative-sized allocations are not currently covered but would be easy to handle.)

I didn't read the discussion but I'm not sure how to interpret this highlighted sentence. malloc takes an unsigned parameter, thus it won't be ever negative. What I advocated for a long time to consider the malloc parameter as-if it was rsize_t (introduced by C11 Annex K N1570, which is basically size_t except that the most significant bit is never supposed to be set. Like passing a negative value to such API, it would go through a signed->unsigned conversion, thus set the MSB; thus its an effective way of detecting "negative" arguments))
Is it similar to what you have in mind?

[NagyDonat]

[...] while negative-sized allocations are not currently covered but would be easy to handle.)

I didn't read the discussion but I'm not sure how to interpret this highlighted sentence.

It's just a dumb mistake, I forgot that the parameter of malloc is unsigned.

[...] What I advocated for a long time to consider the malloc parameter as-if it was rsize_t (introduced by C11 Annex K N1570, which is basically size_t except that the most significant bit is never supposed to be set. Like passing a negative value to such API, it would go through a signed->unsigned conversion, thus set the MSB; thus its an effective way of detecting "negative" arguments)) Is it similar to what you have in mind?

I like this approach, and I would support activating it in our analysis – there is no non-buggy applications of these oversized allocations. In fact, we already have a check where we report an error if the allocation size is tainted and it can be larger than SIZE_MAX / 4. We could extend that (or something like it) for situations where the allocation size is non-tainted but definitely too large.