The recent Qt 6 release compelled us to recheck the framework with PVS-Studio. In this article, we reviewed various interesting errors we found, for example, those related to processing dates. The errors we discovered prove that developers can greatly benefit from regularly checking their projects with tools like PVS-Studio.
This is a standard article that reports the results of an open-source project check. This article will add to our "evidence base" that demonstrates how useful and effective PVS-Studio is in code quality control. Though we have already checked the Qt project in the past (in 2011, 2014, and 2018), rechecking the framework was worth it. The new check's result supported a simple, but very important idea: static analysis should be used regularly!
Our articles show that the PVS-Studio analyzer can find a wide variety of errors. Project authors often quickly fix the errors we describe. However, all this has nothing to do with the benefits of regular static code analysis. When static code analysis is built into the development process, developers quickly find and fix errors in new or recently edited code. Fixing code at this stage is the cheapest.
Alright, enough theory! Let's take a look what the Qt 6 code has in store for us. And while you are reading this article, why don't you download PVS-Studio and request a trial key. See for yourself what the static analyzer can find in your projects :).
Lately we've been noticing one more code pattern that tends to attract an increasing number of bugs. Of course, these code fragments are not as significant as comparison functions or the last line in similar code blocks. We are talking about code that works with dates. Such code can be difficult to test. So it comes as no surprise that these untested functions may process some arguments inadequately and return an incorrect result. We've already described a couple of similar cases in the following article: "Why PVS-Studio doesn't offer automatic fixes".
Qt also fell prey to that trend and has occasional problems with code that processes dates. So here is where we start.
Fragment #1: error status misinterpreted
First, let's see how the developer wrote the function that accepts a month's abbreviated name and returns its number.
static const char qt_shortMonthNames[][4] = {
"Jan", "Feb", "Mar", "Apr", "May", "Jun",
"Jul", "Aug", "Sep", "Oct", "Nov", "Dec"
};
static int fromShortMonthName(QStringView monthName)
{
for (unsigned int i = 0;
i < sizeof(qt_shortMonthNames) / sizeof(qt_shortMonthNames[0]); ++i)
{
if (monthName == QLatin1String(qt_shortMonthNames[i], 3))
return i + 1;
}
return -1;
}If successful, the function returns the month number (a value from 1 to 12). If the month's name is incorrect, the function returns a negative value (-1). Note that the function cannot return 0.
However, the function above is used where the developer expects it to return null in case of an error. Here is the code fragment that uses the fromShortMonthName function incorrectly:
QDateTime QDateTime::fromString(QStringView string, Qt::DateFormat format)
{
....
month = fromShortMonthName(parts.at(1));
if (month)
day = parts.at(2).toInt(&ok);
// If failed, try day then month
if (!ok || !month || !day) {
month = fromShortMonthName(parts.at(2));
if (month) {
QStringView dayPart = parts.at(1);
if (dayPart.endsWith(u'.'))
day = dayPart.chopped(1).toInt(&ok);
}
}
....
}The program never reaches the code that checks the month number for null, and continues to run with an incorrect negative month number. The PVS-Studio analyzer sees a whole bunch of inconsistencies here and reports them with four warnings at once:
Fragment #2: error in date processing logic
Let's take a look at the function that returns a number of seconds.
enum {
....
MSECS_PER_DAY = 86400000,
....
SECS_PER_MIN = 60,
};
int QTime::second() const
{
if (!isValid())
return -1;
return (ds() / 1000)%SECS_PER_MIN;
}The function above can return a value in the range of [0..59] or an error status of -1.
Here is one location where the use of this function is very strange:
static qint64 qt_mktime(QDate *date, QTime *time, ....)
{
....
} else if (yy == 1969 && mm == 12 && dd == 31
&& time->second() == MSECS_PER_DAY - 1) {
// There was, of course, a last second in 1969, at time_t(-1); we won't
// rescue it if it's not in normalised form, and we don't know its DST
// status (unless we did already), but let's not wantonly declare it
// invalid.
} else {
....
}PVS-Studio warns: V560 [CWE-570] A part of conditional expression is always false: time->second() == MSECS_PER_DAY - 1. qdatetime.cpp 2488
The comment in the code tells us that if something goes wrong, it's better to do nothing. However, the condition always evaluates to false and the else branch is always executed.
Here is the comparison that's incorrect:
time->second() == MSECS_PER_DAY - 1"MSECS_PER_DAY - 1" equals 86399999. As we already know, the second function cannot return this value. This means the code has some logical error and requires refactoring.
Static analyzers are powerful in a sense that they check all scenarios no matter how infrequent they are. Thus, static analysis is a good addition to unit tests and other code quality control tools.
Fragment #3: suddenly, let's talk about... HTML!
QString QPixelTool::aboutText() const
{
const QList<QScreen *> screens = QGuiApplication::screens();
const QScreen *windowScreen = windowHandle()->screen();
QString result;
QTextStream str(&result);
str << "<html></head><body><h2>Qt Pixeltool</h2><p>Qt " << QT_VERSION_STR
<< "</p><p>Copyright (C) 2017 The Qt Company Ltd.</p><h3>Screens</h3><ul>";
for (const QScreen *screen : screens)
str << "<li>" << (screen == windowScreen ? "* " : " ")
<< screen << "</li>";
str << "<ul></body></html>";
return result;
}PVS-Studio warns: V735 Possibly an incorrect HTML. The "</body>" closing tag was encountered, while the "</ul>" tag was expected. qpixeltool.cpp 707
PVS-Studio contains diagnostics that don't just check code - they also look for abnormalities in string constants. The code above triggered one of these diagnostics. Such cases are quite rare, and that's what makes this one so intriguing.
Someone intended to create one list, but added two tags that open this list instead of one. This is clearly a typo. The first tag must open the list, and the second one needs to close it. Here is the correct code:
str << "</ul></body></html>";Fragment #4: a double check within one condition
class Node
{
....
bool isGroup() const { return m_nodeType == Group; }
....
};
void DocBookGenerator::generateDocBookSynopsis(const Node *node)
{
....
if (node->isGroup() || node->isGroup()
|| node->isSharedCommentNode() || node->isModule()
|| node->isJsModule() || node->isQmlModule() || node->isPageNode())
return;
....
}PVS-Studio warns: V501 [CWE-570] There are identical sub-expressions to the left and to the right of the '||' operator: node->isGroup() || node->isGroup() docbookgenerator.cpp 2599
This is a common typo. The fix depends on what this code is expected to achieve. If the check is duplicated by accident, one can delete it. A different scenario is also possible: some other necessary condition has been left out.
Fragment #5: one too many local variables
void MainWindow::addToPhraseBook()
{
....
QString selectedPhraseBook;
if (phraseBookList.size() == 1) {
selectedPhraseBook = phraseBookList.at(0);
if (QMessageBox::information(this, tr("Add to phrase book"),
tr("Adding entry to phrasebook %1").arg(selectedPhraseBook),
QMessageBox::Ok | QMessageBox::Cancel, QMessageBox::Ok)
!= QMessageBox::Ok)
return;
} else {
bool okPressed = false;
QString selectedPhraseBook =
QInputDialog::getItem(this, tr("Add to phrase book"),
tr("Select phrase book to add to"),
phraseBookList, 0, false, &okPressed);
if (!okPressed)
return;
}
MessageItem *currentMessage = m_dataModel->messageItem(m_currentIndex);
Phrase *phrase = new Phrase(currentMessage->text(),
currentMessage->translation(),
QString(), nullptr);
phraseBookHash.value(selectedPhraseBook)->append(phrase);
}If you want, you can test your attention to detail and look for the error yourself. I'll even move the text down for you so that you don't see the spoiler right away. Here is a beautiful unicorn from our old collection. Maybe you haven't even seen it before :).
PVS-Studio warns: V561 [CWE-563] It's probably better to assign value to 'selectedPhraseBook' variable than to declare it anew. Previous declaration: mainwindow.cpp, line 1303. mainwindow.cpp 1313
The text that originates from either of the conditional operator's branches needs to be recorded to the selectedPhraseBook variable. The developer felt the variable's name was too long to write it out again and copied it from the line that declares the variable. It looks like the developer hurried a little and copied the type of the variable as well:
QString selectedPhraseBook =As a result, the else block contains an excessive local string variable that is initialized, but never used. Meanwhile, the original variable that should have been assigned a value remains empty.
Fragment #6: operation priority
This is a classic error pattern that we encounter quite frequently.
bool QQmlImportInstance::resolveType(....)
{
....
if (int icID = containingType.lookupInlineComponentIdByName(typeStr) != -1)
{
*type_return = containingType.lookupInlineComponentById(icID);
} else {
auto icType = createICType();
....
}
....
}PVS-Studio warns: V593 [CWE-783] Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. qqmlimport.cpp 754
The icID variable always has a value of 0 or 1. This is clearly not what the developer intended to do. Here's the reason: the comparison to -1 comes first, and then the icID variable is initialized.
You can use modern C++ syntax to phrase the condition correctly - as shown below:
if (int icID = containingType.lookupInlineComponentIdByName(typeStr);
icID != -1)By the way, I have already seen a similar error in Qt before:
char ch;
while (i < dataLen && ((ch = data.at(i) != '\n') && ch != '\r'))
++i;This demonstrates that developers will keep making the same mistakes over and over again until they integrate an analyzer like PVS-Studio into the development process. No one is perfect. Yes, this is a subtle hint that you should start using PVS-Studio :).
Fragment #7: the evil modulus division
Often, you may need to determine whether a number is divisible by 2 without remainder. The correct way to do this is to do a modulo division by two and check the result:
if (A % 2 == 1)However, the developers may write something like this instead:
if (A % 1 == 1)This is wrong because the remainder of the modulo division by one is always zero. Qt also has this error:
bool loadQM(Translator &translator, QIODevice &dev, ConversionData &cd)
{
....
case Tag_Translation: {
int len = read32(m);
if (len % 1) { // <=
cd.appendError(QLatin1String("QM-Format error"));
return false;
}
m += 4;
QString str = QString((const QChar *)m, len/2);
....
}PVS-Studio warns: V1063 The modulo by 1 operation is meaningless. The result will always be zero. qm.cpp 549
Fragment #8: overwriting a value
QString Node::qualifyQmlName()
{
QString qualifiedName = m_name;
if (m_name.startsWith(QLatin1String("QML:")))
qualifiedName = m_name.mid(4);
qualifiedName = logicalModuleName() + "::" + m_name;
return qualifiedName;
}PVS-Studio warns: V519 [CWE-563] The 'qualifiedName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1227, 1228. node.cpp 1228
As far as I understand, the developer accidentally used a wrong variable name. I assume the code should read as follows:
QString qualifiedName = m_name;
if (m_name.startsWith(QLatin1String("QML:")))
qualifiedName = m_name.mid(4);
qualifiedName = logicalModuleName() + "::" + qualifiedName;
return qualifiedName;Fragment #9: copy and paste
class Q_CORE_EXPORT QJsonObject
{
....
bool operator<(const iterator& other) const
{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }
bool operator<=(const iterator& other) const
{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }
....
}PVS-Studio warns: V524 It is odd that the body of '<=' function is fully equivalent to the body of '<' function. qjsonobject.h 155
No one checks boring functions like comparison operators. No one writes tests for them. Developers may take a quick look at them during code review - or skip them altogether. But that's a bad idea. And that's where static code analysis comes in handy. The analyzer never gets tired and is happy to check even boring code snippets.
Here the < and <= operators are implemented in the same way. This is certainly wrong. The developer may have found the code elsewhere, copy-pasted it, and then forgot to customize it. Here is the correct code:
bool operator<(const iterator& other) const
{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }
bool operator<=(const iterator& other) const
{ Q_ASSERT(item.o == other.item.o); return item.index <= other.item.index; }Fragment #10: static_cast / dynamic_cast
void QSGSoftwareRenderThread::syncAndRender()
{
....
bool canRender = wd->renderer != nullptr;
if (canRender) {
auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);
if (softwareRenderer)
softwareRenderer->setBackingStore(backingStore);
....
}PVS-Studio warns: V547 [CWE-571] Expression 'softwareRenderer' is always true. qsgsoftwarethreadedrenderloop.cpp 510
First, let's take a look at this check:
bool canRender = wd->renderer != nullptr;
if (canRender) {The code makes sure that the wd->renderer pointer is never null inside the conditional operator. So why add one more check? What does it do exactly?
auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);
if (softwareRenderer)If the wd->renderer pointer is not null, the softwareRenderer pointer cannot be null. I suspect there is a typo here and the developer intended to use dynamic_cast. In this case, the code starts to make sense. If type conversion is not possible, the dynamic_cast operator returns nullptr. This returned value should be checked. However, I may have misinterpreted the situation and the code needs to be corrected in a different way.
Fragment #11: copied, but not altered
void *QQuickPath::qt_metacast(const char *_clname)
{
if (!_clname) return nullptr;
if (!strcmp(_clname, qt_meta_stringdata_QQuickPath.stringdata0))
return static_cast<void*>(this);
if (!strcmp(_clname, "QQmlParserStatus"))
return static_cast< QQmlParserStatus*>(this);
if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus")) // <=
return static_cast< QQmlParserStatus*>(this);
if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus")) // <=
return static_cast< QQmlParserStatus*>(this);
return QObject::qt_metacast(_clname);
}PVS-Studio warns: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 2719, 2721. moc_qquickpath_p.cpp 2721
Take a look at these two lines:
if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))
return static_cast< QQmlParserStatus*>(this);Someone copied and pasted them multiple times - and forgot to modify them. The way they are now, they do not make sense.
Fragment #12: overflow due to the wrong parenthesis placement
int m_offsetFromUtc;
....
void QDateTime::setMSecsSinceEpoch(qint64 msecs)
{
....
if (!add_overflow(msecs, qint64(d->m_offsetFromUtc * 1000), &msecs))
status |= QDateTimePrivate::ValidWhenMask;
....
}PVS-Studio warns: V1028 [CWE-190] Possible overflow. Consider casting operands of the 'd->m_offsetFromUtc * 1000' operator to the 'qint64' type, not the result. qdatetime.cpp 3922
The developer foresees a case when the int type variable is multiplied by 1000 and causes overflow. To avoid this, the developer plans to use the qint64 64-bit type variable. And uses explicit type casting.
However, the casting does not help at all, because the overflow happens before the casting. The correct code:
add_overflow(msecs, qint64(d->m_offsetFromUtc) * 1000, &msecs)Fragment #13: a partly initialized array
class QPathEdge
{
....
private:
int m_next[2][2];
....
};
inline QPathEdge::QPathEdge(int a, int b)
: flag(0)
, windingA(0)
, windingB(0)
, first(a)
, second(b)
, angle(0)
, invAngle(0)
{
m_next[0][0] = -1;
m_next[1][0] = -1;
m_next[0][0] = -1;
m_next[1][0] = -1;
}PVS-Studio warns:
Above is a failed attempt to initialize a 2x2 array. Two elements are initialized twice, while the other two got overlooked. The correct code:
m_next[0][0] = -1;
m_next[0][1] = -1;
m_next[1][0] = -1;
m_next[1][1] = -1;And let me say, I just love it when I see how professional developers make such silly mistakes. Don't get me wrong, but such cases demonstrate that everyone is human and can make a mistake or a typo. So, static analysis is your best friend. I think it's been about 10 years since I've started fighting sceptical - albeit professional - developers over one simple subject: such errors happen in their own code as well - students are not the only ones to breed typos in their code :). 10 years ago I wrote a note: "The second myth - expert developers do not make silly mistakes". Nothing changed since then. People keep making mistakes and pretending they don't :).
Fragment #14: Unreachable Code
void QmlProfilerApplication::tryToConnect()
{
Q_ASSERT(!m_connection->isConnected());
++ m_connectionAttempts;
if (!m_verbose && !(m_connectionAttempts % 5)) {// print every 5 seconds
if (m_verbose) {
if (m_socketFile.isEmpty())
logError(
QString::fromLatin1("Could not connect to %1:%2 for %3 seconds ...")
.arg(m_hostName).arg(m_port).arg(m_connectionAttempts));
else
logError(
QString::fromLatin1("No connection received on %1 for %2 seconds ...")
.arg(m_socketFile).arg(m_connectionAttempts));
}
}
....
}PVS-Studio warns: V547 [CWE-570] Expression 'm_verbose' is always false. qmlprofilerapplication.cpp 495
This code will never log anything because of the conflicting conditions.
if (!m_verbose && ....) {
if (m_verbose) {Fragment #15: overwriting a variable's value
void QRollEffect::scroll()
{
....
if (currentHeight != totalHeight) {
currentHeight = totalHeight * (elapsed/duration)
+ (2 * totalHeight * (elapsed%duration) + duration)
/ (2 * duration);
// equiv. to int((totalHeight*elapsed) / duration + 0.5)
done = (currentHeight >= totalHeight);
}
done = (currentHeight >= totalHeight) &&
(currentWidth >= totalWidth);
....
}PVS-Studio warns: V519 [CWE-563] The 'done' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 509, 511. qeffects.cpp 511
The entire conditional operator makes no sense, because the done variable is overwritten right after it is assigned. The code could be lacking the else keyword.
Fragment #16-#20: overwriting variables' values
Here is another example of a variable's value that is overwritten:
bool QXmlStreamWriterPrivate::finishStartElement(bool contents)
{
....
if (inEmptyElement) {
....
lastNamespaceDeclaration = tag.namespaceDeclarationsSize; // <=
lastWasStartElement = false;
} else {
write(">");
}
inStartElement = inEmptyElement = false;
lastNamespaceDeclaration = namespaceDeclarations.size(); // <=
return hadSomethingWritten;
}PVS-Studio warns: V519 [CWE-563] The 'lastNamespaceDeclaration' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3030, 3036. qxmlstream.cpp 3036
The lastNamespaceDeclaration variable's first assignment may have happened by accident. It is probably okay to delete this line. However, we could be facing a serious logical error.
Four more warnings indicate the same error patterns in the Qt 6 code:
Fragment #21: confusion between null pointer and empty string
// this could become a list of all languages used for each writing
// system, instead of using the single most common language.
static const char languageForWritingSystem[][6] = {
"", // Any
"en", // Latin
"el", // Greek
"ru", // Cyrillic
...... // No null pointers. Empty string literals are used.
"", // Symbol
"sga", // Ogham
"non", // Runic
"man" // N'Ko
};
static void populateFromPattern(....)
{
....
for (int j = 1; j < QFontDatabase::WritingSystemsCount; ++j) {
const FcChar8 *lang = (const FcChar8*) languageForWritingSystem[j];
if (lang) {
....
}PVS-Studio warns: V547 [CWE-571] Expression 'lang' is always true. qfontconfigdatabase.cpp 462
The languageForWritingSystem array has no null pointers. Which is why the if(lang) check makes no sense. However, the array contains empty strings. Has the developer meant to do an empty string check? If yes, the correct code goes like this:
if (strlen(lang) != 0) {Or you can simplify it even further:
if (lang[0] != '\0') {Fragment #22: A bizarre check
bool QNativeSocketEnginePrivate::createNewSocket(....)
{
....
int socket = qt_safe_socket(domain, type, protocol, O_NONBLOCK);
....
if (socket < 0) {
....
return false;
}
socketDescriptor = socket;
if (socket != -1) {
this->socketProtocol = socketProtocol;
this->socketType = socketType;
}
return true;
}PVS-Studio warns: V547 [CWE-571] Expression 'socket != - 1' is always true. qnativesocketengine_unix.cpp 315
The socket != -1 condition always evaluates to true, because the function above it always exits when the socket value is negative.
Fragment #23: what exactly should the function return?
bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent)
{
Q_D(QSqlTableModel);
if (parent.isValid() || row < 0 || count <= 0)
return false;
else if (row + count > rowCount())
return false;
else if (!count)
return true;
....
}PVS-Studio warns: V547 [CWE-570] Expression '!count' is always false. qsqltablemodel.cpp 1110
To make this more simple, I'll point out the most important lines:
if (.... || count <= 0)
return false;
....
else if (!count)
return true;The first check indicates that if the count value equals to or is below 0, the state is incorrect and the function must return false. However, further on we see this variable compared to zero, and this case is interpreted differently: the function must return true.
There's clearly something wrong here. I suspect that the developer intended to use the < operator instead of <=. Then the code starts to make sense:
bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent)
{
Q_D(QSqlTableModel);
if (parent.isValid() || row < 0 || count < 0)
return false;
else if (row + count > rowCount())
return false;
else if (!count)
return true;
....
}Fragment #24: an unnecessary status?
The code below contains the identifierWithEscapeChars variable that looks like a redundant entity. Or is it a logical error? Or is the code unfinished? By the second check this variable is true in all scenarios
int Lexer::scanToken()
{
....
bool identifierWithEscapeChars = false;
....
if (!identifierWithEscapeChars) {
identifierWithEscapeChars = true;
....
}
....
if (identifierWithEscapeChars) { // <=
....
}
....
}PVS-Studio warns: V547 [CWE-571] Expression 'identifierWithEscapeChars' is always true. qqmljslexer.cpp 817
Fragment #25: what do I do with nine objects?
bool QFont::fromString(const QString &descrip)
{
....
const int count = l.count();
if (!count || (count > 2 && count < 9) || count == 9 || count > 17 ||
l.first().isEmpty()) {
qWarning("QFont::fromString: Invalid description '%s'",
descrip.isEmpty() ? "(empty)" : descrip.toLatin1().data());
return false;
}
setFamily(l[0].toString());
if (count > 1 && l[1].toDouble() > 0.0)
setPointSizeF(l[1].toDouble());
if (count == 9) { // <=
setStyleHint((StyleHint) l[2].toInt());
setWeight(QFont::Weight(l[3].toInt()));
setItalic(l[4].toInt());
setUnderline(l[5].toInt());
setStrikeOut(l[6].toInt());
setFixedPitch(l[7].toInt());
} else if (count >= 10) {
....
}PVS-Studio warns: V547 [CWE-570] Expression 'count == 9' is always false. qfont.cpp 2142
What should the function do if the count variable equals 9? On the one hand, the function should issue a warning and exit. Just as the code says:
if (.... || count == 9 || ....) {
qWarning(....);
return false;
}On the other hand, someone added special code to be executed for 9 objects:
if (count == 9) {
setStyleHint((StyleHint) l[2].toInt());
setWeight(QFont::Weight(l[3].toInt()));
setItalic(l[4].toInt());
....
}The function, of course, never reaches this code. The code is waiting for someone to come and fix it :).
Fragments #26-#42: using a pointer before checking it
class __attribute__((visibility("default"))) QMetaType {
....
const QtPrivate::QMetaTypeInterface *d_ptr = nullptr;
};
QPartialOrdering QMetaType::compare(const void *lhs, const void *rhs) const
{
if (!lhs || !rhs)
return QPartialOrdering::Unordered;
if (d_ptr->flags & QMetaType::IsPointer)
return threeWayCompare(*reinterpret_cast<const void * const *>(lhs),
*reinterpret_cast<const void * const *>(rhs));
if (d_ptr && d_ptr->lessThan) {
if (d_ptr->equals && d_ptr->equals(d_ptr, lhs, rhs))
return QPartialOrdering::Equivalent;
if (d_ptr->lessThan(d_ptr, lhs, rhs))
return QPartialOrdering::Less;
if (d_ptr->lessThan(d_ptr, rhs, lhs))
return QPartialOrdering::Greater;
if (!d_ptr->equals)
return QPartialOrdering::Equivalent;
}
return QPartialOrdering::Unordered;
}PVS-Studio warns: V595 [CWE-476] The 'd_ptr' pointer was utilized before it was verified against nullptr. Check lines: 710, 713. qmetatype.cpp 710
The error is easy to overlook, but everything is straightforward here. Let's see how the code uses the d_ptr pointer:
if (d_ptr->flags & ....)
if (d_ptr && ....)In the first if-block the pointer is dereferenced. Then the next check suggests this pointer can be null.
This is one of the most common error patterns in C and C++. Proofs. We saw quite a few errors of this kind in the Qt source code.
Fragment #43: within one expression, the use of a pointer that has not been checked for null
This case is almost the same as the one above. However, this time the pointer is dereferenced and checked inside one expression. This is a classic incidental error - someone was inattentive when writing and reviewing code.
void QFormLayoutPrivate::updateSizes()
{
....
QFormLayoutItem *field = m_matrix(i, 1);
....
if (userHSpacing < 0 && !wrapAllRows && (label || !field->fullRow) && field)
....
}PVS-Studio warns: V713 [CWE-476] The pointer 'field' was utilized in the logical expression before it was verified against nullptr in the same logical expression. qformlayout.cpp 405
Now let's take a one-minute break.
I got tired from all the writing. I think the readers are tired as well. This article can wear you out even if you are just skimming through the text :). So it's about time I get my second cup of coffee. I finished my first one at around Fragment #12. Why don't you, my readers, join me for a cup of joe - or pick your favorite drink.
And while we are all taking a break, I'll wander off from the topic for a bit. I am inviting the team that develops the Qt project to consider purchasing a license for the PVS-Studio code analyzer. You can request our price list here. We will provide support and help you set up the analyzer. Yes, alright, today I am more insistent. This is something new that I'm trying :).
Fragments #44-#72: no check for the malloc function's product
void assignData(const QQmlProfilerEvent &other)
{
if (m_dataType & External) {
uint length = m_dataLength * (other.m_dataType / 8);
m_data.external = malloc(length); // <=
memcpy(m_data.external, other.m_data.external, length); // <=
} else {
memcpy(&m_data, &other.m_data, sizeof(m_data));
}
}PVS-Studio warns: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 277, 276. qqmlprofilerevent_p.h 277
You cannot simply take and use the pointer the malloc function returns. It is imperative you check this pointer for null, even if you are very lazy to do it. We described 4 possible reasons to do this in our article "Why it is important to check what the malloc function returned".
The need to check the malloc function's output falls within that article's scope. There are more warnings, but I do not want to include them into this list, because they are too many. Just in case, I gathered 28 warnings in the following file for you: qt6-malloc.txt. I do, however, recommend developers to recheck the project and study the warnings themselves. I did not have a goal to find as many errors as possible.
Interestingly enough, with all the important missed checks, I found completely unnecessary ones. I am talking about the new operator call, that, in case of an error, generates the std::bad_alloc exception. Here is one example of such redundant check:
static QImageScaleInfo* QImageScale::qimageCalcScaleInfo(....)
{
....
QImageScaleInfo *isi;
....
isi = new QImageScaleInfo;
if (!isi)
return nullptr;
....
}PVS-Studio warns: V668 [CWE-570] There is no sense in testing the 'isi' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qimagescale.cpp 245
P.S. Here the readers always ask, does the analyzer know about placement new or "new (std::nothrow) T"? Yes, it does, and no, it does not issue any false positives for them.
In some scenarios, the analyzer issues warnings to code that is correct, but excessive. It may happen, for example, when the same variable is checked twice. Sometimes it's not clear whether this is a false positive or not. Technically, the analyzer is correct, but it did not find a real error.
You can probably say it's a "code smell". Since the analyzer does not like this code, other developers may not like it either and may find it difficult to support. You have to spend more time to understand what is happening.
Usually I don't even discuss such warnings in my articles. It's boring to do this. However, the Qt project surprised me with how many so-called "code smells" I was able to find. Definitely more than in most projects. Which is why I decided to turn your attention to "code smells" and investigate a few such cases. I think it will be useful to refactor these and many other similar patterns. To do this, you will need to use a complete report. The report's fragments I added to this article are insufficient.
So let's inspect a few scenarios that illustrate the problem.
Fragment #73: "Code smell" - reverse check
void QQuick3DSceneManager::setWindow(QQuickWindow *window)
{
if (window == m_window)
return;
if (window != m_window) {
if (m_window)
disconnect(....);
m_window = window;
connect(....);
emit windowChanged();
}
}PVS-Studio warns: V547 [CWE-571] Expression 'window != m_window' is always true. qquick3dscenemanager.cpp 60
If window==m_window, the function exists. The consecutive inverse check makes no sense and just clutters the code.
Fragment #74: "Code smell" - weird initialization
QModelIndex QTreeView::moveCursor(....)
{
....
int vi = -1;
if (vi < 0)
vi = qMax(0, d->viewIndex(current));
....
}PVS-Studio warns: V547 [CWE-571] Expression 'vi < 0' is always true. qtreeview.cpp 2219
What is this? Why write something like this? The developer can simplify the code down to one line:
int vi = qMax(0, d->viewIndex(current));Fragment #75: "Code smell" - unreachable code
bool copyQtFiles(Options *options)
{
....
if (unmetDependencies.isEmpty()) {
if (options->verbose) {
fprintf(stdout, " -- Skipping %s, architecture mismatch.\n",
qPrintable(sourceFileName));
}
} else {
if (unmetDependencies.isEmpty()) {
if (options->verbose) {
fprintf(stdout, " -- Skipping %s, architecture mismatch.\n",
qPrintable(sourceFileName));
}
} else {
fprintf(stdout, " -- Skipping %s. It has unmet dependencies: %s.\n",
qPrintable(sourceFileName),
qPrintable(unmetDependencies.join(QLatin1Char(','))));
}
}
....
}PVS-Studio warns: V571 [CWE-571] Recurring check. The 'if (unmetDependencies.isEmpty())' condition was already verified in line 2203. main.cpp 2209
At first this code seems absolutely adequate. Just normal code that creates hints. But let's take a closer look. If the unmetDependencies.isEmpty() condition was met and executed once, it is not going to be executed for the second time. This is not a big deal, because the author was planning to display the same message. There is no real error, but the code is overly complicated. One can simplify it like this:
bool copyQtFiles(Options *options)
{
....
if (unmetDependencies.isEmpty()) {
if (options->verbose) {
fprintf(stdout, " -- Skipping %s, architecture mismatch.\n",
qPrintable(sourceFileName));
}
} else {
fprintf(stdout, " -- Skipping %s. It has unmet dependencies: %s.\n",
qPrintable(sourceFileName),
qPrintable(unmetDependencies.join(QLatin1Char(','))));
}
....
}Fragment #76: "Code smell" - a complex ternary operator
bool QDockAreaLayoutInfo::insertGap(....)
{
....
QDockAreaLayoutItem new_item
= widgetItem == nullptr
? QDockAreaLayoutItem(subinfo)
: widgetItem ? QDockAreaLayoutItem(widgetItem)
: QDockAreaLayoutItem(placeHolderItem);
....
}PVS-Studio warns: V547 [CWE-571] Expression 'widgetItem' is always true. qdockarealayout.cpp 1167
We could be dealing with a real bug here. But I am more inclined to believe the developers reworked this code several times and got an unexpectedly and unnecessarily complicated code block with redundant statements. You can reduce it down to the following:
QDockAreaLayoutItem new_item
= widgetItem == nullptr
? QDockAreaLayoutItem(subinfo) : QDockAreaLayoutItem(widgetItem);Fragment #77: "Code smell" – excessive protection
typedef unsigned int uint;
ReturnedValue TypedArrayCtor::virtualCallAsConstructor(....)
{
....
qint64 l = argc ? argv[0].toIndex() : 0;
if (scope.engine->hasException)
return Encode::undefined();
// ### lift UINT_MAX restriction
if (l < 0 || l > UINT_MAX)
return scope.engine->throwRangeError(QLatin1String("Index out of range."));
uint len = (uint)l;
if (l != len)
scope.engine->throwRangeError(
QStringLiteral("Non integer length for typed array."));
....
}PVS-Studio warns: V547 [CWE-570] Expression 'l != len' is always false. qv4typedarray.cpp 306
Someone worried too much that a value from a 64-bit variable might not fit into the unsigned 32-bit variable. And used two checks at once. The second check is redundant.
The following code is more than enough:
if (l < 0 || l > UINT_MAX)Then you can safely delete the snippet below. This will not endanger your code's reliability in any way.
uint len = (uint)l;
if (l != len)
scope.engine->throwRangeError(
QStringLiteral("Non integer length for typed array."));I can keep doing this, but I'll stop. I think you get the idea.
One can draw a nice conclusion here: the use of PVS-Studio will benefit your code in several ways - you can remove errors and simplify your code.
I stopped after I described 77 defects. This is a beautiful number, and I wrote more than enough to shape an article. However, this does not mean there are no more mistakes PVS-Studio can find. While studying the log, I was very quick. I skipped everything that required more than 2 minutes of my time to figure out whether it was a mistake :).
This is why I always urge you not to rely on our articles that explore your errors, but to use PVS-Studio on your projects yourself instead.
Static analysis is awesome! After you introduce the PVS-Studio into your development process, it will save your time and brain cells by finding many mistakes right after you write new code. It is much more fun to gather with your team for code review and discuss high-level errors and the efficiency of the implemented algorithms instead of typos. Moreover, as my experience shows, these nasty typos are always hiding, even if you check your code with your eyes. So let the software look for them instead.
If you have any more questions or objections, I invite you to read the following article: "Why you should choose the PVS-Studio static analyzer to integrate into your development process". I give this article a 90% chance to be able to answer your questions :). If you are in the 10% - message us, let's talk :).