Skip to content

Commit 40a69e5

Browse files
firewavefirewave
authored
optimized deserializing of ErrorMessage and related main process code (#4610)
1 parent bdee2ff commit 40a69e5

8 files changed

Lines changed: 103 additions & 53 deletions

File tree

‎cli/filelister.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -210,15 +210,17 @@ static std::string addFiles2(std::map<std::string, std::size_t> &files,
210210
// TODO: suppress instead?
211211
(void)dir_result_buffer.buf; // do not trigger cppcheck itself on the "unused buf"
212212
std::string new_path;
213-
new_path.reserve(path.length() + 100);// prealloc some memory to avoid constant new/deletes in loop
214-
213+
new_path.reserve(path.length() + 1 + sizeof(dir_result->d_name));// prealloc some memory to avoid constant new/deletes in loop
214+
new_path += path;
215+
new_path += '/';
215216

216217
while ((SUPPRESS_DEPRECATED_WARNING(readdir_r(dir, &dir_result_buffer.entry, &dir_result)) == 0) && (dir_result != nullptr)) {
217218
if ((std::strcmp(dir_result->d_name, ".") == 0) ||
218219
(std::strcmp(dir_result->d_name, "..") == 0))
219220
continue;
220221

221-
new_path = path + '/' + dir_result->d_name;
222+
new_path.erase(path.length() + 1);
223+
new_path += dir_result->d_name;
222224

223225
#if defined(_DIRENT_HAVE_D_TYPE) || defined(_BSD_SOURCE)
224226
const bool path_is_directory = (dir_result->d_type == DT_DIR || (dir_result->d_type == DT_UNKNOWN && FileLister::isDirectory(new_path)));

‎lib/errorlogger.cpp‎

Lines changed: 78 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -277,17 +277,17 @@ void ErrorMessage::deserialize(const std::string &data)
277277
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - premature end of data");
278278

279279
std::string temp;
280-
for (unsigned int i = 0; i < len && iss.good(); ++i) {
281-
const char c = static_cast<char>(iss.get());
282-
temp.append(1, c);
283-
}
280+
if (len > 0) {
281+
temp.resize(len);
282+
iss.read(&temp[0], len);
284283

285-
if (!iss.good())
286-
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - premature end of data");
284+
if (!iss.good())
285+
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - premature end of data");
287286

288-
if (temp == "inconclusive") {
289-
certainty = Certainty::inconclusive;
290-
continue;
287+
if (temp == "inconclusive") {
288+
certainty = Certainty::inconclusive;
289+
continue;
290+
}
291291
}
292292

293293
results[elem++] = temp;
@@ -299,15 +299,32 @@ void ErrorMessage::deserialize(const std::string &data)
299299
if (elem != 7)
300300
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - insufficient elements");
301301

302-
id = results[0];
302+
id = std::move(results[0]);
303303
severity = Severity::fromString(results[1]);
304-
if (!(std::istringstream(results[2]) >> cwe.id))
305-
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid CWE ID");
306-
if (!(std::istringstream(results[3]) >> hash))
307-
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid hash");
308-
file0 = results[4];
309-
mShortMessage = results[5];
310-
mVerboseMessage = results[6];
304+
unsigned long long tmp = 0;
305+
if (!results[2].empty()) {
306+
try {
307+
tmp = MathLib::toULongNumber(results[2]);
308+
}
309+
catch (const InternalError&) {
310+
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid CWE ID");
311+
}
312+
if (tmp > std::numeric_limits<unsigned short>::max())
313+
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - CWE ID is out of range");
314+
}
315+
cwe.id = static_cast<unsigned short>(tmp);
316+
hash = 0;
317+
if (!results[3].empty()) {
318+
try {
319+
hash = MathLib::toULongNumber(results[3]);
320+
}
321+
catch (const InternalError&) {
322+
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid hash");
323+
}
324+
}
325+
file0 = std::move(results[4]);
326+
mShortMessage = std::move(results[5]);
327+
mVerboseMessage = std::move(results[6]);
311328

312329
unsigned int stackSize = 0;
313330
if (!(iss >> stackSize))
@@ -324,14 +341,20 @@ void ErrorMessage::deserialize(const std::string &data)
324341
if (!(iss >> len))
325342
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid length (stack)");
326343

327-
iss.get();
344+
if (iss.get() != ' ')
345+
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid separator (stack)");
346+
328347
std::string temp;
329-
for (unsigned int i = 0; i < len && iss.good(); ++i) {
330-
const char c = static_cast<char>(iss.get());
331-
temp.append(1, c);
348+
if (len > 0) {
349+
temp.resize(len);
350+
iss.read(&temp[0], len);
351+
352+
if (!iss.good())
353+
throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - premature end of data (stack)");
332354
}
333355

334356
std::vector<std::string> substrings;
357+
substrings.reserve(5);
335358
for (std::string::size_type pos = 0; pos < temp.size() && substrings.size() < 5; ++pos) {
336359
if (substrings.size() == 4) {
337360
substrings.push_back(temp.substr(pos));
@@ -351,7 +374,7 @@ void ErrorMessage::deserialize(const std::string &data)
351374
// (*loc).line << '\t' << (*loc).column << '\t' << (*loc).getfile(false) << '\t' << loc->getOrigFile(false) << '\t' << loc->getinfo();
352375

353376
ErrorMessage::FileLocation loc(substrings[3], MathLib::toLongNumber(substrings[0]), MathLib::toLongNumber(substrings[1]));
354-
loc.setfile(substrings[2]);
377+
loc.setfile(std::move(substrings[2]));
355378
if (substrings.size() == 5)
356379
loc.setinfo(substrings[4]);
357380

@@ -490,14 +513,22 @@ static std::string readCode(const std::string &file, int linenr, int column, con
490513

491514
static void replaceColors(std::string& source)
492515
{
493-
findAndReplace(source, "{reset}", ::toString(Color::Reset));
494-
findAndReplace(source, "{bold}", ::toString(Color::Bold));
495-
findAndReplace(source, "{dim}", ::toString(Color::Dim));
496-
findAndReplace(source, "{red}", ::toString(Color::FgRed));
497-
findAndReplace(source, "{green}", ::toString(Color::FgGreen));
498-
findAndReplace(source, "{blue}", ::toString(Color::FgBlue));
499-
findAndReplace(source, "{magenta}", ::toString(Color::FgMagenta));
500-
findAndReplace(source, "{default}", ::toString(Color::FgDefault));
516+
static const std::string reset_str = ::toString(Color::Reset);
517+
findAndReplace(source, "{reset}", reset_str);
518+
static const std::string bold_str = ::toString(Color::Bold);
519+
findAndReplace(source, "{bold}", bold_str);
520+
static const std::string dim_str = ::toString(Color::Dim);
521+
findAndReplace(source, "{dim}", dim_str);
522+
static const std::string red_str = ::toString(Color::FgRed);
523+
findAndReplace(source, "{red}", red_str);
524+
static const std::string green_str = ::toString(Color::FgGreen);
525+
findAndReplace(source, "{green}", green_str);
526+
static const std::string blue_str = ::toString(Color::FgBlue);
527+
findAndReplace(source, "{blue}", blue_str);
528+
static const std::string magenta_str = ::toString(Color::FgMagenta);
529+
findAndReplace(source, "{magenta}", magenta_str);
530+
static const std::string default_str = ::toString(Color::FgDefault);
531+
findAndReplace(source, "{default}", default_str);
501532
}
502533

503534
std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation) const
@@ -506,17 +537,20 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
506537

507538
// No template is given
508539
if (templateFormat.empty()) {
509-
std::ostringstream text;
510-
if (!callStack.empty())
511-
text << ErrorLogger::callStackToString(callStack) << ": ";
540+
std::string text;
541+
if (!callStack.empty()) {
542+
text += ErrorLogger::callStackToString(callStack);
543+
text += ": ";
544+
}
512545
if (severity != Severity::none) {
513-
text << '(' << Severity::toString(severity);
546+
text += '(';
547+
text += Severity::toString(severity);
514548
if (certainty == Certainty::inconclusive)
515-
text << ", inconclusive";
516-
text << ") ";
549+
text += ", inconclusive";
550+
text += ") ";
517551
}
518-
text << (verbose ? mVerboseMessage : mShortMessage);
519-
return text.str();
552+
text += (verbose ? mVerboseMessage : mShortMessage);
553+
return text;
520554
}
521555

522556
// template is given. Reformat the output according to it
@@ -542,8 +576,9 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
542576
findAndReplace(result, "{severity}", Severity::toString(severity));
543577
findAndReplace(result, "{cwe}", MathLib::toString(cwe.id));
544578
findAndReplace(result, "{message}", verbose ? mVerboseMessage : mShortMessage);
545-
findAndReplace(result, "{callstack}", callStack.empty() ? emptyString : ErrorLogger::callStackToString(callStack));
546579
if (!callStack.empty()) {
580+
if (result.find("{callstack}") != std::string::npos)
581+
findAndReplace(result, "{callstack}", ErrorLogger::callStackToString(callStack));
547582
findAndReplace(result, "{file}", callStack.back().getfile());
548583
findAndReplace(result, "{line}", MathLib::toString(callStack.back().line));
549584
findAndReplace(result, "{column}", MathLib::toString(callStack.back().column));
@@ -559,6 +594,7 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
559594
findAndReplace(result, "{code}", readCode(callStack.back().getOrigFile(), callStack.back().line, callStack.back().column, endl));
560595
}
561596
} else {
597+
findAndReplace(result, "{callstack}", emptyString);
562598
findAndReplace(result, "{file}", "nofile");
563599
findAndReplace(result, "{line}", "0");
564600
findAndReplace(result, "{column}", "0");
@@ -663,11 +699,10 @@ std::string ErrorMessage::FileLocation::getOrigFile(bool convert) const
663699
return mOrigFileName;
664700
}
665701

666-
void ErrorMessage::FileLocation::setfile(const std::string &file)
702+
void ErrorMessage::FileLocation::setfile(std::string file)
667703
{
668-
mFileName = file;
669-
mFileName = Path::fromNativeSeparators(mFileName);
670-
mFileName = Path::simplifyPath(mFileName);
704+
mFileName = Path::fromNativeSeparators(std::move(file));
705+
mFileName = Path::simplifyPath(std::move(mFileName));
671706
}
672707

673708
std::string ErrorMessage::FileLocation::stringify() const

‎lib/errorlogger.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ class CPPCHECKLIB ErrorMessage {
8484
* Set the filename.
8585
* @param file Filename to set.
8686
*/
87-
void setfile(const std::string &file);
87+
void setfile(std::string file);
8888

8989
/**
9090
* @return the location as a string. Format: [file:line]

‎lib/preprocessor.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -842,10 +842,10 @@ void Preprocessor::missingInclude(const std::string &filename, unsigned int line
842842
if (!mSettings.checks.isEnabled(Checks::missingInclude) && !mSettings.checkConfiguration)
843843
return;
844844

845-
const std::string fname = Path::fromNativeSeparators(filename);
845+
std::string fname = Path::fromNativeSeparators(filename);
846846
Suppressions::ErrorMessage errorMessage;
847847
errorMessage.errorId = "missingInclude";
848-
errorMessage.setFileName(fname);
848+
errorMessage.setFileName(std::move(fname));
849849
errorMessage.lineNumber = linenr;
850850
if (mSettings.nomsg.isSuppressed(errorMessage))
851851
return;

‎lib/suppressions.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,9 +268,9 @@ std::string Suppressions::addSuppressions(const std::list<Suppression> &suppress
268268
return "";
269269
}
270270

271-
void Suppressions::ErrorMessage::setFileName(const std::string &s)
271+
void Suppressions::ErrorMessage::setFileName(std::string s)
272272
{
273-
mFileName = Path::simplifyPath(s);
273+
mFileName = Path::simplifyPath(std::move(s));
274274
}
275275

276276
bool Suppressions::Suppression::parseComment(std::string comment, std::string *errorMessage)

‎lib/suppressions.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class CPPCHECKLIB Suppressions {
4242
struct CPPCHECKLIB ErrorMessage {
4343
std::size_t hash;
4444
std::string errorId;
45-
void setFileName(const std::string &s);
45+
void setFileName(std::string s);
4646
const std::string &getFileName() const {
4747
return mFileName;
4848
}

‎lib/utils.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ bool matchglob(const std::string& pattern, const std::string& name)
5656
{
5757
const char* p = pattern.c_str();
5858
const char* n = name.c_str();
59-
std::stack<std::pair<const char*, const char*>> backtrack;
59+
std::stack<std::pair<const char*, const char*>, std::vector<std::pair<const char*, const char*>>> backtrack;
6060

6161
for (;;) {
6262
bool matching = true;

‎test/testerrorlogger.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,19 @@ class TestErrorLogger : public TestFixture {
356356
ErrorMessage msg;
357357
ASSERT_THROW_EQUALS(msg.deserialize(str), InternalError, "Internal Error: Deserialization of error message failed - invalid hash");
358358
}
359+
{
360+
// out-of-range CWE ID
361+
const char str[] = "7 errorId"
362+
"5 error"
363+
"5 65536" // max +1
364+
"1 0"
365+
"8 test.cpp"
366+
"17 Programming error"
367+
"17 Programming error"
368+
"0 ";
369+
ErrorMessage msg;
370+
ASSERT_THROW(msg.deserialize(str), InternalError);
371+
}
359372
}
360373

361374
void SerializeSanitize() const {

0 commit comments

Comments
 (0)