[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
ebevhan created this revision.
ebevhan added reviewers: dergachev.a, dcoughlin.
Herald added subscribers: cfe-commits, a.sidorin, szepet, xazax.hun.
Herald added a reviewer: george.karpenkov.
RegionStoreManager::getSizeInElements used 'int' for size
calculations, and ProgramState::assumeInBound fell back
to 'int' as well for its index calculations. This causes
truncation for sufficiently large sizes/indexes.
Use a signed size_t and ArrayIndexTy respectively to
prevent these problems.
Repository:
rC Clang
https://reviews.llvm.org/D46944
Files:
lib/StaticAnalyzer/Core/ProgramState.cpp
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/array-index.c
Index: test/Analysis/array-index.c
===
--- /dev/null
+++ test/Analysis/array-index.c
@@ -0,0 +1,21 @@
+// RUN: %clang_analyze_cc1
-analyzer-checker=core,alpha.security.ArrayBound,alpha.unix.cstring.OutOfBounds
-verify -Wno-implicit-function-declaration %s
+
+// expected-no-diagnostics
+
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void fie() {
+ buf[SIZE-1] = 1;
+}
+
+void foo() {
+ memcpy(buf, addr, size);
+}
+
+void bar() {
+ memcpy(addr, buf, size);
+}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,7 @@
// If a variable is reinterpreted as a type that doesn't fit into a larger
// type evenly, round it down.
// This is a signed value, since it's used in arithmetic with signed indices.
- return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+ return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType());
}
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
// Get the offset: the minimum value of the array index type.
BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
- // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
// Adjust the index.
Index: test/Analysis/array-index.c
===
--- /dev/null
+++ test/Analysis/array-index.c
@@ -0,0 +1,21 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.ArrayBound,alpha.unix.cstring.OutOfBounds -verify -Wno-implicit-function-declaration %s
+
+// expected-no-diagnostics
+
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void fie() {
+ buf[SIZE-1] = 1;
+}
+
+void foo() {
+ memcpy(buf, addr, size);
+}
+
+void bar() {
+ memcpy(addr, buf, size);
+}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,7 @@
// If a variable is reinterpreted as a type that doesn't fit into a larger
// type evenly, round it down.
// This is a signed value, since it's used in arithmetic with signed indices.
- return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+ return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType());
}
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
// Get the offset: the minimum value of the array index type.
BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
- // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
// Adjust the index.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added a comment. This is a nice extension of https://reviews.llvm.org/D16063. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType()); } I think we should initialize SValBuilder::ArrayIndexTy with getSignedSizeType() instead of LongLongTy and use `svalBuilder.getArrayIndexType()` here instead. Comment at: test/Analysis/array-index.c:1 +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.ArrayBound,alpha.unix.cstring.OutOfBounds -verify -Wno-implicit-function-declaration %s + Can we place these tests into Analysis/index-type.c? Repository: rC Clang https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added inline comments.
Comment at: test/Analysis/array-index.c:11
+
+void fie() {
+ buf[SIZE-1] = 1;
Could you please give meaningful names to the tests?
Repository:
rC Clang
https://reviews.llvm.org/D46944
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
ebevhan added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType()); } a.sidorin wrote: > I think we should initialize SValBuilder::ArrayIndexTy with > getSignedSizeType() instead of LongLongTy and use > `svalBuilder.getArrayIndexType()` here instead. I made the change, but it caused a spurious out of bounds warning in index-type.c for the 32-bit case. Making the type signed means that anything above MAX/2 will break, and the test uses arrays of that size. Repository: rC Clang https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType()); } ebevhan wrote: > a.sidorin wrote: > > I think we should initialize SValBuilder::ArrayIndexTy with > > getSignedSizeType() instead of LongLongTy and use > > `svalBuilder.getArrayIndexType()` here instead. > I made the change, but it caused a spurious out of bounds warning in > index-type.c for the 32-bit case. Making the type signed means that anything > above MAX/2 will break, and the test uses arrays of that size. Hm, yes. ssize_t is 32-bit on 32-bit targets but our indices can exceed it. Even if so, `svalBuilder.getArrayIndexType()` should be fine. Repository: rC Clang https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
ebevhan added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType()); } a.sidorin wrote: > ebevhan wrote: > > a.sidorin wrote: > > > I think we should initialize SValBuilder::ArrayIndexTy with > > > getSignedSizeType() instead of LongLongTy and use > > > `svalBuilder.getArrayIndexType()` here instead. > > I made the change, but it caused a spurious out of bounds warning in > > index-type.c for the 32-bit case. Making the type signed means that > > anything above MAX/2 will break, and the test uses arrays of that size. > Hm, yes. ssize_t is 32-bit on 32-bit targets but our indices can exceed it. > Even if so, `svalBuilder.getArrayIndexType()` should be fine. Well, the problem is that it's only enough for objects whose size is less than half of the range. If they're larger, the out of bounds calculation overflows and it thinks that we're trying to index outside the object (at a lower address). The 64-bit test avoids this since its array is smaller. It's MAX/16 instead. Repository: rC Clang https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
ebevhan updated this revision to Diff 147738.
ebevhan edited the summary of this revision.
ebevhan added a comment.
Made ArrayIndexTy into ssize_t, consolidated the tests and fixed the test that
was failing.
https://reviews.llvm.org/D46944
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
lib/StaticAnalyzer/Core/ProgramState.cpp
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/index-type.c
Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -6,15 +6,34 @@
#ifdef M32
-#define X86_ARRAY_SIZE (UINT_MAX/2 + 4)
+#define X86_ARRAY_SIZE (UINT_MAX/4 + 4)
void testIndexTooBig() {
char arr[X86_ARRAY_SIZE];
- char *ptr = arr + UINT_MAX/2;
+ char *ptr = arr + UINT_MAX/4;
ptr += 2; // index shouldn't overflow
*ptr = 42; // no-warning
}
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+ // not out of bounds
+ buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+ memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+ memcpy(addr, buf, size); // no-warning
+}
+
#else // 64-bit tests
#define ARRAY_SIZE 0x1
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
// If a variable is reinterpreted as a type that doesn't fit into a larger
// type evenly, round it down.
// This is a signed value, since it's used in arithmetic with signed indices.
- return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+ return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
}
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
// Get the offset: the minimum value of the array index type.
BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
- // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
// Adjust the index.
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -86,7 +86,7 @@
ProgramStateManager &stateMgr)
: Context(context), BasicVals(context, alloc),
SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
virtual ~SValBuilder() = default;
Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -6,15 +6,34 @@
#ifdef M32
-#define X86_ARRAY_SIZE (UINT_MAX/2 + 4)
+#define X86_ARRAY_SIZE (UINT_MAX/4 + 4)
void testIndexTooBig() {
char arr[X86_ARRAY_SIZE];
- char *ptr = arr + UINT_MAX/2;
+ char *ptr = arr + UINT_MAX/4;
ptr += 2; // index shouldn't overflow
*ptr = 42; // no-warning
}
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+ // not out of bounds
+ buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+ memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+ memcpy(addr, buf, size); // no-warning
+}
+
#else // 64-bit tests
#define ARRAY_SIZE 0x1
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
// If a variable is reinterpreted as a type that doesn't fit into a larger
// type evenly, round it down.
// This is a signed value, since it's used in arithmetic with signed indices.
- return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+ return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
}
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- li
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added a comment.
Hi Bevin,
Could you please address these comments?
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89
SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
As you correctly pointed, ssize_t is 32-bit on 32-bit systems. Therefore, it is
too short. So, we can leave this line as-is.
Comment at: test/Analysis/index-type.c:13
char arr[X86_ARRAY_SIZE];
- char *ptr = arr + UINT_MAX/2;
+ char *ptr = arr + UINT_MAX/4;
ptr += 2; // index shouldn't overflow
We don't need to fix the test - it is correct. We have to fix the type instead.
Comment at: test/Analysis/index-type.c:25
+void testOutOfBounds() {
+ // not out of bounds
+ buf[SIZE-1] = 1; // no-warning
The comments should be normal sentences: "Not out of bounds."
https://reviews.llvm.org/D46944
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
ebevhan added inline comments.
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89
SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
a.sidorin wrote:
> As you correctly pointed, ssize_t is 32-bit on 32-bit systems. Therefore, it
> is too short. So, we can leave this line as-is.
But if it's hardcoded to LongLongTy, you have the same problem on 64-bit
systems.
https://reviews.llvm.org/D46944
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added inline comments.
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89
SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
ebevhan wrote:
> a.sidorin wrote:
> > As you correctly pointed, ssize_t is 32-bit on 32-bit systems. Therefore,
> > it is too short. So, we can leave this line as-is.
> But if it's hardcoded to LongLongTy, you have the same problem on 64-bit
> systems.
Some reasons why LongLongTy is used here are listed in D16063. In brief, you
just cannot create an array of size greater than SIZE_MAX/2 on 64-bit
platforms.
https://reviews.llvm.org/D46944
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
ebevhan added inline comments.
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89
SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
a.sidorin wrote:
> ebevhan wrote:
> > a.sidorin wrote:
> > > As you correctly pointed, ssize_t is 32-bit on 32-bit systems. Therefore,
> > > it is too short. So, we can leave this line as-is.
> > But if it's hardcoded to LongLongTy, you have the same problem on 64-bit
> > systems.
> Some reasons why LongLongTy is used here are listed in D16063. In brief, you
> just cannot create an array of size greater than SIZE_MAX/2 on 64-bit
> platforms.
I don't think that's limited to 64-bit platforms, it applies to 32-bit ones as
well. I know that LLVM has issues with indexing arrays that are larger than
half of the address space in general due to limitations of GEP.
https://reviews.llvm.org/D46944
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added a subscriber: NoQ. a.sidorin added a comment. There are some results for clang and gcc max value for x86 and x64. Source code: const unsigned long long SIZE_MAX = (unsigned long long)(unsigned long)(-1); const unsigned long long size = SIZE_MAX/2; char arr[size+1]; Compiler output: % g++ -c cast-comp.cpp -m32 cast-comp.cpp:6:16: error: size of array ‘arr’ is negative char arr[size+1]; ^ % clang++-6.0 -c cast-comp.cpp -m32 % g++ -c cast-comp.cpp -m32 cast-comp.cpp:6:16: error: size of array ‘arr’ is negative char arr[size+1]; ^ % g++ -c cast-comp.cpp cast-comp.cpp:6:16: error: size of array ‘arr’ is negative char arr[size+1]; ^ % clang++-6.0 -c cast-comp.cpp cast-comp.cpp:6:10: error: array is too large (9223372036854775808 elements) char arr[size+1]; ^~ So, clang accepts indices > SIZE_MAX/2 for x86. For `arr[size]`, only clang-x64 fails with error. I think this means that we need to use LongLongTy as index type, not SizeType. @NoQ, what do you think? https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
ebevhan updated this revision to Diff 149415.
ebevhan edited the summary of this revision.
ebevhan added a comment.
Changed ArrayIndexTy back to LongLongTy and reverted the test change.
https://reviews.llvm.org/D46944
Files:
lib/StaticAnalyzer/Core/ProgramState.cpp
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/index-type.c
Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10
-analyzer-checker=core,alpha.security.ArrayBoundV2 -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10
-analyzer-checker=core,alpha.security.ArrayBoundV2 -DM32 -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10
-analyzer-checker=core,alpha.security.ArrayBoundV2
-Wno-implicit-function-declaration -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10
-analyzer-checker=core,alpha.security.ArrayBoundV2
-Wno-implicit-function-declaration -DM32 -verify %s
// expected-no-diagnostics
#define UINT_MAX (~0u)
@@ -36,4 +36,23 @@
*ptr = 42; // no-warning
}
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+ // Not out of bounds.
+ buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+ memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+ memcpy(addr, buf, size); // no-warning
+}
+
#endif
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
// If a variable is reinterpreted as a type that doesn't fit into a larger
// type evenly, round it down.
// This is a signed value, since it's used in arithmetic with signed indices.
- return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+ return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
}
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
// Get the offset: the minimum value of the array index type.
BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
- // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
// Adjust the index.
Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -DM32 -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -Wno-implicit-function-declaration -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -Wno-implicit-function-declaration -DM32 -verify %s
// expected-no-diagnostics
#define UINT_MAX (~0u)
@@ -36,4 +36,23 @@
*ptr = 42; // no-warning
}
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+ // Not out of bounds.
+ buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+ memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+ memcpy(addr, buf, size); // no-warning
+}
+
#endif
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
// If a variable is reinterpreted as a type that doesn't fit into a larger
// type evenly, round it down.
// This is a signed value, since it's used in arithmetic with signed indices.
- return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+ return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
}
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
// Get the offset: the minim
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
ebevhan added a comment. Herald added a subscriber: mikhail.ramalho. Ping. https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Hi Bevin, The patch looks good to me. But let's wait for maintainers to approve it. @NoQ , could you take a look? https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
NoQ accepted this revision. NoQ edited reviewers, added: NoQ; removed: dergachev.a. NoQ added a comment. Yep, this definitely looks safe and sound in the current shape. I'm also very sorry for the lack of attention. https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC335803: [analyzer] Use sufficiently large types for index
bounds calculation. (authored by dergachev, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D46944
Files:
lib/StaticAnalyzer/Core/ProgramState.cpp
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/index-type.c
Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10
-analyzer-checker=core,alpha.security.ArrayBoundV2 -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10
-analyzer-checker=core,alpha.security.ArrayBoundV2 -DM32 -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10
-analyzer-checker=core,alpha.security.ArrayBoundV2
-Wno-implicit-function-declaration -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10
-analyzer-checker=core,alpha.security.ArrayBoundV2
-Wno-implicit-function-declaration -DM32 -verify %s
// expected-no-diagnostics
#define UINT_MAX (~0u)
@@ -36,4 +36,23 @@
*ptr = 42; // no-warning
}
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+ // Not out of bounds.
+ buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+ memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+ memcpy(addr, buf, size); // no-warning
+}
+
#endif
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
// Get the offset: the minimum value of the array index type.
BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
- // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
// Adjust the index.
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
// If a variable is reinterpreted as a type that doesn't fit into a larger
// type evenly, round it down.
// This is a signed value, since it's used in arithmetic with signed indices.
- return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+ return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
}
//===--===//
Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -DM32 -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -Wno-implicit-function-declaration -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -Wno-implicit-function-declaration -DM32 -verify %s
// expected-no-diagnostics
#define UINT_MAX (~0u)
@@ -36,4 +36,23 @@
*ptr = 42; // no-warning
}
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+ // Not out of bounds.
+ buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+ memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+ memcpy(addr, buf, size); // no-warning
+}
+
#endif
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
// Get the offset: the minimum value of the array index type.
BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
- // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
// Adjust the index.
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
// If a variable is reinterpreted as a type that doesn't fit into
