Skip to content

Commit de4f57e

Browse files
danmardanmar
committed
Buffer overflow: Add CTU checking for pointer arithmetic overflows
1 parent 9f3ecdd commit de4f57e

4 files changed

Lines changed: 119 additions & 60 deletions

File tree

‎lib/analyzerinfo.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,14 @@ void AnalyzerInformation::writeFilesTxt(const std::string &buildDir, const std::
5353
const std::string afile = getFilename(f);
5454
if (fileCount.find(afile) == fileCount.end())
5555
fileCount[afile] = 0;
56-
fout << afile << ".a" << (++fileCount[afile]) << "::" << Path::fromNativeSeparators(f) << '\n';
56+
fout << afile << ".a" << (++fileCount[afile]) << "::" << Path::simplifyPath(Path::fromNativeSeparators(f)) << '\n';
5757
}
5858

5959
for (const ImportProject::FileSettings &fs : fileSettings) {
6060
const std::string afile = getFilename(fs.filename);
6161
if (fileCount.find(afile) == fileCount.end())
6262
fileCount[afile] = 0;
63-
fout << afile << ".a" << (++fileCount[afile]) << ":" << fs.cfg << ":" << Path::fromNativeSeparators(fs.filename) << std::endl;
63+
fout << afile << ".a" << (++fileCount[afile]) << ":" << fs.cfg << ":" << Path::simplifyPath(Path::fromNativeSeparators(fs.filename)) << std::endl;
6464
}
6565
}
6666

‎lib/checkbufferoverrun.cpp‎

Lines changed: 92 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -740,49 +740,80 @@ void CheckBufferOverrun::bufferNotZeroTerminatedError(const Token *tok, const st
740740

741741
std::string CheckBufferOverrun::MyFileInfo::toString() const
742742
{
743-
return CTU::toString(unsafeUsage);
743+
std::string xml;
744+
if (!unsafeArrayIndex.empty())
745+
xml = " <array-index>\n" + CTU::toString(unsafeArrayIndex) + " </array-index>\n";
746+
if (!unsafePointerArith.empty())
747+
xml += " <pointer-arith>\n" + CTU::toString(unsafePointerArith) + " </pointer-arith>\n";
748+
return xml;
744749
}
745750

746-
bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Check *check, const Token *argtok, MathLib::bigint *offset)
751+
bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Check *check, const Token *argtok, MathLib::bigint *offset, int type)
747752
{
748753
const CheckBufferOverrun *c = dynamic_cast<const CheckBufferOverrun *>(check);
749754
if (!c)
750755
return false;
751756
if (!argtok->valueType())
752757
return false;
753-
if (!Token::Match(argtok, "%name% [") || argtok->astParent() != argtok->next() || Token::simpleMatch(argtok->linkAt(1), "] ["))
758+
const Token *indexTok = nullptr;
759+
if (type == 1 && Token::Match(argtok, "%name% [") && argtok->astParent() == argtok->next() && !Token::simpleMatch(argtok->linkAt(1), "] ["))
760+
indexTok = argtok->next()->astOperand2();
761+
else if (type == 2 && Token::simpleMatch(argtok->astParent(), "+"))
762+
indexTok = (argtok == argtok->astParent()->astOperand1()) ?
763+
argtok->astParent()->astOperand2() :
764+
argtok->astParent()->astOperand1();
765+
if (!indexTok)
754766
return false;
755-
if (!argtok->next()->astOperand2())
756-
return false;
757-
if (!argtok->next()->astOperand2()->hasKnownIntValue())
767+
if (!indexTok->hasKnownIntValue())
758768
return false;
759769
if (!offset)
760770
return false;
761-
*offset = argtok->next()->astOperand2()->getKnownIntValue() * argtok->valueType()->typeSize(*c->mSettings);
771+
*offset = indexTok->getKnownIntValue() * argtok->valueType()->typeSize(*c->mSettings);
762772
return true;
763773
}
764774

775+
bool CheckBufferOverrun::isCtuUnsafeArrayIndex(const Check *check, const Token *argtok, MathLib::bigint *offset)
776+
{
777+
return CheckBufferOverrun::isCtuUnsafeBufferUsage(check, argtok, offset, 1);
778+
}
779+
780+
bool CheckBufferOverrun::isCtuUnsafePointerArith(const Check *check, const Token *argtok, MathLib::bigint *offset)
781+
{
782+
return CheckBufferOverrun::isCtuUnsafeBufferUsage(check, argtok, offset, 2);
783+
}
784+
765785
/** @brief Parse current TU and extract file info */
766786
Check::FileInfo *CheckBufferOverrun::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const
767787
{
768788
CheckBufferOverrun checkBufferOverrun(tokenizer, settings, nullptr);
769-
const std::list<CTU::FileInfo::UnsafeUsage> &unsafeUsage = CTU::getUnsafeUsage(tokenizer, settings, &checkBufferOverrun, isCtuUnsafeBufferUsage);
770-
if (unsafeUsage.empty())
771-
return nullptr;
772-
773789
MyFileInfo *fileInfo = new MyFileInfo;
774-
fileInfo->unsafeUsage = unsafeUsage;
790+
fileInfo->unsafeArrayIndex = CTU::getUnsafeUsage(tokenizer, settings, &checkBufferOverrun, isCtuUnsafeArrayIndex);
791+
fileInfo->unsafePointerArith = CTU::getUnsafeUsage(tokenizer, settings, &checkBufferOverrun, isCtuUnsafePointerArith);
792+
if (fileInfo->unsafeArrayIndex.empty() && fileInfo->unsafePointerArith.empty()) {
793+
delete fileInfo;
794+
return nullptr;
795+
}
775796
return fileInfo;
776797
}
777798

778799
Check::FileInfo * CheckBufferOverrun::loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const
779800
{
780-
const std::list<CTU::FileInfo::UnsafeUsage> &unsafeUsage = CTU::loadUnsafeUsageListFromXml(xmlElement);
781-
if (unsafeUsage.empty())
782-
return nullptr;
801+
const std::string arrayIndex("array-index");
802+
const std::string pointerArith("pointer-arith");
783803

784804
MyFileInfo *fileInfo = new MyFileInfo;
785-
fileInfo->unsafeUsage = unsafeUsage;
805+
for (const tinyxml2::XMLElement *e = xmlElement->FirstChildElement(); e; e = e->NextSiblingElement()) {
806+
if (e->Name() == arrayIndex)
807+
fileInfo->unsafeArrayIndex = CTU::loadUnsafeUsageListFromXml(e);
808+
else if (e->Name() == pointerArith)
809+
fileInfo->unsafePointerArith = CTU::loadUnsafeUsageListFromXml(e);
810+
}
811+
812+
if (fileInfo->unsafeArrayIndex.empty() && fileInfo->unsafePointerArith.empty()) {
813+
delete fileInfo;
814+
return nullptr;
815+
}
816+
786817
return fileInfo;
787818
}
788819

@@ -800,40 +831,53 @@ bool CheckBufferOverrun::analyseWholeProgram(const CTU::FileInfo *ctu, const std
800831
const MyFileInfo *fi = dynamic_cast<MyFileInfo*>(fi1);
801832
if (!fi)
802833
continue;
803-
for (const CTU::FileInfo::UnsafeUsage &unsafeUsage : fi->unsafeUsage) {
804-
const CTU::FileInfo::FunctionCall *functionCall = nullptr;
805-
806-
const std::list<ErrorLogger::ErrorMessage::FileLocation> &locationList =
807-
ctu->getErrorPath(CTU::FileInfo::InvalidValueType::bufferOverflow,
808-
unsafeUsage,
809-
callsMap,
810-
"Using argument ARG",
811-
&functionCall,
812-
false);
813-
if (locationList.empty())
814-
continue;
834+
for (const CTU::FileInfo::UnsafeUsage &unsafeUsage : fi->unsafeArrayIndex)
835+
foundErrors |= analyseWholeProgram1(ctu, callsMap, unsafeUsage, 1, errorLogger);
836+
for (const CTU::FileInfo::UnsafeUsage &unsafeUsage : fi->unsafePointerArith)
837+
foundErrors |= analyseWholeProgram1(ctu, callsMap, unsafeUsage, 2, errorLogger);
838+
}
839+
return foundErrors;
840+
}
815841

816-
if (unsafeUsage.value > 0) {
817-
const ErrorLogger::ErrorMessage errmsg(locationList,
818-
emptyString,
819-
Severity::error,
820-
"Buffer access out of bounds; '" + unsafeUsage.myArgumentName + "' buffer size is " + MathLib::toString(functionCall->callArgValue) + " and it is accessed at offset " + MathLib::toString(unsafeUsage.value) + ".",
821-
"ctubufferoverrun",
822-
CWE_BUFFER_OVERRUN, false);
823-
errorLogger.reportErr(errmsg);
824-
} else {
825-
const ErrorLogger::ErrorMessage errmsg(locationList,
826-
emptyString,
827-
Severity::error,
828-
"Buffer access out of bounds; buffer '" + unsafeUsage.myArgumentName + "' is accessed at offset " + MathLib::toString(unsafeUsage.value) + ".",
829-
"ctubufferunderrun",
830-
CWE_BUFFER_UNDERRUN, false);
831-
errorLogger.reportErr(errmsg);
832-
}
842+
bool CheckBufferOverrun::analyseWholeProgram1(const CTU::FileInfo *ctu, const std::map<std::string, std::list<const CTU::FileInfo::CallBase *>> &callsMap, const CTU::FileInfo::UnsafeUsage &unsafeUsage, int type, ErrorLogger &errorLogger)
843+
{
844+
const CTU::FileInfo::FunctionCall *functionCall = nullptr;
845+
846+
const std::list<ErrorLogger::ErrorMessage::FileLocation> &locationList =
847+
ctu->getErrorPath(CTU::FileInfo::InvalidValueType::bufferOverflow,
848+
unsafeUsage,
849+
callsMap,
850+
"Using argument ARG",
851+
&functionCall,
852+
false);
853+
if (locationList.empty())
854+
return false;
833855

834-
foundErrors = true;
835-
}
856+
const char *errorId = nullptr;
857+
std::string errmsg;
858+
CWE cwe(0);
859+
860+
if (type == 1) {
861+
errorId = "ctuArrayIndex";
862+
if (unsafeUsage.value > 0)
863+
errmsg = "Array index out of bounds; '" + unsafeUsage.myArgumentName + "' buffer size is " + MathLib::toString(functionCall->callArgValue) + " and it is accessed at offset " + MathLib::toString(unsafeUsage.value) + ".";
864+
else
865+
errmsg = "Array index out of bounds; buffer '" + unsafeUsage.myArgumentName + "' is accessed at offset " + MathLib::toString(unsafeUsage.value) + ".";
866+
cwe = (unsafeUsage.value > 0) ? CWE_BUFFER_OVERRUN : CWE_BUFFER_UNDERRUN;
867+
} else {
868+
errorId = "ctuPointerArith";
869+
errmsg = "Pointer arithmetic overflow; '" + unsafeUsage.myArgumentName + "' buffer size is " + MathLib::toString(functionCall->callArgValue);
870+
cwe = CWE_POINTER_ARITHMETIC_OVERFLOW;
836871
}
837-
return foundErrors;
872+
873+
const ErrorLogger::ErrorMessage errorMessage(locationList,
874+
emptyString,
875+
Severity::error,
876+
errmsg,
877+
errorId,
878+
cwe, false);
879+
errorLogger.reportErr(errorMessage);
880+
881+
return true;
838882
}
839883

‎lib/checkbufferoverrun.h‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
8585
/** @brief Analyse all file infos for all TU */
8686
bool analyseWholeProgram(const CTU::FileInfo *ctu, const std::list<Check::FileInfo*> &fileInfo, const Settings& settings, ErrorLogger &errorLogger) OVERRIDE;
8787

88-
8988
private:
9089

9190
void arrayIndex();
@@ -112,16 +111,22 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
112111
/** data for multifile checking */
113112
class MyFileInfo : public Check::FileInfo {
114113
public:
115-
/** function arguments that data are unconditionally read */
116-
std::list<CTU::FileInfo::UnsafeUsage> unsafeUsage;
114+
/** unsafe array index usage */
115+
std::list<CTU::FileInfo::UnsafeUsage> unsafeArrayIndex;
116+
117+
/** unsafe pointer arithmetics */
118+
std::list<CTU::FileInfo::UnsafeUsage> unsafePointerArith;
117119

118120
/** Convert MyFileInfo data into xml string */
119121
std::string toString() const OVERRIDE;
120122
};
121123

122-
static bool isCtuUnsafeBufferUsage(const Check *check, const Token *argtok, MathLib::bigint *value);
124+
static bool isCtuUnsafeBufferUsage(const Check *check, const Token *argtok, MathLib::bigint *value, int type);
125+
static bool isCtuUnsafeArrayIndex(const Check *check, const Token *argtok, MathLib::bigint *value);
126+
static bool isCtuUnsafePointerArith(const Check *check, const Token *argtok, MathLib::bigint *value);
123127

124128
Check::FileInfo * loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const OVERRIDE;
129+
bool analyseWholeProgram1(const CTU::FileInfo *ctu, const std::map<std::string, std::list<const CTU::FileInfo::CallBase *>> &callsMap, const CTU::FileInfo::UnsafeUsage &unsafeUsage, int type, ErrorLogger &errorLogger);
125130

126131

127132
static std::string myName() {

‎test/testbufferoverrun.cpp‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ class TestBufferOverrun : public TestFixture {
249249
TEST_CASE(ctu_malloc);
250250
TEST_CASE(ctu_array);
251251
TEST_CASE(ctu_variable);
252+
TEST_CASE(ctu_arithmetic);
252253
}
253254

254255

@@ -4088,7 +4089,7 @@ class TestBufferOverrun : public TestFixture {
40884089
" char *s = malloc(4);\n"
40894090
" dostuff(s);\n"
40904091
"}");
4091-
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:7] -> [test.cpp:2]: (error) Buffer access out of bounds; buffer 'p' is accessed at offset -3.\n", errout.str());
4092+
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:7] -> [test.cpp:2]: (error) Array index out of bounds; buffer 'p' is accessed at offset -3.\n", errout.str());
40924093

40934094
ctu("void dostuff(char *p) {\n"
40944095
" p[4] = 0;\n"
@@ -4098,7 +4099,7 @@ class TestBufferOverrun : public TestFixture {
40984099
" char *s = malloc(4);\n"
40994100
" dostuff(s);\n"
41004101
"}");
4101-
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:7] -> [test.cpp:2]: (error) Buffer access out of bounds; 'p' buffer size is 4 and it is accessed at offset 4.\n", errout.str());
4102+
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:7] -> [test.cpp:2]: (error) Array index out of bounds; 'p' buffer size is 4 and it is accessed at offset 4.\n", errout.str());
41024103
}
41034104

41044105
void ctu_array() {
@@ -4109,7 +4110,7 @@ class TestBufferOverrun : public TestFixture {
41094110
" char str[4];\n"
41104111
" dostuff(str);\n"
41114112
"}");
4112-
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:2]: (error) Buffer access out of bounds; 'p' buffer size is 4 and it is accessed at offset 10.\n", errout.str());
4113+
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:2]: (error) Array index out of bounds; 'p' buffer size is 4 and it is accessed at offset 10.\n", errout.str());
41134114

41144115
ctu("static void memclr( char *data )\n"
41154116
"{\n"
@@ -4121,7 +4122,7 @@ class TestBufferOverrun : public TestFixture {
41214122
" char str[5];\n"
41224123
" memclr( str );\n"
41234124
"}");
4124-
ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:3]: (error) Buffer access out of bounds; 'data' buffer size is 5 and it is accessed at offset 10.\n", errout.str());
4125+
ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:3]: (error) Array index out of bounds; 'data' buffer size is 5 and it is accessed at offset 10.\n", errout.str());
41254126

41264127
ctu("static void memclr( int i, char *data )\n"
41274128
"{\n"
@@ -4133,7 +4134,7 @@ class TestBufferOverrun : public TestFixture {
41334134
" char str[5];\n"
41344135
" memclr( 0, str );\n"
41354136
"}");
4136-
ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:3]: (error) Buffer access out of bounds; 'data' buffer size is 5 and it is accessed at offset 10.\n", errout.str());
4137+
ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:3]: (error) Array index out of bounds; 'data' buffer size is 5 and it is accessed at offset 10.\n", errout.str());
41374138

41384139
ctu("static void memclr( int i, char *data )\n"
41394140
"{\n"
@@ -4185,7 +4186,16 @@ class TestBufferOverrun : public TestFixture {
41854186
" int x = 4;\n"
41864187
" dostuff(&x);\n"
41874188
"}");
4188-
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:2]: (error) Buffer access out of bounds; 'p' buffer size is 4 and it is accessed at offset 40.\n", errout.str());
4189+
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:2]: (error) Array index out of bounds; 'p' buffer size is 4 and it is accessed at offset 40.\n", errout.str());
4190+
}
4191+
4192+
void ctu_arithmetic() {
4193+
ctu("void dostuff(int *p) { x = p + 10; }\n"
4194+
"int main() {\n"
4195+
" int x[3];\n"
4196+
" dostuff(x);\n"
4197+
"}");
4198+
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:1]: (error) Pointer arithmetic overflow; 'p' buffer size is 12\n", errout.str());
41894199
}
41904200
};
41914201

0 commit comments

Comments
 (0)