From ab36911f20a87982ae078e8413a9516d126e91bd Mon Sep 17 00:00:00 2001 From: John Kastner Date: Mon, 23 Aug 2021 15:58:20 -0400 Subject: [PATCH 1/3] Ensure bounds are not inserted into unwritable files --- clang/include/clang/3C/CtxSensAVarBounds.h | 3 + clang/lib/3C/AVarBoundsInfo.cpp | 32 ++++++--- clang/lib/3C/CtxSensAVarBounds.cpp | 51 +++++++------ clang/test/3C/base_subdir/unwritable_bounds.c | 71 +++++++++++++++++++ clang/test/3C/unwritable_bounds.h | 34 +++++++++ 5 files changed, 156 insertions(+), 35 deletions(-) create mode 100644 clang/test/3C/base_subdir/unwritable_bounds.c create mode 100644 clang/test/3C/unwritable_bounds.h diff --git a/clang/include/clang/3C/CtxSensAVarBounds.h b/clang/include/clang/3C/CtxSensAVarBounds.h index 11b41a880c0c..f9d4d7705d86 100644 --- a/clang/include/clang/3C/CtxSensAVarBounds.h +++ b/clang/include/clang/3C/CtxSensAVarBounds.h @@ -92,6 +92,9 @@ class CtxSensitiveBoundsKeyHandler { std::map &BKMap, bool IsGlobal); + void contextualizeCVar(RecordDecl *RD, std::string AccessKey, bool IsGlobal, + ASTContext *C, ProgramInfo &I); + // Get string that represents a context sensitive key for the struct // member access ME. std::string getCtxStructKey(MemberExpr *ME, ASTContext *C); diff --git a/clang/lib/3C/AVarBoundsInfo.cpp b/clang/lib/3C/AVarBoundsInfo.cpp index 0f8d0b6df5cf..a739344863a7 100644 --- a/clang/lib/3C/AVarBoundsInfo.cpp +++ b/clang/lib/3C/AVarBoundsInfo.cpp @@ -592,18 +592,37 @@ void PotentialBoundsInfo::addPotentialBoundsPOne( } bool AVarBoundsInfo::isValidBoundVariable(clang::Decl *D) { + if (D == nullptr) + return false; + + // Any pointer declaration in an unwritable file without existing bounds + // annotations is not valid. This ensures we do not add bounds onto pointers + // where attempting to rewrite the variable to insert the bound would be an + // error. If there are existing bounds, no new bound will be inferred, so no + // rewriting will be attempted. By leaving existing bounds as valid, these + // bounds can be used infer bounds on other (writeable) declarations. + // Non-pointer types are also still valid because these will never need bounds + // expression, and they need to remain valid so that they can be used by + // existing array pointer bounds. + auto PSL = PersistentSourceLoc::mkPSL(D, D->getASTContext()); + if (!canWrite(PSL.getFileName())) { + if (auto *DD = dyn_cast(D)) + return !DD->getType()->isPointerType() || DD->hasBoundsExpr(); + return false; + } + // All return and field values are valid bound variables. - if (D && (isa(D) || isa(D))) + if (isa(D) || isa(D)) return true; // For Parameters, check if they belong to a valid function. // Function pointer types are not considered valid functions, so function - // pointer parameters are are disqualified as valid bound variables here. - if (auto *PD = dyn_cast_or_null(D)) + // pointer parameters are disqualified as valid bound variables here. + if (auto *PD = dyn_cast(D)) return PD->getParentFunctionOrMethod() != nullptr; - // For VarDecls, check if these are are not dummy and have a name. - if (auto *VD = dyn_cast_or_null(D)) + // For VarDecls, check if these are not dummy and have a name. + if (auto *VD = dyn_cast(D)) return !VD->getNameAsString().empty(); return false; @@ -662,9 +681,6 @@ bool AVarBoundsInfo::tryGetVariable(clang::Expr *E, const ASTContext &C, } else if (DeclRefExpr *DRE = dyn_cast(E)) { auto *D = DRE->getDecl(); Ret = tryGetVariable(D, Res); - if (!Ret) { - assert(false && "Invalid declaration found inside bounds expression"); - } } else if (MemberExpr *ME = dyn_cast(E)) { return tryGetVariable(ME->getMemberDecl(), Res); } else { diff --git a/clang/lib/3C/CtxSensAVarBounds.cpp b/clang/lib/3C/CtxSensAVarBounds.cpp index 0224f56cf169..9ef28f4a1aaf 100644 --- a/clang/lib/3C/CtxSensAVarBounds.cpp +++ b/clang/lib/3C/CtxSensAVarBounds.cpp @@ -170,7 +170,7 @@ bool CtxSensitiveBoundsKeyHandler::tryGetFieldCSKey( FieldDecl *FD, CtxStKeyMap *CSK, const std::string &AK, ASTContext *C, ProgramInfo &I, BoundsKey &CSKey) { bool RetVal = false; - if (CSK->find(AK) != CSK->end()) { + if (ABI->isValidBoundVariable(FD) && CSK->find(AK) != CSK->end()) { CVarOption CV = I.getVariable(FD, C); BoundsKey OrigK; if (CV.hasValue() && CV.getValue().hasBoundsKey()) { @@ -225,44 +225,41 @@ void CtxSensitiveBoundsKeyHandler::contextualizeStructRecord( } } +void CtxSensitiveBoundsKeyHandler::contextualizeCVar(RecordDecl *RD, + std::string AccessKey, + bool IsGlobal, + ASTContext *C, + ProgramInfo &I) { + std::string FileName = PersistentSourceLoc::mkPSL(RD, *C).getFileName(); + if (canWrite(FileName)) { + // Context sensitive struct key map. + CtxStKeyMap *MECSMap = getCtxStKeyMap(IsGlobal); + auto &BKeyMap = (*MECSMap)[AccessKey]; + contextualizeStructRecord(I, C, RD, AccessKey, BKeyMap, IsGlobal); + } +} + void CtxSensitiveBoundsKeyHandler::contextualizeCVar(MemberExpr *ME, ASTContext *C, ProgramInfo &I) { FieldDecl *FD = dyn_cast_or_null(ME->getMemberDecl()); - if (FD != nullptr) { - RecordDecl *RD = FD->getParent(); - // If the base decl is not null. - if (RD != nullptr) { - // Get structure access key. - StructAccessVisitor SKV(C); - SKV.TraverseStmt(ME->getBase()->getExprStmt()); - std::string AK = SKV.getStructAccessKey(); - // Context sensitive struct key map. - CtxStKeyMap *MECSMap = getCtxStKeyMap(SKV.IsGlobal); - auto &BKeyMap = (*MECSMap)[AK]; - contextualizeStructRecord(I, C, RD, AK, BKeyMap, SKV.IsGlobal); - } + if (RecordDecl *RD = FD != nullptr ? FD->getParent() : nullptr) { + // Get structure access key. + StructAccessVisitor SKV(C); + SKV.TraverseStmt(ME->getBase()->getExprStmt()); + contextualizeCVar(RD, SKV.getStructAccessKey(), SKV.IsGlobal, C, I); } } void CtxSensitiveBoundsKeyHandler::contextualizeCVar(VarDecl *VD, ASTContext *C, ProgramInfo &I) { - const RecordType *RT = dyn_cast_or_null( - VD->getType()->getUnqualifiedDesugaredType()); - const RecordDecl *RD = nullptr; - if (RT != nullptr) { - RD = RT->getDecl(); - } - // If the base decl is not null. - if (RT != nullptr) { + const auto *RT = dyn_cast_or_null( + VD->getType()->getUnqualifiedDesugaredType()); + if (RecordDecl *RD = RT != nullptr ? RT->getDecl() : nullptr) { // Get structure access key. StructAccessVisitor SKV(C); SKV.processVarDecl(VD); - // Context sensitive struct key map. - CtxStKeyMap *MECSMap = getCtxStKeyMap(SKV.IsGlobal); - std::string AK = SKV.getStructAccessKey(); - auto &BKeyMap = (*MECSMap)[AK]; - contextualizeStructRecord(I, C, RD, AK, BKeyMap, SKV.IsGlobal); + contextualizeCVar(RD, SKV.getStructAccessKey(), SKV.IsGlobal, C, I); } } diff --git a/clang/test/3C/base_subdir/unwritable_bounds.c b/clang/test/3C/base_subdir/unwritable_bounds.c new file mode 100644 index 000000000000..5d9ac8cd9942 --- /dev/null +++ b/clang/test/3C/base_subdir/unwritable_bounds.c @@ -0,0 +1,71 @@ +// RUN: rm -rf %t* +// RUN: cd %S +// RUN: 3c -alltypes -addcr -output-dir=%t.checked/base_subdir %s -- + +// The functions called from this file would normally have bounds inferred if +// they were declared in a writable file. The file is not writable, so the new +// bounds must not be inferred. This possibility of this happening existed +// before 3C started inferring itypes on undefined functions, but it became a +// significant issue and was noticed with the introduction of this feature. + +#include "../unwritable_bounds.h" +void test_functions() { + { + char s[0]; + unwrite0(s); + } + { + char s[0]; + unwrite1(s); + } + { + char s[0]; + unwrite2(s); + } + { + char s[0]; + unwrite3(s); + } + { + char s[0]; + unwrite4(s); + } + { + char s[0]; + unwrite5(s); + } + { + char s[0]; + unwrite6(s); + } +} + +void test_struct() { + // There is a code path for variable declarations and a different path for + // uses of a variable. The initializer is required to test the declaration + // path. + struct arr_struct a = {}; + for (int i = 0; i < a.n; i++) + a.arr[i]; + + // I don't quite understand why this was a problem, but it caused an + // assertion to fail after apply the fix for the first struct test + // case. + union e { + float d + }; + int i = struct_ret()->c; + union e f; + f.d; +} + +void test_glob() { + for (int i = 0; i < 10; i++) + glob0[i]; + + for (int i = 0; i < 10; i++) + glob1[i]; + + for (int i = 0; i < 10; i++) + glob3[i]; +} diff --git a/clang/test/3C/unwritable_bounds.h b/clang/test/3C/unwritable_bounds.h new file mode 100644 index 000000000000..898e373b7869 --- /dev/null +++ b/clang/test/3C/unwritable_bounds.h @@ -0,0 +1,34 @@ +// Used by base_subdir/unwritable_bounds.c . + +void unwrite0(void *p : itype(_Array_ptr)); +void unwrite1(char *p); +void unwrite2(char *p : itype(_Array_ptr)); +void unwrite3(_Array_ptr p); + +void unwrite4(char *p) { + for (int i = 0; i < 10; i++) + p[i]; +} +void unwrite5(char *p : itype(_Array_ptr)) { + for (int i = 0; i < 10; i++) + p[i]; +} +void unwrite6(_Array_ptr p) { + for (int i = 0; i < 10; i++) + p[i]; +} + +struct arr_struct { + int *arr; + int n; +}; + +struct other_struct { + char *c; +}; +struct other_struct *struct_ret(); + +int *glob0; +int *glob1 : itype(_Array_ptr); +int *glob2 : count(10); +int *glob3 : itype(_Ptr); From 3c46f1426a9eaee42901024479419b97cfd7334b Mon Sep 17 00:00:00 2001 From: John Kastner Date: Thu, 26 Aug 2021 16:36:35 -0400 Subject: [PATCH 2/3] Tidy --- clang/lib/3C/AVarBoundsInfo.cpp | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/clang/lib/3C/AVarBoundsInfo.cpp b/clang/lib/3C/AVarBoundsInfo.cpp index a739344863a7..a13ff98cde12 100644 --- a/clang/lib/3C/AVarBoundsInfo.cpp +++ b/clang/lib/3C/AVarBoundsInfo.cpp @@ -670,24 +670,25 @@ bool AVarBoundsInfo::tryGetVariable(clang::Decl *D, BoundsKey &R) { bool AVarBoundsInfo::tryGetVariable(clang::Expr *E, const ASTContext &C, BoundsKey &Res) { - Optional OptConsVal; - bool Ret = false; if (E != nullptr) { E = E->IgnoreParenCasts(); - if (E->getType()->isArithmeticType() && - (OptConsVal = E->getIntegerConstantExpr(C))) { + + // Get the BoundsKey for the constant value if the expression is a constant + // integer expression. + Optional OptConsVal = E->getIntegerConstantExpr(C); + if (E->getType()->isArithmeticType() && OptConsVal.hasValue()) { Res = getVarKey(*OptConsVal); - Ret = true; - } else if (DeclRefExpr *DRE = dyn_cast(E)) { - auto *D = DRE->getDecl(); - Ret = tryGetVariable(D, Res); - } else if (MemberExpr *ME = dyn_cast(E)) { - return tryGetVariable(ME->getMemberDecl(), Res); - } else { - // assert(false && "Variable inside bounds declaration is an expression"); + return true; } + + // For declarations or struct member references, get the bounds key for the + // references variables or field. + if (auto *DRE = dyn_cast(E)) + return tryGetVariable(DRE->getDecl(), Res); + if (auto *ME = dyn_cast(E)) + return tryGetVariable(ME->getMemberDecl(), Res); } - return Ret; + return false; } // Merging bounds B with the present bounds of key L at the same priority P From 364cb0e9edc7acc9cb9c62bb0b4751c983014b15 Mon Sep 17 00:00:00 2001 From: John Kastner Date: Thu, 26 Aug 2021 16:49:58 -0400 Subject: [PATCH 3/3] Fix test case --- clang/test/3C/base_subdir/unwritable_bounds.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clang/test/3C/base_subdir/unwritable_bounds.c b/clang/test/3C/base_subdir/unwritable_bounds.c index 5d9ac8cd9942..7636ea1006f5 100644 --- a/clang/test/3C/base_subdir/unwritable_bounds.c +++ b/clang/test/3C/base_subdir/unwritable_bounds.c @@ -66,6 +66,9 @@ void test_glob() { for (int i = 0; i < 10; i++) glob1[i]; + for (int i = 0; i < 10; i++) + glob2[i]; + for (int i = 0; i < 10; i++) glob3[i]; }