Geekly Articles each Day
📜 ⬆️ ⬇️

Third check of Qt 5 with PVS-Studio

PVS-Studio & Qt

From time to time, our team re-checks projects about which we have already written articles. The next such rechecked project was Qt. Last time we checked it with PVS-Studio in 2014. Starting in 2014, the project began to be regularly checked using Coverity. It is interesting. Let's see if we can now find some interesting errors with PVS-Studio.

Qt


Previous articles:


This time, Qt Base (Core, Gui, Widgets, Network, ...) and Qt5 super module were tested . About Qt Creator, we plan to write a separate article later. For the test, we used the PVS-Studio static analyzer, the trial version of which you can download from the site.

In my subjective opinion, the Qt code has become better. Over the years since the last check, many new diagnostics have appeared in the PVS-Studio analyzer. Despite this, during the review of the warnings, I found not too many errors for a project of this size. I repeat once again that this is my personal impression. I did not do any special research on the density of errors then or now.
')
Regular checks using the Coverity static analyzer were most likely to have a positive effect on the quality of the code. From 2014, the Qt project ( qt-project ) began to be checked with the help of Coverity, and from 2016 - Qt Creator ( qt-creator ). My opinion: if you are developing an open source project, then Coverity Scan can be a good free solution that will significantly improve the quality and reliability of your projects.

However, as the reader can guess, if I had not noticed anything interesting in the PVS-Studio report, then there would have been no article :). And if there is an article, there are defects. Let's look at them. I wrote a total of 96 errors.

Bad Copy-Paste and Typos


Let's start with the classics of the genre, when the cause of the error is inattention. These errors are underestimated by programmers. Those who have not read, I recommend to see these two articles:


These are language errors. For example, the second article gives a lot of examples of errors in comparison functions written in C, C ++, and C # languages. Now, implementing Java language support in PVS-Studio, we encounter all the same error patterns. Here, for example, is the 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, it turns out that the purchaseSequence field is compared to itself. The correct option is:

 return that.purchaseSequence.equals(this.purchaseSequence) && that.purchaseNumber == this.purchaseNumber;

In general, everything is as usual, and the PVS-Studio analyzer will have to “rake the Augean stables” in Java projects. By the way, we invite everyone to take part in the testing of the Beta-version of PVS-Studio for Java, which should appear soon. To do this, write to us (select "I want an analyzer for Java").

Now back to the errors in the Qt project.

Defect n1

 static inline int windowDpiAwareness(HWND hwnd) { return QWindowsContext::user32dll.getWindowDpiAwarenessContext && QWindowsContext::user32dll.getWindowDpiAwarenessContext ? QWindowsContext::user32dll.getAwarenessFromDpiAwarenessContext( QWindowsContext::user32dll.getWindowDpiAwarenessContext(hwnd)) : -1; }

PVS-Studio Warning: V501 CWE-571 There are identical sub-expressions of QWindowsContext :: user32dll.getWindowDpiAwarenessContext of the operator. qwindowscontext.cpp 150

Any special explanation besides the analyzer message is not required here. It seems to me that the expression should have been like this:

 return QWindowsContext::user32dll.getAwarenessFromDpiAwarenessContext && QWindowsContext::user32dll.getWindowDpiAwarenessContext ? QWindowsContext::user32dll.getAwarenessFromDpiAwarenessContext( QWindowsContext::user32dll.getWindowDpiAwarenessContext(hwnd)) : -1;

Defect N2, N3

 void QReadWriteLockPrivate::release() { Q_ASSERT(!recursive); Q_ASSERT(!waitingReaders && !waitingReaders && !readerCount && !writerCount); freelist->release(id); }

PVS-Studio warning: V501 CWE-571 operator:! WaitingReaders &&! WaitingReaders qreadwritelock.cpp 632

The error is inside the condition of the Q_ASSERT macro, so it is not significant. But still this is a mistake. The waitingReaders variable is checked twice . And some other variable apparently forgot to check.

An identical error is found in the 625 line of the qreadwritelock.cpp file. Long live Copy-Paste! :)

Defect n4

 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 Warning: V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. qgraphicsscene_bsp.cpp 179

Most likely, the text block was copied, but forgot to fix it.

Defect n5

 enum FillRule { OddEvenFill, WindingFill }; QDataStream &operator>>(QDataStream &s, QPainterPath &p) { .... int fillRule; s >> fillRule; Q_ASSERT(fillRule == Qt::OddEvenFill || Qt::WindingFill); .... }

PVS-Studio warning: V768 CWE-571 The enumeration constant 'WindingFill' is used as a variable of a Boolean-type. qpainterpath.cpp 2479

Agree, this is a beautiful blooper! Q_ASSERT does not check anything, since the condition is always true. True condition due to the fact that the Qt :: WindingFill named constant is 1.

Defect n6

 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; .... }

Before reading the warning, try to notice a typo yourself. Having added a picture, I will help you not to read the message of the analyzer immediately :).

Time to think


PVS-Studio warning: V560 CWE-570 A part of conditional expression is always false: currentType == QMetaType :: Char. qvariant.cpp 3529

The condition “currentType == QMetaType :: Char” is checked in the first if . If the condition is met, the currentType variable is assigned the value QMetaType :: UInt . Therefore, the currentType variable can no longer be equal to QMetaType :: Char . Therefore, the analyzer reports that in the second if the subexpression “currentType == QMetaType :: Char” is always false.

In fact, the second if should be like this:

 if (targetTypeId == QMetaType::SChar || targetTypeId == QMetaType::Char) targetTypeId = QMetaType::UInt;


V560 Diagnostic Note

The report turned out to be a lot of warnings V560. However, I didn’t review them more, as soon as I found an interesting case for an article that was considered above as defect N6.

The vast majority of messages V560 can not be called false, but there is no use for them. In other words, to describe them in the article is not interesting. To understand exactly what I mean, consider one such case.

 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()) { // <= .... }

Warning PVS-Stuidio: V560 CWE-571 A part of the conditional expression is always true: doc. qtextdocument.cpp 2992

The analyzer is absolutely right that the doc pointer is not equal to nullptr when re-checking. But this is not a mistake, just a programmer reinsured. You can simplify the code by writing:

 if (doc->docHandle()) {

Defect n7

And the last case, which can be classified as a typo. The error occurs because of confusion in the names of constants, 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 Warning: V616 CWE-480 The "CursorShowing" is used in the bitwise operation. qwindowscursor.cpp 669

I have already analyzed this error in more detail in a separate small note: " Once again, the PVS-Studio analyzer turned out to be more attentive than a person ."

Security flaws


In fact, all the errors that are considered in this article can be called security defects. All of them are classified according to the Common Weakness Enumeration (see CWE ID in the analyzer messages). If errors are classified as CWE, then they are potentially a security threat. This is explained in more detail on the PVS-Studio SAST page .

However, a number of errors and want to highlight a separate group. Let's take a look at them.

Defect N8, N9

 bool QLocalServerPrivate::addListener() { .... SetSecurityDescriptorOwner(pSD.data(), pTokenUser->User.Sid, FALSE); SetSecurityDescriptorGroup(pSD.data(), pTokenGroup->PrimaryGroup, FALSE); .... }

PVS-Studio warnings:


There are various functions related to access control. The functions SetSecurityDescriptorOwner and SetSecurityDescriptorGroup are among them.

It is necessary to work with such functions very carefully. For example, be sure to check the status that they return. What happens if the call to these functions fails? No need to guess, you need to write code to handle such a case.

It is not necessary to take advantage of the lack of verification and turn such errors into vulnerabilities. However, this is in no case a place for risk, and you need to write more secure code.

Defect N10

 bool QLocalServerPrivate::addListener() { .... InitializeAcl(acl, aclSize, ACL_REVISION_DS); .... }

PVS-Studio warning: V530 CWE-252 The return value of the 'InitializeAcl' is required to be utilized. qlocalserver_win.cpp 144

The situation is similar to that discussed above.

Defect N11, N12

 static inline void sha1ProcessChunk(....) { .... quint8 chunkBuffer[64]; .... #ifdef SHA1_WIPE_VARIABLES .... memset(chunkBuffer, 0, 64); #endif }

PVS-Studio warning: V597 CWE-14 The compiler could delete the memset function call, which is used to flush the chunkBuffer buffer. The RtlSecureZeroMemory () function should be used to erase the private data. sha1.cpp 189

The compiler will remove the memset function call. Already many times in articles I analyzed this situation. I do not want to repeat. I refer to the article " Secure cleaning of private data ."

And one more error is in the same sha1.cpp file, on line 247.

Null pointers


It's time to talk about pointers. There are a lot of mistakes on this topic.

Defect n13

 QByteArray &QByteArray::append(const char *str, int len) { if (len < 0) len = qstrlen(str); if (str && len) { .... }

PVS-Studio Warning: V595 CWE-476 The 'str' pointer was used against nullptr. Check lines: 2118, 2119. qbytearray.cpp 2118

The classic situation is when the pointer is used at the beginning, and then checked for equality nullptr . This is a very common pattern of error, and we meet it regularly in almost all projects.

Defect N14, N15

 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 warning: V595 CWE-476 The 'mobj' pointer was used against it. Check lines: 2671, 2672. qmetaobject.cpp 2671

Just in case, I provide the priv function body . For some reason, sometimes readers begin to invent situations in which the code will work. I don’t understand where this distrust and desire to see a cunning feature in an error come from :). For example, someone may comment in the comments that priv is a macro like:

 #define priv(A) foo(sizeof(A))

Then everything will work.

To avoid a place for such discussions, I try to provide code snippets, where all the information confirming the presence of an error is provided.

So, the pointer modj is dereferenced and then checked.

Next on the scene goes "mighty and terrible» Copy-Paste. Because of what exactly the same error is found 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 warning: V595 CWE-476 The 'mobj' pointer was used against it. Check lines: 2683, 2684. qmetaobject.cpp 2683

Defect N16-N21

Consider another example and, I think, is enough.

 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 warning: V595 CWE-476 The 'fragment.d' Check lines: 2238, 2241. qtextcursor.cpp 2238

All the same. Pay attention to the sequence of working with the pointer stored in the fragment.d variable.

Other errors of this type:


Defect N22-N33

There is a code where the pointer is checked, which returns a new operator. This is especially amusing because there are lots of places where the result of the malloc function is not checked (see the next group of errors).

 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 warning: V668 CWE-571 against-un un un un pointer pointer pointer against pointer pointer pointer pointer pointer pointer. The exception will be generated in the case of memory allocation error. qtranslator.cpp 596

Checking the pointer does not make sense, since in case of a memory allocation error, the exception std :: bad_alloc will be generated. If you want the new operator to return nullptr when there is a shortage of memory, then you should write:

 d->unmapPointer = new (std::nothrow) char[d->unmapLength];

The analyzer knows about this use case of the new operator and would not issue a warning in this case.

Other errors: I will give them the qt-V668.txt file .

Defect N34-N70

As promised, it’s now the error queue when the result of calling the functions malloc , calloc , strdup , etc. is not checked. These errors are more serious than it seems at first glance. More: " Why is it important to check that the malloc function returned ?"

 SourceFiles::SourceFiles() { nodes = (SourceFileNode**)malloc(sizeof(SourceFileNode*)*(num_nodes=3037)); for(int n = 0; n < num_nodes; n++) nodes[n] = nullptr; }

PVS-Studio warning: V522 CWE-690 There might be a null pointer 'nodes' dereferencing. Check lines: 138, 136. makefiledeps.cpp 138

The pointer is used without prior verification.

All these errors are of the same type, so I will not dwell on them in more detail. I will give the rest of the warning list: qt-V522-V575.txt .

Logical errors in conditions


Defect n71

 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 warning: V547 CWE-570 Expression 'buffer [i] <' \ 040 '&& buffer [i]>' \ 176 '' is always false. qedidparser.cpp 169

The function must perform the following action “Replace non-printable characters with dash”. However, she does not. Take a close look at this condition:

 if (buffer[i] < '\040' && buffer[i] > '\176')

It does not make sense. A character cannot be less than '\ 040' and no longer '\ 176'. In the condition it is necessary to use the operator '||'. The correct code is:

 if (buffer[i] < '\040' || buffer[i] > '\176')

Defect n72

A similar error, due to which Windows users are unlucky.

 #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); } #endif

The analyzer generates two warnings at once:


The logical error is in the condition:

 if (c < 'a' && c > 'z' && c < 'A' && c > 'Z')

As I understand it, the programmer wanted to find a character that is not a letter of the Latin alphabet. In this case, the condition should be:

 if ((c < 'a' || c > 'z') && (c < 'A' || c > 'Z'))

Defect n73

 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 warning: V616 CWE-480 The 'QAbstractItemView :: NoSelection' is used in the bitwise operation. itemviews.cpp 976

The QAbstractItemView :: NoSelection named constant is zero. Therefore, the subexpression (selectionMode & QAbstractItemView :: NoSelection) does not make sense. It will always get 0.

I think it should be written here:

 if (!m_index.isValid() || (selectionMode == QAbstractItemView::NoSelection))

Defect N74

The following code is hard for me to understand. He is wrong, but what he should be I do not know. A comment on a function does not help me 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 warning: V547 CWE-570 Expression 'message' is always false. qwindowscontext.cpp 802

The programmer probably assumes that the FormatMessage function will change the value of the message pointer. But it is not. The FormatMessage function cannot change the pointer value, since it is passed to the function by value. Here is the prototype of this function:

 DWORD __stdcall FormatMessageW( DWORD dwFlags, LPCVOID lpSource, DWORD dwMessageId, DWORD dwLanguageId, LPWSTR lpBuffer, DWORD nSize, va_list *Arguments );


Potential memory leaks


Defect N75-N92

 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 warning: 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

Increasing the buffer is implemented in a dangerous way. If the realloc function cannot allocate memory, it will return NULL . This NULL will be immediately placed in the children variable and there will be no possibility to somehow free the buffer allocated earlier. There is a memory leak.

Similar errors: qt-701.txt .

miscellanea


N93 defect

 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 warning: V610 CWE-758 Unspecified behavior. Check the shift operator '>>'. The left operand '(- 2147483647 - 1)' is negative. qdrawhelper.cpp 4015

You cannot move a negative INT_MIN value. This is an unspecified behavior, and one cannot rely on the result of such an operation. The upper bits can be either 0 or 1.

Defect n94

 void QObjectPrivate::addConnection(int signal, Connection *c) { .... if (signal >= connectionLists->count()) connectionLists->resize(signal + 1); ConnectionList &connectionList = (*connectionLists)[signal]; .... if (signal < 0) { .... }

PVS-Studio warning: V781 CWE-129 Perhaps there is a mistake in program logic. Check lines: 397, 413. qobject.cpp 397

A check (signal <0) indicates that the value of the signal argument may be negative. However, this argument was previously used to index the array. It turns out that the check is performed too late. The program will already be disrupted.

Defect N95

 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 warning: V519 CWE-563 The 'lastNamespaceDeclaration' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 3188, 3194. qxmlstream.cpp 3194

I will highlight the essence of the error:

 if (inEmptyElement) { lastNamespaceDeclaration = tag.namespaceDeclarationsSize; } lastNamespaceDeclaration = namespaceDeclarations.size();

Defect N96

 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

All the same as in the previous case. Notice the done variable.

Conclusion


Even superficially looking at the report, I wrote out almost 100 errors. I am pleased with the results of the work of PVS-Studio.

Of course, such rare code checks have nothing to do with improving the quality and reliability of the code. They only demonstrate the capabilities of the code analyzer. Static analysis tools should be used regularly. In this case, they reduce the cost of fixing errors and protect applications from many potential vulnerabilities.

Thanks for attention. To keep abreast of our new publications, I invite you to subscribe to one of our channels:
  1. VK.com: pvsstudio_rus
  2. Old School RSS: viva64-blog-en
  3. Twitter: @pvsstudio_rus
  4. Instagram: @pvsstudio_rus
  5. Telegram: @pvsstudio_rus



If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. A Third Check of Qt 5 with PVS-Studio

Source: https://habr.com/ru/post/426485/

More articles:


All Articles