================
@@ -57,92 +57,147 @@ getAsCleanArraySubscriptExpr(const Expr *E, const
CheckerContext &C) {
return ASE;
}
-/// If `E` is a "clean" array subscript expression, return the type of the
-/// accessed element; otherwise return std::nullopt because that's the best (or
-/// least bad) option for the diagnostic generation that relies on this.
-static std::optional<QualType> determineElementType(const Expr *E,
- const CheckerContext &C) {
- const auto *ASE = getAsCleanArraySubscriptExpr(E, C);
- if (!ASE)
- return std::nullopt;
+class SizeUnit {
+ QualType AsType;
+ int64_t AsCharUnits;
- return ASE->getType();
-}
+ SizeUnit() : AsType(), AsCharUnits(1) {}
-static std::optional<int64_t>
-determineElementSize(const std::optional<QualType> T, const CheckerContext &C)
{
- if (!T)
- return std::nullopt;
- return C.getASTContext().getTypeSizeInChars(*T).getQuantity();
-}
+public:
+ SizeUnit(QualType T, const ASTContext &ACtx)
+ : AsType(T), AsCharUnits(ACtx.getTypeSizeInChars(T).getQuantity()) {
+ assert(!T.isNull());
+ }
-class StateUpdateReporter {
- const MemSpaceRegion *Space;
- const SubRegion *Reg;
- const NonLoc ByteOffsetVal;
- const std::optional<QualType> ElementType;
- const std::optional<int64_t> ElementSize;
- bool AssumedNonNegative = false;
- std::optional<NonLoc> AssumedUpperBound = std::nullopt;
+ static SizeUnit bytes() { return SizeUnit(); }
-public:
- StateUpdateReporter(const SubRegion *R, NonLoc ByteOffsVal, const Expr *E,
- CheckerContext &C)
- : Space(R->getMemorySpace(C.getState())), Reg(R),
- ByteOffsetVal(ByteOffsVal), ElementType(determineElementType(E, C)),
- ElementSize(determineElementSize(ElementType, C)) {}
+ bool isBytes() const { return AsType.isNull(); }
+
+ /// Return the element type that is "natural" for reporting out-of-bounds
+ /// memory access to 'Location'.
+ static SizeUnit forSVal(SVal Location, const ASTContext &ACtx) {
+ if (const auto *R = Location.getAsRegion()->getAs<TypedValueRegion>())
+ return SizeUnit(R->getValueType(), ACtx);
+ return bytes();
+ }
- void recordNonNegativeAssumption() { AssumedNonNegative = true; }
- void recordUpperBoundAssumption(NonLoc UpperBoundVal) {
- AssumedUpperBound = UpperBoundVal;
+ /// If `E` is a "clean" array subscript expression, return the type of the
+ /// accessed element; otherwise return 'Bytes' because that's the best (or
+ /// least bad) option for the assumption messages that use this.
+ /// FIXME: It is unfortunate that this heuristic differs from the heuristic
+ /// used for reporting assumption; but this difference is currently needed
+ /// due to the unfortunate phrasing of the assumption messages.
+ /// Get rid of this when the assumption note is rephrased and improved.
+ static SizeUnit forExpr(const Expr *E, const CheckerContext &C) {
+ const auto *ASE = getAsCleanArraySubscriptExpr(E, C);
+ if (!ASE)
+ return bytes();
+
+ return SizeUnit(ASE->getType(), C.getASTContext());
}
- bool assumedNonNegative() { return AssumedNonNegative; }
+ int64_t asCharUnits() const { return AsCharUnits; }
- const NoteTag *createNoteTag(CheckerContext &C) const;
+ bool canExpress(std::optional<int64_t> Val) const {
+ return asCharUnits() && (!Val || !(*Val % asCharUnits()));
+ }
-private:
- std::string getMessage(PathSensitiveBugReport &BR) const;
-
- /// Return true if information about the value of `Sym` can put constraints
- /// on some symbol which is interesting within the bug report `BR`.
- /// In particular, this returns true when `Sym` is interesting within `BR`;
- /// but it also returns true if `Sym` is an expression that contains integer
- /// constants and a single symbolic operand which is interesting (in `BR`).
- /// We need to use this instead of plain `BR.isInteresting()` because if we
- /// are analyzing code like
- /// int array[10];
- /// int f(int arg) {
- /// return array[arg] && array[arg + 10];
- /// }
- /// then the byte offsets are `arg * 4` and `(arg + 10) * 4`, which are not
- /// sub-expressions of each other (but `getSimplifiedOffsets` is smart enough
- /// to detect this out of bounds access).
- static bool providesInformationAboutInteresting(SymbolRef Sym,
- PathSensitiveBugReport &BR);
- static bool providesInformationAboutInteresting(SVal SV,
- PathSensitiveBugReport &BR) {
- return providesInformationAboutInteresting(SV.getAsSymbol(), BR);
+ std::string asExtentDesc() const {
+ if (isBytes())
+ return "the extent of";
+ return formatv("the number of '{0}' elements in", AsType.getAsString());
+ }
+
+ std::string asElementName() const {
+ if (isBytes())
+ return "byte";
+ return formatv("'{0}' element", AsType.getAsString());
}
};
-struct Messages {
- std::string Short, Full;
+} // anonymous namespace
+
+namespace clang::ento::bounds {
+
+struct CheckFlags {
+ unsigned CheckUnderflow : 1;
+ unsigned OffsetObviouslyNonnegative : 1;
+ unsigned AcceptPastTheEnd : 1;
};
-enum class BadOffsetKind { Negative, Overflowing, Indeterminate };
+class CheckResult;
-constexpr llvm::StringLiteral Adjectives[] = {"a negative", "an overflowing",
- "a negative or overflowing"};
-static StringRef asAdjective(BadOffsetKind Problem) {
- return Adjectives[static_cast<int>(Problem)];
-}
+CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset,
+ std::optional<NonLoc> Extent, CheckFlags Flags);
-constexpr llvm::StringLiteral Prepositions[] = {"preceding", "after the end
of",
- "around"};
-static StringRef asPreposition(BadOffsetKind Problem) {
- return Prepositions[static_cast<int>(Problem)];
-}
+class CheckResult {
+public:
+ /// When true, the bounds check noticed that the value of an unsigned
+ /// expression is constrained to negative values (because the analyzer
+ /// skipped the modeling of a cast expression). This execution path must be
+ /// discarded because it does not represent a real possibility.
+ /// FIXME: This hack is currently needed to filter out many ugly false
+ /// positives; but it should be removed when we fix cast modeling.
+ bool isCorruptedState() const { return IsCorruptedState; }
+
+ /// When true, the checked offset may be in bounds.
+ /// As an exceptional case, this is also true for idiomatic expressions that
+ /// define a past-the-end pointer (and do not dereference it).
+ bool mayBeValid() const { return static_cast<bool>(ValidState); }
+
+ /// When true, the checked offset may be negative.
+ bool mayUnderflow() const { return MayUnderflow; }
+ /// When true, the checked offset may be >= the extent of the region.
+ /// As an exceptional case, this is also false for idiomatic expressions that
+ /// define a past-the-end pointer (and do not dereference it).
+ bool mayOverflow() const { return MayOverflowExtent.has_value(); }
+ /// When true, the checked offset may be out of bounds.
+ bool mayBeInvalid() const { return MayUnderflow || MayOverflowExtent; }
+
+ /// Returns the offset of the accessed location from the beginning of the
+ /// accessd region.
+ NonLoc getOffset() const { return Offset; }
+
+ /// Returns the extent of the accessed region if it is relevant (because the
+ /// offset may overflow it), otherwise returns std::nullopt.
+ std::optional<NonLoc> getExtentIfRelevant() const {
+ return MayOverflowExtent;
+ }
+
+ /// Returns the program state that should be used for continuing the analysis
+ /// after this bounds check. This returns null if mayBeValid() is false, in
+ /// that case the state before the check should be used in the error node.
+ /// Note that we also have a valid state in the exception case when the
+ /// 'access' calculates the past-the-end pointer without dereferencing it.
+ ProgramStateRef getValidState() const { return ValidState; }
----------------
NagyDonat wrote:
> What is the exact assumption we are making here? Are we assuming `[begin,
> end]` or `[begin, end+1]`?
We are assuming `0 <= Offset` and `Offset < Extent`, where `Offset` is the
offset from the beginning of the memory region [1] and `Extent` is the extent
of the region. (As of now, `Offset` and `Extent` is measured in bytes, but I
may generalize them to allow indices / element counts.)
I will document this in a doc-comment for `checkBounds`.
[1] That, is if we have `char array[10]; char *p = array + 3; return p[3]` then
the `Offset` is 6.
> And is this a decision we can make for everyone or should this be controlled
> by the caller?
We need to make the decision that the lower bound because the workarounds that
compensate for the lack of proper modeling of signed/unsigned casts heavily
rely on the fact that the lower bound is 0.
For the upper bound `<` is a much better choice than `<=` because in the common
case the extent is a relatively simple symbol, while the last valid index
(`Extent - 1`) is usually represented as a `SymIntExpr` subtraction.
> If we want this to be reusable, I wonder if this needs to be user controlled.
This is an "is the accessed memory location inside of this memory region"
check, not an "is this integer inside of this range of integers" check. This
interface is sufficient for the (currently alpha) checkers that try to perform
"is this memory access" checks.
I do not intend to generalize this code for "is this integer inside of this
range of integers" checks because the core feature of this bounds checking code
is aggressive simplification of the inequality, which is justified by the fact
that simple operations on memory offset/extent values cannot overflow
`ssize_t`. Checkers that need a generic range check should just perform two
`evalBinOp` calls with their preferred comparison operators.
> If the only difference is the past end behaviour, I wonder if "InRange" might
> be a way to capture that, and you can define that to include the past end
> pointer.
>
> Similarly, `mayBeInRange` would be more descriptive for me than `mayBeValid`.
Actually now that I reconsider this, I'm leaning towards the names
`getInBoundsState` and `mayBeInBounds`. I agree that consistently using a
`InXXX` name is better than saying `Valid`, but prefer `InBounds` over
`InRange` because this is a memory bounds check and not a generic number range
check. (Compared to your original proposal, `getAssumedInBoundState` I think it
is valuable that `getInBoundsState` is shorter.)
https://github.com/llvm/llvm-project/pull/210774
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
