Every now and then, we re-check projects that we already checked and mentioned in our articles in the past. Qt is one of them. The last time we checked it with PVS-Studio was in 2014. Starting with 2014, the project has been regularly checked with Coverity, which makes things more interesting. Let's see if PVS-Studio can find any cool bugs this time.
The previous articles:
This time we checked Qt Base (Core, Gui, Widgets, Network, ...) and Qt5 super module. As for Qt Creator, we plan on writing a separate article about it later. The check was done with the PVS-Studio static analyzer; you can download the demo version from our website.
Personally, I think that the code quality of Qt has improved. During the years since the last check, we have added lots of new diagnostics to PVS-Studio. Despite that, a quick review of the analyzer warnings showed that there were relatively few bugs for a project that size. As I said, that's my own impression of the code; I didn't do any special research on the error density, either earlier or now.
It seems the high code quality results from the regular checks with the Coverity static analyzer. Starting with 2014, the developers have been using it to check their project, and starting with 2016, Qt Creator. In my opinion, if you develop an open-source project, Coverity Scan is a good choice among free tools and can greatly improve the quality and reliability of your projects.
Anyway, I obviously wouldn't have written this article if I hadn't found anything worthy in the PVS-Studio report :). And since it's here, it means I found some bugs. Let's see what we've got here. The total number of defects I noted down is 96.
Let's start with a classic: errors resulting from inattention. Such errors are often underestimated, and if you haven't yet read these two articles, I recommend doing so:
Errors of this type are common in every language. For example, the second article above shows lots of bug examples in comparison functions written in C, C++, and C#. Now, as we work on adding Java support to PVS-Studio, we are seeing the same error patterns. Here's, for instance, an error we recently found in the Hibernate library:
public boolean equals(Object other) {
if (other instanceof Id) {
Id that = (Id) other;
return purchaseSequence.equals(this.purchaseSequence) &&
that.purchaseNumber == this.purchaseNumber;
}
else {
return false;
}
}If you look closely, you'll notice that the purchaseSequence field is compared with itself. This is the correct version:
return that.purchaseSequence.equals(this.purchaseSequence) &&
that.purchaseNumber == this.purchaseNumber;It's the same old story, and now PVS-Studio will have to "clean the Augean stables" inside Java projects as well. By the way, everyone interested is welcome to take part in the beta testing of PVS-Studio for Java, which is due to release soon. E-mail us if you want to participate (select the subject "I want to analyze Java").
Going back to the bugs in Qt.
Defect 1
static inline int windowDpiAwareness(HWND hwnd)
{
return QWindowsContext::user32dll.getWindowDpiAwarenessContext &&
QWindowsContext::user32dll.getWindowDpiAwarenessContext
? QWindowsContext::user32dll.getAwarenessFromDpiAwarenessContext(
QWindowsContext::user32dll.getWindowDpiAwarenessContext(hwnd))
: -1;
}PVS-Studio diagnostic message: V501 CWE-571 There are identical sub-expressions 'QWindowsContext::user32dll.getWindowDpiAwarenessContext' to the left and to the right of the '&&' operator. qwindowscontext.cpp 150
This case doesn't need any special comment besides the message. I think the expression was meant to look like this:
return QWindowsContext::user32dll.getAwarenessFromDpiAwarenessContext &&
QWindowsContext::user32dll.getWindowDpiAwarenessContext
? QWindowsContext::user32dll.getAwarenessFromDpiAwarenessContext(
QWindowsContext::user32dll.getWindowDpiAwarenessContext(hwnd))
: -1;Defects 2, 3
void QReadWriteLockPrivate::release()
{
Q_ASSERT(!recursive);
Q_ASSERT(!waitingReaders && !waitingReaders &&
!readerCount && !writerCount);
freelist->release(id);
}PVS-Studio diagnostic message: V501 CWE-571 There are identical sub-expressions to the left and to the right of the '&&' operator: !waitingReaders &&!waitingReaders qreadwritelock.cpp 632
The error is inside the condition in the Q_ASSERT macro, which makes it a minor defect, but it's still an error. The waitingReaders variable is checked twice, which means some other variable wasn't checked at all.
The same bug is found in line 625 of the qreadwritelock.cpp file. Hail to copy-paste! :)
Defect 4
QString QGraphicsSceneBspTree::debug(int index) const
{
....
if (node->type == Node::Horizontal) {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
} else {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
}
....
}PVS-Studio diagnostic message: V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. qgraphicsscene_bsp.cpp 179
It looks like the programmer copied this block of code but forgot to modify it.
Defect 5
enum FillRule {
OddEvenFill,
WindingFill
};
QDataStream &operator>>(QDataStream &s, QPainterPath &p)
{
....
int fillRule;
s >> fillRule;
Q_ASSERT(fillRule == Qt::OddEvenFill || Qt::WindingFill);
....
}PVS-Studio diagnostic message: V768 CWE-571 The enumeration constant 'WindingFill' is used as a variable of a Boolean-type. qpainterpath.cpp 2479
This one is so cool! Q_ASSERT doesn't check anything as the condition is always true. And it's true because the value of the named constant Qt::WindingFill is 1.
Defect 6
bool QVariant::canConvert(int targetTypeId) const
{
....
if (currentType == QMetaType::SChar || currentType == QMetaType::Char)
currentType = QMetaType::UInt;
if (targetTypeId == QMetaType::SChar || currentType == QMetaType::Char)
targetTypeId = QMetaType::UInt;
....
}Try to find the bug on your own before going on to read the warning. To make sure you don't look at it right away, here's a nice picture :).
PVS-Studio diagnostic message: V560 CWE-570 A part of conditional expression is always false: currentType == QMetaType::Char. qvariant.cpp 3529
The "currentType == QMetaType::Char" condition is checked in the first if statement. If it is true, the currentType variable is assigned the value QMetaType::UInt. It means there's no way it could become equal to QMetaType::Char after that. That's why the analyzer is telling us that the "currentType == QMetaType::Char" subexpression in the second if statement is always false.
The second if should actually look like this:
if (targetTypeId == QMetaType::SChar || targetTypeId == QMetaType::Char)
targetTypeId = QMetaType::UInt;Note on diagnostic V560
There were lots of V560 warnings in the report, but I stopped reading when I found an interesting case to include in the article - see Defect 6 above.
Most of the V560 warnings can't be called false positives but they are still of no use. In other words, they aren't interesting to discuss. Here's one example to explain what I mean.
QString QTextHtmlExporter::findUrlForImage(const QTextDocument *doc, ....)
{
QString url;
if (!doc)
return url;
if (QTextDocument *parent = qobject_cast<QTextDocument *>(doc->parent()))
return findUrlForImage(parent, cacheKey, isPixmap);
if (doc && doc->docHandle()) { // <=
....
}PVS-Studio diagnostic message: V560 CWE-571 A part of conditional expression is always true: doc. qtextdocument.cpp 2992
The analyzer is totally correct when saying that the doc pointer is always not equal to nullptr in the second check. But it's not a bug; the programmer was just playing safe. The code can be simplified as follows:
if (doc->docHandle()) {Defect 7
This is the last case we could classify as a typo. The error is the result of confusing the constants' names, which differ only in the case of the first letter.
class QWindowsCursor : public QPlatformCursor
{
public:
enum CursorState {
CursorShowing,
CursorHidden,
CursorSuppressed
};
....
}
QWindowsCursor::CursorState QWindowsCursor::cursorState()
{
enum { cursorShowing = 0x1, cursorSuppressed = 0x2 };
CURSORINFO cursorInfo;
cursorInfo.cbSize = sizeof(CURSORINFO);
if (GetCursorInfo(&cursorInfo)) {
if (cursorInfo.flags & CursorShowing)
....
}PVS-Studio diagnostic message: V616 CWE-480 The 'CursorShowing' named constant with the value of 0 is used in the bitwise operation. qwindowscursor.cpp 669
I already discussed this defect in detail in a small separate post some time ago: "Once again the PVS-Studio analyzer has proved to be more attentive than a person".
To tell the truth, all the bugs discussed here could be called security issues; they all fall into the Common Weakness Enumeration classification (see the CWE ID tag in the analyzer warnings). Errors registered in the CWE are potentially dangerous from the security viewpoint. For more details on this topic, see the page PVS-Studio SAST.
However, some of the bugs call for being put in a separate group. Let's take a look at them.
Defects 8, 9
bool QLocalServerPrivate::addListener()
{
....
SetSecurityDescriptorOwner(pSD.data(), pTokenUser->User.Sid, FALSE);
SetSecurityDescriptorGroup(pSD.data(), pTokenGroup->PrimaryGroup, FALSE);
....
}PVS-Studio diagnostic messages:
There are functions that deal with access control, and the functions SetSecurityDescriptorOwner and SetSecurityDescriptorGroup are among them.
You should be very cautious when using these functions. Always check on the status they return. What if a call to them fails? Don't make guesses, just write code to handle that case.
Missing checks aren't necessarily easy to exploit to turn defects like that into vulnerabilities. But you still don't want to take the risk. Write safer code.
Defect 10
bool QLocalServerPrivate::addListener()
{
....
InitializeAcl(acl, aclSize, ACL_REVISION_DS);
....
}PVS-Studio diagnostic message: V530 CWE-252 The return value of function 'InitializeAcl' is required to be utilized. qlocalserver_win.cpp 144
This one is similar to the previous case.
Defects 11, 12
static inline void sha1ProcessChunk(....)
{
....
quint8 chunkBuffer[64];
....
#ifdef SHA1_WIPE_VARIABLES
....
memset(chunkBuffer, 0, 64);
#endif
}PVS-Studio diagnostic message: V597 CWE-14 The compiler could delete the 'memset' function call, which is used to flush 'chunkBuffer' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha1.cpp 189
The compiler will delete the call to the memset function. I already discussed this situation in many posts before and don't want to repeat myself. See this article: "Safe Clearing of Private Data".
Another vulnerability is found in the same file sha1.cpp, line 247.
Now it's time to talk about pointers. There are quite a lot of errors from this group.
Defect 13
QByteArray &QByteArray::append(const char *str, int len)
{
if (len < 0)
len = qstrlen(str);
if (str && len) {
....
}PVS-Studio diagnostic message: V595 CWE-476 The 'str' pointer was utilized before it was verified against nullptr. Check lines: 2118, 2119. qbytearray.cpp 2118
This is a typical situation: a pointer is first used and then checked against nullptr. This error pattern is very common, and we see it all the time in almost every project.
Defects 14, 15
static inline const QMetaObjectPrivate *priv(const uint* data)
{ return reinterpret_cast<const QMetaObjectPrivate*>(data); }
bool QMetaEnum::isFlag() const
{
const int offset = priv(mobj->d.data)->revision >= 8 ? 2 : 1;
return mobj && mobj->d.data[handle + offset] & EnumIsFlag;
}PVS-Studio diagnostic message: V595 CWE-476 The 'mobj' pointer was utilized before it was verified against nullptr. Check lines: 2671, 2672. qmetaobject.cpp 2671
I'm citing the body of the priv function just in case. For some reason, readers will sometimes come up with scenarios where they believe this code would work. I wonder where this mistrust and desire to view a bug as a tricky feature come from :). For instance, somebody could write a comment suggesting that priv is a macro of this pattern:
#define priv(A) foo(sizeof(A))In this case, they say, everything will be fine.
To avert any such debates, I try to give all the necessary information when citing code snippets to prove that the code is indeed faulty.
So, the modj pointer is dereferenced and then checked.
Then the "Great and Powerful" copy-paste comes into play, creating a clone of this bug in the isScoped function:
bool QMetaEnum::isScoped() const
{
const int offset = priv(mobj->d.data)->revision >= 8 ? 2 : 1;
return mobj && mobj->d.data[handle + offset] & EnumIsScoped;
}PVS-Studio diagnostic message: V595 CWE-476 The 'mobj' pointer was utilized before it was verified against nullptr. Check lines: 2683, 2684. qmetaobject.cpp 2683
Defects 16-21
The last example from this group.
void QTextCursor::insertFragment(const QTextDocumentFragment &fragment)
{
if (!d || !d->priv || fragment.isEmpty())
return;
d->priv->beginEditBlock();
d->remove();
fragment.d->insert(*this);
d->priv->endEditBlock();
if (fragment.d && fragment.d->doc)
d->priv->mergeCachedResources(fragment.d->doc->docHandle());
}PVS-Studio diagnostic message: V595 CWE-476 The 'fragment.d' pointer was utilized before it was verified against nullptr. Check lines: 2238, 2241. qtextcursor.cpp 2238
Nothing new here. Note the order of operations performed on the pointer stored in the fragment.d variable.
Other bugs of this type:
Defects 22-33
Some checks test the pointer returned by the new operator. This is especially funny considering that there are lots of cases where the return result of the malloc function is not checked at all (see the next group of defects).
bool QTranslatorPrivate::do_load(const QString &realname,
const QString &directory)
{
....
d->unmapPointer = new char[d->unmapLength];
if (d->unmapPointer) {
file.seek(0);
qint64 readResult = file.read(d->unmapPointer, d->unmapLength);
if (readResult == qint64(unmapLength))
ok = true;
}
....
}PVS-Studio diagnostic message: V668 CWE-571 There is no sense in testing the 'd->unmapPointer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qtranslator.cpp 596
The pointer check makes no sense because memory allocation failure will raise an std::bad_alloc exception. If the developers wanted the new operator to return nullptr when memory can't be allocated, they should have written it as follows:
d->unmapPointer = new (std::nothrow) char[d->unmapLength];The analyzer knows about this implementation of the new operator and doesn't issue warnings on it.
Other defects: see the file qt-V668.txt.
Defects 34-70
As I promised, here's a group of errors that have to do with missing checks of the values returned by the functions malloc, calloc, strdup, etc. These are more severe than you might think. See the article "Why it is important to check what the malloc function returned" for details.
SourceFiles::SourceFiles()
{
nodes = (SourceFileNode**)malloc(sizeof(SourceFileNode*)*(num_nodes=3037));
for(int n = 0; n < num_nodes; n++)
nodes[n] = nullptr;
}PVS-Studio diagnostic message: V522 CWE-690 There might be dereferencing of a potential null pointer 'nodes'. Check lines: 138, 136. makefiledeps.cpp 138
The pointer is used without a prior check.
All these defects are alike, so I won't go into much detail. The rest of warnings of this type are listed in qt-V522-V575.txt.
Defect 71
QString QEdidParser::parseEdidString(const quint8 *data)
{
QByteArray buffer(reinterpret_cast<const char *>(data), 13);
// Erase carriage return and line feed
buffer = buffer.replace('\r', '\0').replace('\n', '\0');
// Replace non-printable characters with dash
for (int i = 0; i < buffer.count(); ++i) {
if (buffer[i] < '\040' && buffer[i] > '\176')
buffer[i] = '-';
}
return QString::fromLatin1(buffer.trimmed());
}PVS-Studio diagnostic message: V547 CWE-570 Expression 'buffer[i] < '\040' && buffer[i] > '\176'' is always false. qedidparser.cpp 169
The function is meant to "Replace non-printable characters with dash", which it doesn't. Let's look into the following condition:
if (buffer[i] < '\040' && buffer[i] > '\176')It makes no sense. A character cannot be less than '\040' and greater than '\176' at the same time. The '||' operator should be used instead:
if (buffer[i] < '\040' || buffer[i] > '\176')Defect 72
Another similar bug, which will affect Windows users.
#if defined(Q_OS_WIN)
static QString driveSpec(const QString &path)
{
if (path.size() < 2)
return QString();
char c = path.at(0).toLatin1();
if (c < 'a' && c > 'z' && c < 'A' && c > 'Z')
return QString();
if (path.at(1).toLatin1() != ':')
return QString();
return path.mid(0, 2);
}
#endifThis code triggers two warnings at once:
The logical error is found in the following condition:
if (c < 'a' && c > 'z' && c < 'A' && c > 'Z')As far as I understand, the developer's intention was to find a character that didn't belong to the Latin alphabet. If so, the condition should look like this:
if ((c < 'a' || c > 'z') && (c < 'A' || c > 'Z'))Defect 73
enum SelectionMode {
NoSelection,
SingleSelection,
MultiSelection,
ExtendedSelection,
ContiguousSelection
};
void QAccessibleTableCell::unselectCell()
{
QAbstractItemView::SelectionMode selectionMode = view->selectionMode();
if (!m_index.isValid() || (selectionMode & QAbstractItemView::NoSelection))
return;
....
}PVS-Studio diagnostic message: V616 CWE-480 The 'QAbstractItemView::NoSelection' named constant with the value of 0 is used in the bitwise operation. itemviews.cpp 976
Since the value of the named constant QAbstractItemView::NoSelection is zero, the (selectionMode & QAbstractItemView::NoSelection) subexpression is meaningless: it will always evaluate to 0.
I think the authors intended to write the following:
if (!m_index.isValid() || (selectionMode == QAbstractItemView::NoSelection))Defect 74
The snippet below is something that I can't fully figure out. It's faulty, but I'm still not sure what its correct version should look like. The comment doesn't help either.
// Re-engineered from the inline function _com_error::ErrorMessage().
// We cannot use it directly since it uses swprintf_s(), which is not
// present in the MSVCRT.DLL found on Windows XP (QTBUG-35617).
static inline QString errorMessageFromComError(const _com_error &comError)
{
TCHAR *message = nullptr;
FormatMessage(
FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM,
NULL, DWORD(comError.Error()), MAKELANGID(LANG_NEUTRAL,SUBLANG_DEFAULT),
message, 0, NULL);
if (message) {
const QString result = QString::fromWCharArray(message).trimmed();
LocalFree(static_cast<HLOCAL>(message));
return result;
}
if (const WORD wCode = comError.WCode())
return QString::asprintf("IDispatch error #%u", uint(wCode));
return QString::asprintf("Unknown error 0x0%x", uint(comError.Error()));
}PVS-Studio diagnostic message: V547 CWE-570 Expression 'message' is always false. qwindowscontext.cpp 802
The programmer was probably assuming that the FormatMessage function would change the value of the message pointer, but that's wrong. The FormatMessage function can't do that because the pointer is passed by value. Here's the function's prototype:
DWORD
__stdcall
FormatMessageW(
DWORD dwFlags,
LPCVOID lpSource,
DWORD dwMessageId,
DWORD dwLanguageId,
LPWSTR lpBuffer,
DWORD nSize,
va_list *Arguments
);Defects 75-92
struct SourceDependChildren {
SourceFile **children;
int num_nodes, used_nodes;
SourceDependChildren() : children(nullptr), num_nodes(0), used_nodes(0) { }
~SourceDependChildren() { if (children) free(children); children = nullptr; }
void addChild(SourceFile *s) {
if(num_nodes <= used_nodes) {
num_nodes += 200;
children = (SourceFile**)realloc(children,
sizeof(SourceFile*)*(num_nodes));
}
children[used_nodes++] = s;
}
};PVS-Studio diagnostic message: V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'children' is lost. Consider assigning realloc() to a temporary pointer. makefiledeps.cpp 103
The buffer resizing is done in a dangerous way. If the realloc function fails to allocate the memory block, it will return a NULL value, which will be immediately assigned to the children variable, leaving no chance to free the previously allocated buffer. The program ends up with a memory leak.
Other similar defects: qt-701.txt.
Defect 93
template<class GradientBase, typename BlendType>
static inline const BlendType * QT_FASTCALL
qt_fetch_linear_gradient_template(....)
{
....
if (t+inc*length < qreal(INT_MAX >> (FIXPT_BITS + 1)) &&
t+inc*length > qreal(INT_MIN >> (FIXPT_BITS + 1))) {
....
}PVS-Studio diagnostic message: V610 CWE-758 Unspecified behavior. Check the shift operator '>>'. The left operand '(- 2147483647 - 1)' is negative. qdrawhelper.cpp 4015
You can't shift a negative INT_MIN value. This is unspecified behavior, and you can't rely on the result of such an operation. The most significant bits may happen to contain 0s as well as 1s.
Defect 94
void QObjectPrivate::addConnection(int signal, Connection *c)
{
....
if (signal >= connectionLists->count())
connectionLists->resize(signal + 1);
ConnectionList &connectionList = (*connectionLists)[signal];
....
if (signal < 0) {
....
}PVS-Studio diagnostic message: V781 CWE-129 The value of the 'signal' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 397, 413. qobject.cpp 397
The check (signal < 0) suggests that the value of the signal argument might be negative. But this argument was used earlier to index into an array. It means execution will reach the check too late; the program will have been affected by then.
Defect 95
bool QXmlStreamWriterPrivate::finishStartElement(bool contents)
{
....
if (inEmptyElement) {
write("/>");
QXmlStreamWriterPrivate::Tag &tag = tagStack_pop();
lastNamespaceDeclaration = tag.namespaceDeclarationsSize;
lastWasStartElement = false;
} else {
write(">");
}
inStartElement = inEmptyElement = false;
lastNamespaceDeclaration = namespaceDeclarations.size();
return hadSomethingWritten;
}PVS-Studio diagnostic message: V519 CWE-563 The 'lastNamespaceDeclaration' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3188, 3194. qxmlstream.cpp 3194
Here's the most important part:
if (inEmptyElement) {
lastNamespaceDeclaration = tag.namespaceDeclarationsSize;
}
lastNamespaceDeclaration = namespaceDeclarations.size();Defect 96
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);
....
}V519 CWE-563 The 'done' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 509, 511. qeffects.cpp 511
Just the same as in the previous case. Note the done variable.
Even a quick review revealed about 100 defects. I'm glad about how PVS-Studio performed.
Of course, occasional checks like that have nothing in common with improving code quality and reliability. They are just good enough to demonstrate what the analyzer is capable of. Static analysis tools must be used regularly: only then will they help reduce the price of bug fixing and protect your programs from many potential vulnerabilities.
Thanks for reading. Stay tuned by following our channels: