llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Radovan Božić (bozicrHT) <details> <summary>Changes</summary> `ArrayBoundChecker` reports false positives for `container-of`-style pointer arithmetic. The pointer starts at an embedded field and subtracts the field offset to recover the containing structure. `computeOffset()` follows consecutive `ElementRegion`s. Once it reaches the `FieldRegion`, traversal ends and the embedded field becomes the bounds owner. The negative adjustment is therefore checked relative to the field instead of the containing structure. The discussion around issue #<!-- -->104771 considers broader solutions. This patch takes a narrower, checker-local approach. It does not change expression tracking or the analyzer's value model. When the region hierarchy proves that: - the pointer originated from a direct field; - the result is viewed as the field's containing record; - the negative character offset exactly matches the field's ABI offset; - the backing storage can contain the record; the checker continues bounds calculation from the field's parent region. Fixes #<!-- -->104771 --- Full diff: https://github.com/llvm/llvm-project/pull/214140.diff 2 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp (+175) - (added) clang/test/Analysis/ArrayBound/container-of.c (+326) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 67110f021bc56..f2e1b0b008bc4 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -192,6 +192,171 @@ class ArrayBoundChecker : public Checker<check::PostStmt<ArraySubscriptExpr>, } // anonymous namespace +static QualType getRegionObjectType(const MemRegion *Region) { + if (const auto *TVR = dyn_cast<TypedValueRegion>(Region)) + return TVR->getValueType(); + if (const auto *SR = dyn_cast<SymbolicRegion>(Region)) + return SR->getPointeeStaticType(); + return {}; +} + +/// Return true when the region containing \p ContainerRegion has type +/// \p ContainerType. ElementRegion represents both array elements and casts, +/// so the type of ContainerRegion itself is not sufficient evidence. +static bool hasContainerTypeProvenance(const SubRegion *ContainerRegion, + QualType ContainerType, + ASTContext &Ctx) { + const MemRegion *StorageRegion = ContainerRegion; + if (const auto *ER = dyn_cast<ElementRegion>(ContainerRegion)) { + if (!ASTContext::hasSameUnqualifiedType(ER->getElementType(), + ContainerType)) + return false; + StorageRegion = ER->getSuperRegion(); + } + + QualType StorageType = getRegionObjectType(StorageRegion); + if (StorageType.isNull()) + return false; + + if (const ArrayType *AT = Ctx.getAsArrayType(StorageType)) + StorageType = AT->getElementType(); + + return ASTContext::hasSameUnqualifiedType(StorageType, ContainerType); +} + +/// Return whether the concrete storage containing \p ContainerRegion is large +/// enough to contain an object of \p ContainerType at that region's offset. +/// Return std::nullopt when either the offset or the extent is symbolic. +static std::optional<bool> +hasSufficientContainerExtent(ProgramStateRef State, + const SubRegion *ContainerRegion, + QualType ContainerType, SValBuilder &SVB) { + ASTContext &Ctx = SVB.getContext(); + RegionOffset Offset = ContainerRegion->getAsOffset(); + if (!Offset.isValid() || Offset.hasSymbolicOffset()) + return std::nullopt; + + const int64_t OffsetBits = Offset.getOffset(); + const uint64_t CharWidth = Ctx.getCharWidth(); + if (OffsetBits < 0 || static_cast<uint64_t>(OffsetBits) % CharWidth != 0) + return false; + + const MemRegion *BaseRegion = Offset.getRegion(); + const auto BaseExtent = + getDynamicExtent(State, BaseRegion, SVB).getAs<nonloc::ConcreteInt>(); + if (!BaseExtent) + return std::nullopt; + + const int64_t ContainerSize = + Ctx.getTypeSizeInChars(ContainerType).getQuantity(); + if (ContainerSize < 0) + return false; + + const uint64_t OffsetChars = static_cast<uint64_t>(OffsetBits) / CharWidth; + const uint64_t ContainerSizeChars = static_cast<uint64_t>(ContainerSize); + if (OffsetChars > std::numeric_limits<uint64_t>::max() - ContainerSizeChars) + return false; + + const uint64_t RequiredExtent = OffsetChars + ContainerSizeChars; + const llvm::APSInt RequiredExtentValue = + llvm::APSInt::getUnsigned(RequiredExtent); + return llvm::APSInt::compareValues(*BaseExtent->getValue(), + RequiredExtentValue) >= 0; +} + +/// Recognize the region shape produced when a pointer to a direct field is +/// adjusted back to the beginning of its containing record. For example, +/// +/// (struct Parent *)((char *)&P.Field - offsetof(struct Parent, Field)) +/// +/// is represented as: +/// +/// ElementRegion<Parent, 0> +/// ElementRegion<char, -offsetof(Parent, Field)> +/// FieldRegion<Parent::Field> +/// <region for P> +/// +/// The character ElementRegion is absent when the field offset is zero. Return +/// the region for P only when the record type, field declaration, target ABI +/// layout, and underlying storage prove that the adjustment lands exactly at +/// the beginning of P. +static const SubRegion * +getContainerOfParentRegion(const ElementRegion *ContainerER, + ProgramStateRef State, SValBuilder &SVB) { + ASTContext &Ctx = SVB.getContext(); + const MemRegion *SuperRegion = ContainerER->getSuperRegion(); + const FieldRegion *FieldR = nullptr; + int64_t CharacterIndex = 0; + + if (const auto *CharacterER = dyn_cast<ElementRegion>(SuperRegion)) { + QualType CharacterType = CharacterER->getElementType(); + if (!CharacterType->isCharType() || + Ctx.getTypeSizeInChars(CharacterType).getQuantity() != 1) + return nullptr; + + const auto ConcreteIndex = + CharacterER->getIndex().getAs<nonloc::ConcreteInt>(); + if (!ConcreteIndex) + return nullptr; + + std::optional<int64_t> Index = ConcreteIndex->getValue()->tryExtValue(); + if (!Index) + return nullptr; + CharacterIndex = *Index; + + FieldR = dyn_cast<FieldRegion>(CharacterER->getSuperRegion()); + } else { + // SValBuilder folds an adjustment of zero, so a first field is represented + // without an intermediate character ElementRegion. + FieldR = dyn_cast<FieldRegion>(SuperRegion); + } + + if (!FieldR) + return nullptr; + + const FieldDecl *Field = FieldR->getDecl(); + if (Field->isBitField()) + return nullptr; + + QualType ContainerType = + ContainerER->getElementType().getCanonicalType().getUnqualifiedType(); + const auto *ContainerRT = ContainerType->getAs<RecordType>(); + if (!ContainerRT) + return nullptr; + + const RecordDecl *FieldParent = Field->getParent(); + if (!FieldParent || !FieldParent->isCompleteDefinition() || + ContainerRT->getDecl()->getCanonicalDecl() != + FieldParent->getCanonicalDecl()) + return nullptr; + + const uint64_t FieldOffsetBits = Ctx.getFieldOffset(Field); + const uint64_t CharWidth = Ctx.getCharWidth(); + if (FieldOffsetBits % CharWidth != 0 || CharacterIndex > 0) + return nullptr; + + // Avoid negating INT64_MIN while comparing the signed character index with + // the unsigned ABI field offset. + const uint64_t BackwardOffset = + static_cast<uint64_t>(-(CharacterIndex + 1)) + 1; + if (BackwardOffset != FieldOffsetBits / CharWidth) + return nullptr; + + const auto *ParentRegion = dyn_cast<SubRegion>(FieldR->getSuperRegion()); + if (!ParentRegion) + return nullptr; + + std::optional<bool> HasSufficientExtent = + hasSufficientContainerExtent(State, ParentRegion, ContainerType, SVB); + if (HasSufficientExtent && !*HasSufficientExtent) + return nullptr; + if (!HasSufficientExtent && + !hasContainerTypeProvenance(ParentRegion, ContainerType, Ctx)) + return nullptr; + + return ParentRegion; +} + /// For a given Location that can be represented as a symbolic expression /// Arr[Idx] (or perhaps Arr[Idx1][Idx2] etc.), return the parent memory block /// Arr and the distance of Location from the beginning of Arr (expressed in a @@ -236,6 +401,16 @@ computeOffset(ProgramStateRef State, SValBuilder &SVB, SVal Location) { if (!Offset) return std::nullopt; + if (const SubRegion *ParentRegion = + getContainerOfParentRegion(CurRegion, State, SVB)) { + // The negative character offset exactly cancels the field's offset in + // its parent record. Continue from the parent so that an enclosing array + // (if any) remains the bounds owner. + OwnerRegion = ParentRegion; + CurRegion = dyn_cast<ElementRegion>(OwnerRegion); + continue; + } + OwnerRegion = CurRegion->getSuperRegion()->getAs<SubRegion>(); // When this is just another ElementRegion layer, we need to continue the // offset calculations: diff --git a/clang/test/Analysis/ArrayBound/container-of.c b/clang/test/Analysis/ArrayBound/container-of.c new file mode 100644 index 0000000000000..a1a997f0f1cf7 --- /dev/null +++ b/clang/test/Analysis/ArrayBound/container-of.c @@ -0,0 +1,326 @@ +// RUN: %clang_analyze_cc1 -Wno-array-bounds -Wno-address-of-packed-member \ +// RUN: -analyzer-checker=core,security.ArrayBound,unix.Malloc \ +// RUN: -verify %s +// + +#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER) +#define container_of(PTR, TYPE, MEMBER) \ + ((TYPE *)((char *)(PTR) - offsetof(TYPE, MEMBER))) +#define container_of_uchar(PTR, TYPE, MEMBER) \ + ((TYPE *)((unsigned char *)(PTR) - offsetof(TYPE, MEMBER))) +#define container_of_typed(PTR, TYPE, MEMBER) ({ \ + const __typeof__(((TYPE *)0)->MEMBER) *__member_ptr = (PTR); \ + (TYPE *)((char *)__member_ptr - offsetof(TYPE, MEMBER)); \ +}) + +void *malloc(__SIZE_TYPE__); +void free(void *); + +struct Test { + int a; + int b; +}; + +static void update_a(int *b) { + struct Test *head = container_of_typed(b, struct Test, b); + head->a = 10; // no-warning +} + +void scalar_member(void) { + struct Test object = {0}; + update_a(&object.b); +} + +struct Child { + int value; +}; + +struct Parent { + int id; + struct Child child; + int tail; +}; + +static void set_id(struct Child *child) { + struct Parent *parent = + container_of_typed(child, struct Parent, child); + parent->id = 1; // no-warning +} + +void direct_member(void) { + struct Parent object = {0}; + set_id(&object.child); +} + +static int read_tail(struct Child *child) { + struct Parent *parent = container_of(child, struct Parent, child); + return parent->tail; // no-warning +} + +struct Holder { + struct Parent *parent; +}; + +int symbolic_parent(struct Holder *holder) { + return read_tail(&holder->parent->child); // no-warning +} + +struct PathList { + int flags; +}; + +struct Route { + char pad[56]; + struct PathList pathlist; + void *head; +}; + +struct QueuedRoute { + struct Route *route; +}; + +static void bind_pathlist(struct PathList *pathlist) { + struct Route *route = container_of(pathlist, struct Route, pathlist); + if (route->head) // no-warning + (void)0; +} + +void symbolic_field_parent(struct QueuedRoute *queued) { + bind_pathlist(&queued->route->pathlist); +} + +struct GrandParent { + int prefix; + struct Parent parent; +}; + +int nested_parent(void) { + struct GrandParent object = {0}; + struct Parent *parent = + container_of(&object.parent.child, struct Parent, child); + return parent->tail; // no-warning +} + +void containing_array(void) { + struct Parent objects[2] = {0}; + struct Parent *parent = + container_of(&objects[0].child, struct Parent, child); + parent[1].tail = 1; // no-warning +} + +void containing_array_from_second_element(void) { + struct Parent objects[2] = {0}; + struct Parent *parent = + container_of(&objects[1].child, struct Parent, child); + (parent - 1)->id = 1; // no-warning +} + +struct FirstMember { + struct Child child; + int tail; +}; + +int zero_offset_field(void) { + struct FirstMember object = {0}; + struct FirstMember *parent = + container_of(&object.child, struct FirstMember, child); + return parent->tail; // no-warning +} + +void zero_offset_containing_array(void) { + struct FirstMember objects[2] = {0}; + struct FirstMember *parent = + container_of(&objects[0].child, struct FirstMember, child); + parent[1].tail = 1; // no-warning +} + +union ParentUnion { + struct Child child; + int value; +}; + +void union_containing_array(void) { + union ParentUnion objects[2] = {0}; + union ParentUnion *parent = + container_of(&objects[0].child, union ParentUnion, child); + parent[1].value = 1; // no-warning +} + +struct PackedParent { + char tag; + struct Child child; + int tail; +} __attribute__((packed)); + +int packed_parent(void) { + struct PackedParent object = {0}; + struct PackedParent *parent = + container_of(&object.child, struct PackedParent, child); + return parent->tail; // no-warning +} + +int unsigned_character_arithmetic(void) { + struct Parent object = {0}; + struct Parent *parent = + container_of_uchar(&object.child, struct Parent, child); + return parent->tail; // no-warning +} + +int sufficient_raw_storage(void) { + unsigned char storage[sizeof(struct Parent)] = {0}; + struct Parent *object = (struct Parent *)storage; + struct Parent *parent = + container_of(&object->child, struct Parent, child); + parent->tail = 1; // no-warning + return parent->tail; // no-warning +} + +int sufficient_heap_storage(void) { + struct Parent *object = (struct Parent *)malloc(sizeof(*object)); + if (!object) + return 0; + + struct Parent *parent = + container_of(&object->child, struct Parent, child); + parent->tail = 1; // no-warning + int result = parent->tail; // no-warning + free(object); + return result; +} + +struct ForwardParent; +struct ForwardParent { + int id; + struct Child child; +}; + +int forward_declared_parent(void) { + struct ForwardParent object = {0}; + struct ForwardParent *parent = + container_of(&object.child, struct ForwardParent, child); + return parent->id; // no-warning +} + +int split_adjustment(void) { + struct Parent object = {0}; + char *address = (char *)&object.child; + address -= offsetof(struct Parent, child); + struct Parent *parent = (struct Parent *)address; + return parent->tail; // no-warning +} + +// The matcher relies on region provenance and the ABI field offset, not on an +// OffsetOfExpr surviving in the subtraction expression. +enum { ParentChildOffset = offsetof(struct Parent, child) }; + +int saved_offset_constant(void) { + struct Parent object = {0}; + struct Parent *parent = + (struct Parent *)((char *)&object.child - ParentChildOffset); + return parent->tail; // no-warning +} + +int off_by_one_before_parent(void) { + struct Parent object = {0}; + struct Parent *parent = + (struct Parent *)((char *)&object.child - + offsetof(struct Parent, child) - 1); + return parent->id; // expected-warning{{Out of bound access to memory}} +} + +struct OtherParent { + int prefix[2]; + struct Child child; + int tail; +}; + +int wrong_parent_type(void) { + struct Parent object = {0}; + struct OtherParent *parent = + container_of(&object.child, struct OtherParent, child); + return parent->prefix[0]; // expected-warning{{Out of bound access to memory}} +} + +int raw_storage_with_sufficient_extent(void) { + unsigned char storage[sizeof(struct Parent)] = {0}; + struct Parent *fake_parent = (struct Parent *)storage; + struct Parent *parent = + container_of(&fake_parent->child, struct Parent, child); + return parent->tail; // no-warning +} + +int unrelated_storage(void) { + int storage = 0; + struct Parent *fake_parent = (struct Parent *)&storage; + struct Parent *parent = + container_of(&fake_parent->child, struct Parent, child); + return parent->tail; // expected-warning{{Out of bound access to memory}} +} + +int insufficient_raw_storage(void) { + unsigned char storage[sizeof(struct Parent) - 1] = {0}; + struct Parent *fake_parent = (struct Parent *)storage; + struct Parent *parent = + container_of(&fake_parent->child, struct Parent, child); + return parent->tail; // expected-warning{{Out of bound access to memory}} +} + +int insufficient_heap_storage(void) { + struct Parent *object = (struct Parent *)malloc(sizeof(*object) - 1); + // expected-warning@-1{{allocation of insufficient size}} + if (!object) + return 0; + + struct Parent *parent = + container_of(&object->child, struct Parent, child); + parent->tail = 1; // expected-warning{{Out of bound access to memory}} + free(object); + return 0; +} + +int standalone_child(void) { + struct Child child = {0}; + struct Parent *parent = container_of(&child, struct Parent, child); + parent->id = 1; // expected-warning{{Out of bound access to memory}} + return 0; +} + +int unrelated_storage_zero_offset(void) { + int storage = 0; + struct FirstMember *fake_parent = (struct FirstMember *)&storage; + struct FirstMember *parent = + container_of(&fake_parent->child, struct FirstMember, child); + return parent[1].tail; // expected-warning{{Out of bound access to memory}} +} + +struct TwoChildren { + int id; + struct Child first; + struct Child second; +}; + +int wrong_member_offset(void) { + struct TwoChildren object = {0}; + struct TwoChildren *parent = + container_of(&object.first, struct TwoChildren, second); + return parent->id; // expected-warning{{Out of bound access to memory}} +} + +int before_reconstructed_parent(void) { + struct Parent object = {0}; + struct Parent *parent = container_of(&object.child, struct Parent, child); + return (parent - 1)->id; // expected-warning{{Out of bound access to memory}} +} + +int after_reconstructed_parent(void) { + struct Parent object = {0}; + struct Parent *parent = container_of(&object.child, struct Parent, child); + return (parent + 1)->id; // expected-warning{{Out of bound access to memory}} +} + +int after_containing_array(void) { + struct Parent objects[2] = {0}; + struct Parent *parent = + container_of(&objects[0].child, struct Parent, child); + return parent[2].id; // expected-warning{{Out of bound access to memory}} +} `````````` </details> https://github.com/llvm/llvm-project/pull/214140 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
