Geekly Articles each Day
📜 ⬆️ ⬇️

Checking the Qt 5 framework

Qt and PVS-Studio
Static code analysis tools allow you to eliminate a lot of errors at the stage of writing the program text. For example, you can quickly identify and eliminate typos. Some programmers sincerely believe that they do not make typos and stupid mistakes. This is not true. Such mistakes are made by everyone. This article will serve as a good demo. Errata can be found even in such a quality and tested project as Qt.

Qt


Qt is a cross-platform software development toolkit in the C ++ programming language. It allows you to run software written with it in most modern operating systems by simply compiling the program for each OS without changing the source code. It includes all the main classes that may be required in the development of application software, ranging from GUI elements and ending with classes for working with the network, databases and XML. Qt is completely object-oriented, easily extensible, and supports component programming techniques. [source: Wikipedia]

References:We will work with Qt version 5.2.1. For analysis, we use PVS-Studio version 5.15.

I want to note that PVS-Studio managed to find errors, despite the fact that the Qt project was checked with the help of Klocwork and Coverity analyzers. How regularly these analyzers are used, I do not know. However, Klocwork and Coverity is mentioned in bugtracker and ChangeLog-xxx files. I also found a mention on the Internet that Qt is regularly checked using PC-lint.
')

Features of Qt Project Verification


For a change, we checked Qt with the help of a new mechanism that appeared in PVS-Studio Standalone. No one knows about the mechanism yet, and we will remind from time to time in articles about its existence. What is this new mysterious and beautiful mechanism?

Sometimes it is difficult to check a project with PVS-Studio if it is built with nmake and the like. It is required to integrate PVS-Studio into the assembly, which is not so easy. At least, this clearly makes it difficult to quickly try and evaluate the tool.

Now, a new mode of operation has appeared in PVS-Studio, which will greatly facilitate the work with such projects. The analyzer can monitor the parameters with which the compiler is launched and collects all the information necessary for verification. It is enough to tell the analyzer when to start surveillance and when to stop it.

You can monitor the compiler from both the GUI application and the command line. How it all works and how to use the new mode is described in more detail in the article:

Yevgeny Ryzhkov. PVS-Studio now supports any build system on Windows and any compiler. Easy and out of the box .

This article describes how the Qt project was tested in the monitoring mode from the command line.

Be sure to read this article so that there are fewer misunderstandings. For example, while the compilation of a project is being monitored, it is not possible to program in parallel. If you compile files from another project, information about them will be collected and they will also be checked. As a result, messages related to another project will be included in the report. Get a "mess".

Test results


In general, I got the following impression about the code:

The Qt code is qualitative and contains almost no errors related to the dangerous features of the C ++ language. But a lot of ordinary typos.

This article will well demonstrate that regardless of professionalism, all developers make typos. The benefits of static code analysis were, are and will be. Suppose the analyzer found 10 typos on a single launch. So, if you use it regularly, then by now it would have prevented hundreds or thousands of errors. This is a huge time saver. It is much more profitable to detect the error immediately after its appearance in the code, than to find it when debugging the code or thanks to the user's complaint.

Dive into the wonderful world of typos


Welcome to a wondrous world of typos

Typo n1
bool QWindowsUser32DLL::initTouch() { QSystemLibrary library(QStringLiteral("user32")); registerTouchWindow = ....; unregisterTouchWindow = ....; getTouchInputInfo = ....; closeTouchInputHandle = ....; return registerTouchWindow && unregisterTouchWindow && getTouchInputInfo && getTouchInputInfo; }

PVS-Studio Warning: V501 There’s the '&&' operator. qwindowscontext.cpp 216

Four variables are assigned values. These four variables must be verified. But because of a typo, only 3 are checked. In the last line, instead of 'getTouchInputInfo', it should say 'closeTouchInputHandle'.

Typo N2
 QWindowsNativeImage *QWindowsFontEngine::drawGDIGlyph(....) { .... int iw = gm.width.toInt(); int ih = gm.height.toInt(); if (iw <= 0 || iw <= 0) return 0; .... }

PVS-Studio Warning: V501 | || ' operator: iw <= 0 || iw <= 0 qwindowsfontengine.cpp 1095

There is no height check stored in the variable 'ih'.

Typo N3, N4

This error applies to tests. A good example of how static analysis complements unit tests. More on this topic: " How does static analysis complement TDD ?"
 inline bool qCompare(QImage const &t1, QImage const &t2, ....) { .... if (t1.width() != t2.width() || t2.height() != t2.height()) { .... }

PVS-Studio warning: V501 there is no need for a '! =' Operator: t2.height ()! = T2.height () qtest_gui.h 101

The function of comparing two pictures incorrectly compares their height. Or rather, does not compare at all.

Error reproduced using Copy-Paste. Exactly the same comparison can be seen in the same file below (line 135).

Typo n5

I apologize that I formatted the code ugly. The lines were too long.
 void QXmlSimpleReader::setFeature( const QString& name, bool enable) { .... } else if ( name == QLatin1String( "http://trolltech.com/xml/features/report-start-end-entity") || name == QLatin1String( "http://trolltech.com/xml/features/report-start-end-entity")) { .... }

PVS-Studio Warning: V501 | || ' operator. qxml.cpp 3249

The variable 'name' is compared twice with the same string. Above in the code, there is a similar comparison. There, the variable is compared with these two lines:By analogy, we can assume that in the above code, the variable 'name' should be compared with:Typo N6, N7, N8, N9
 QString DayTimeDuration::stringValue() const { .... if(!m_hours && !m_minutes && !m_seconds && !m_seconds) .... }

PVS-Studio warning: V501 There are identical sub-expressions '! M_seconds' of the operator. qdaytimeduration.cpp 148

Forgot about milliseconds. Milliseconds are stored in the 'm_mseconds' variable. The check should be:
 if(!m_hours && !m_minutes && !m_seconds && !m_mseconds)

Identical error with milliseconds is still in three places:Typo n10
 QV4::ReturnedValue QQuickJSContext2DPrototype::method_getImageData( QV4::CallContext *ctx) { .... qreal x = ctx->callData->args[0].toNumber(); qreal y = ctx->callData->args[1].toNumber(); qreal w = ctx->callData->args[2].toNumber(); qreal h = ctx->callData->args[3].toNumber(); if (!qIsFinite(x) || !qIsFinite(y) || !qIsFinite(w) || !qIsFinite(w)) .... }

PVS-Studio warning: V501 There are identical sub-expressions! QIsFinite (w) operator. qquickcontext2d.cpp 3305

There is no check for the 'h' variable. Instead, the variable 'w' is checked twice.

Typo n11
 AtomicComparator::ComparisonResult IntegerComparator::compare(const Item &o1, const AtomicComparator::Operator, const Item &o2) const { const Numeric *const num1 = o1.as<Numeric>(); const Numeric *const num2 = o1.as<Numeric>(); if(num1->isSigned() || num2->isSigned()) .... }

V656 Variables 'num1', 'num2' are initialized. It's probably not an error or un-optimized code. Consider inspecting the 'o1.as <Numeric> ()' expression. Check lines: 220, 221. qatomiccomparators.cpp 221

The variables 'num1' and 'num2' are initialized with the same value. Below both of these variables are checked. This is suspicious. After all, it suffices to check the value of only one variable.

Most likely, the variable 'num2' should be initialized by an expression where the argument 'o2' is used:
 const Numeric *const num1 = o1.as<Numeric>(); const Numeric *const num2 = o2.as<Numeric>();

Typo n12
 void Atlas::uploadBgra(Texture *texture) { const QRect &r = texture->atlasSubRect(); QImage image = texture->image(); if (image.format() != QImage::Format_ARGB32_Premultiplied || image.format() != QImage::Format_RGB32) { .... }

V547 Expression is always true. Probably the '&&' operator should be used here. qsgatlastexture.cpp 271

The condition in this code does not make sense. She is always true. To make it easier to understand what is wrong here, I will give a simplified example:
 int a = ...; if (a != 1 || a != 2)

A variable will always be unequal.

I don't know what the correct code should look like. Maybe it should be like this:
 if (image.format() == QImage::Format_ARGB32_Premultiplied || image.format() == QImage::Format_RGB32) {

Or so:
 if (image.format() != QImage::Format_ARGB32_Premultiplied && image.format() != QImage::Format_RGB32) {

Typo n13
 void QDeclarativeStateGroupPrivate::setCurrentStateInternal( const QString &state, bool ignoreTrans) { .... QDeclarativeTransition *transition = (ignoreTrans || ignoreTrans) ? 0 : findTransition(currentState, state); .... }

PVS-Studio Warning: V501 | || ' operator: ignoreTrans || ignoreTrans qdeclarativestategroup.cpp 442

Something is wrong here. How exactly wanted to write check, it is not clear to me.

Typo N14
 QV4::ReturnedValue QQuickJSContext2DPrototype::method_createPattern(....) { .... if (repetition == QStringLiteral("repeat") || repetition.isEmpty()) { pattern->patternRepeatX = true; pattern->patternRepeatY = true; } else if (repetition == QStringLiteral("repeat-x")) { pattern->patternRepeatX = true; } else if (repetition == QStringLiteral("repeat-y")) { pattern->patternRepeatY = true; } else if (repetition == QStringLiteral("no-repeat")) { pattern->patternRepeatY = false; pattern->patternRepeatY = false; } else { //TODO: exception: SYNTAX_ERR } .... }

PVS-Studio warning: V519 The 'pattern-> patternRepeatY' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1775, 1776. qquickcontext2d.cpp 1776

Two consecutive assignments are made to the variable 'patternRepeatY':
 pattern->patternRepeatY = false; pattern->patternRepeatY = false;

I think it should be written here:
 } else if (repetition == QStringLiteral("no-repeat")) { pattern->patternRepeatX = false; pattern->patternRepeatY = false; } else {

Incorrect use of C ++


Misuse of the C ++ language

As I said before, most of the errors are common typos. Almost no errors associated with improper use of the C ++ language. But a couple of such errors still found.

Beautiful error associated with the priority of operations
 bool QConfFileSettingsPrivate::readIniLine(....) { .... char ch; while (i < dataLen && ((ch = data.at(i) != '\n') && ch != '\r')) ++i; .... }

V593 Consider reviewing the A = B! = C 'kind. The expression is calculated as the following: 'A = (B! = C)'. qsettings.cpp 1702

The loop is designed to find the end of the line. The sign of the end of the line is the symbol '\ n' or '\ r'.

Inside the condition, the symbol must be taken and compared with '\ n' and '\ r'. The error occurs because the priority of the '! =' Operator is higher than that of the '=' operator. Because of this, not the character code is written to the 'ch' variable, but 'true' or 'false'. As a result, the comparison of '\ r' no longer plays any role.

Place brackets so that the error is more noticeable:
 while (i < dataLen && ((ch = (data.at(i) != '\n')) && ch != '\r'))

Due to an error, the end of the line is considered only the '\ n' character. If strings end with a '\ r' character, the function will not work correctly.

The correct code is:
 while (i < dataLen && (ch = data.at(i)) != '\n' && ch != '\r')

Loss of accuracy
 bool QWindowsTabletSupport::translateTabletPacketEvent() { .... const double radAzim = (packet.pkOrientation.orAzimuth / 10) * (M_PI / 180); .... }

V636 The 'packet.pkOrientation.orAzimuth / 10' expression was implicitly casted from 'int' type to 'double' type. Consider using a fractional part. An example: double A = (double) (X) / Y ;. qwindowstabletsupport.cpp 467

The variable 'packet.pkOrientation.orAzimuth' is of type 'int'. This integer variable is divided by the number 10. It is suspicious that the result of the division is then used in conjunction with the values ​​of the type 'double'. The end result is also placed in a variable of type 'double'.

Such an integer division is not necessarily a mistake. Perhaps the code is written exactly as intended. But as practice shows, this is often a flaw, leading to a loss of accuracy of calculations.

For example, let the variable 'packet.pkOrientation.orAzimuth' be 55. Then the result of the calculations will be equal to:

(55/10) * (3.14159 ... / 180) = 5 * 0.01745 ... = 0.087266 ...

Accuracy can be significantly increased. It is enough to declare a constant 10 as a double type: "(packet.pkOrientation.orAzimuth / 10.0 ) * (M_PI / 180)". Now the result of the calculation is:

(55 / 10.0) * (3.14159 ... / 180) = 5.5 * 0.01745 ... = 0.095993 ...

Such inaccuracies occur frequently, as programmers do not pay attention to expressions where variables of different types are mixed. Such inattention often leads also to 64-bit errors (see mixed arithmetic ).

The analyzer detects another 51 suspicious integer divisions. Perhaps some expressions will be less accurate than the developers wanted. I give them a list: qt-v636.txt .

Pointless pointer checks

If memory is allocated using the 'new' operator, it has long been pointless to check for a pointer to equality to zero. If memory fails to allocate, an exception will be thrown. Of course, you can make the 'new' operator return 0, but now it's not about these cases.

However, programmers forget from time to time, and a meaningless check reappears in the code.
 HRESULT STDMETHODCALLTYPE QWindowsEnumerate::Clone( IEnumVARIANT **ppEnum) { QWindowsEnumerate *penum = 0; *ppEnum = 0; penum = new QWindowsEnumerate(array); if (!penum) return E_OUTOFMEMORY; .... }

PVS-Studio warning: V668 um using um um allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated. The exception will be generated in the case of memory allocation error. qwindowsmsaaaccessible.cpp 141

There are a few more such checks: main.cpp 127, qaudiodevicefactory.cpp 236, qaudiodevicefactory, and scrp 263, qaudiobuffer.cpp 488, mfvideorendevelopes, and others. , positionpollfactory.cpp 60.

Dark side


The dark side

There are two code fragments about which I cannot say that this is exactly a mistake. I do not know the project architecture and implementation features. However, even if this is not an error, then this is the dark side of C ++ programming.
 class Q_CORE_EXPORT QObject { .... virtual ~QObject(); virtual bool event(QEvent *); virtual bool eventFilter(QObject *, QEvent *); .... }; QObject *QQmlVME::run(....) { .... QObject *o = (QObject *)operator new(instr.typeSize + sizeof(QQmlData)); ::memset(static_cast<void *>(o), 0, instr.typeSize + sizeof(QQmlData)); .... }

PVS-Studio warning: V598 The 'memset' function is used to configure the fields of the 'QObject' class. Virtual method table will be maintained by this. qqmlvme.cpp 658

The QObject class has virtual functions. This means that the object stores a pointer to a table of virtual methods. It seems to me not a good idea to initialize such objects using the memset () function.

Another caveat: V598 The 'memset' function is used to configure the fields of the 'QObject' class. Virtual method table will be maintained by this. qdeclarativevme.cpp 286

Null pointer dereferencing


Perhaps these errors could be attributed to typos. But I prefer to separate them into a separate group. So they seem darker and more serious.

Note. The division of errors into groups is always quite conditional. Often one and the same error can be classified as a typo, as a vulnerability, as an overrun of an array, and so on.

Let's return to the null pointers.

Null pointer dereferencing due to typo
 QV4::ReturnedValue QQuickJSContext2DPixelData::getIndexed( QV4::Managed *m, uint index, bool *hasProperty) { .... if (!m) return m->engine()->currentContext()->throwTypeError(); .... }

PVS-Studio warning: V522 Dereferencing of the null pointer 'm' might take place. qquickcontext2d.cpp 3169

Apparently, the operator '!' here is superfluous. A simple typo leading to a serious error.

The dereference of the null pointer in the error handler
 void QDocIndexFiles::readIndexSection(....) { .... DocNode* dn = qdb_->findGroup(groupNames[i]); if (dn) { dn->addMember(node); } else { .... qDebug() << "DID NOT FIND GROUP:" << dn->name() << "for:" << node->name(); } .... }

PVS-Studio warning: V522 Dereferencing of the null pointer 'dn' might take place. qdocindexfiles.cpp 539

If an error occurs, a diagnostic message should be printed. It will try to take the name of a non-existent object: dn-> name ().

82 potential null pointer dereferencing

In almost all projects, including Qt, there are flaws in working with null pointers. Often the check is carried out after the pointer has been used. It is not always a mistake. It happens that the pointer in principle can not be equal to zero.

In any case, such places deserve study and refactoring. Even if there is no error, an extra pointer check confuses the programmer when reading the code.

Consider one of the fragments of the dangerous code:
 static int gray_raster_render(....) { const QT_FT_Outline* outline = (const QT_FT_Outline*)params->source; .... /* return immediately if the outline is empty */ if ( outline->n_points == 0 || outline->n_contours <= 0 ) return 0; if ( !outline || !outline->contours || !outline->points ) return ErrRaster_Invalid_Outline; .... }

PVS-Studio warning: V595 The 'outline' pointer was used before it was verified against nullptr. Check lines: 1746, 1749. qgrayraster.c 1746

I think the error appeared at the moment when they wanted to optimize the function of gray_raster_render (). Most likely, these lines were inserted into the already completed function code:
 /* return immediately if the outline is empty */ if ( outline->n_points == 0 || outline->n_contours <= 0 ) return 0;

The trouble is that the 'outline' pointer may be null. The check for pointer zero equality is slightly lower.

The analyzer found 81 more potential errors. I will give diagnostic messages list: qt-v595.txt .

Unanswered Questions


Questions without answers

There are strange code fragments about which it is difficult to say how they appeared and what was actually planned to write. Probably, these are typos, possibly unfinished code, possibly unsuccessful refactoring.

Double check
 QWindowsFontEngine::~QWindowsFontEngine() { .... if (QWindowsContext::verboseFonts) if (QWindowsContext::verboseFonts) qDebug("%s: font='%s", __FUNCTION__, qPrintable(_name)); .... }

PVS-Studio warning: V571 Recurring check. The 'if (QWindowsContext :: verboseFonts)' condition was already verified in line 369. qwindowsfontengine.cpp 370

Why double check the same thing? Perhaps one check is superfluous. Perhaps you forgot to check something else.

Double assignment
 void Moc::parse() { .... index = def.begin + 1; namespaceList += def; index = rewind; .... }

PVS-Studio warning: V519 The 'index' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 568, 570. moc.cpp 570

Why are 'index' variables assigned to different values?

There are some more similar strange code fragments:Suspicion of missing 'break' statement
 bool GSuggestCompletion::eventFilter(QObject *obj, QEvent *ev) { .... switch (key) { case Qt::Key_Enter: case Qt::Key_Return: doneCompletion(); consumed = true; case Qt::Key_Escape: editor->setFocus(); popup->hide(); consumed = true; case Qt::Key_Up: case Qt::Key_Down: case Qt::Key_Home: case Qt::Key_End: case Qt::Key_PageUp: case Qt::Key_PageDown: break; .... }

PVS-Studio warning: V519 The 'consumed' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 110, 115. googlesuggest.cpp 115

Is the break statement forgotten or not?

It seemed to the analyzer that the variable 'consumed' could be assigned the value 'true' two times in a row. There is a possibility that the break statement is forgotten. But I'm not so sure. Maybe you just need to delete the first assignment: "consumed = true;".

Suspicion of extra 'break' operator
 bool QHelpGenerator::registerVirtualFolder(....) { .... while (d->query->next()) { d->namespaceId = d->query->value(0).toInt(); break; } .... }

PVS-Studio warning: V612 An unconditional 'break' within a loop. qhelpgenerator.cpp 429

Is it correct that the loop will immediately stop due to the 'break' operator?

Another such code snippet lives here: qhelpgenerator.cpp 642

miscellanea


Be patient. The article will end soon. There are still a few mixed errors.

Incorrect use of the toLower () function
 int main(int argc, char **argv) { .... QByteArray arg(argv[a]); .... arg = arg.mid(1); arg.toLower(); if (arg == "o") .... }

PVS-Studio warning: V530 The return value of the function 'toLower' is required to be utilized. main.cpp 72

The 'toLower ()' function does not modify the object. It returns a copy of the object that will hold the lowercase characters.

Another error: V530 The return value of the function "toLower" is required to be utilized. main.cpp 1522

Array bounds

The situation is complicated. Please focus.

There is such a numbered type:
 typedef enum { JNone, JCausing, JDual, JRight, JTransparent } Joining;

Remember that JTransparent == 4.

Now consider the getNkoJoining () function:
 static Joining getNkoJoining(unsigned short uc) { if (uc < 0x7ca) return JNone; if (uc <= 0x7ea) return JDual; if (uc <= 0x7f3) return JTransparent; if (uc <= 0x7f9) return JNone; if (uc == 0x7fa) return JCausing; return JNone; }

What matters to us is that this function can return a 'JTransparent'. That is, the function can return 4.

The program has a two-dimensional array 'joining_table':
 static const JoiningPair joining_table[5][4] = { .... };

And now the actual place where the error may occur:
 static void getNkoProperties(....) { .... Joining j = getNkoJoining(chars[0]); ArabicShape shape = joining_table[XIsolated][j].form2; .... }

PVS-Studio warning: V557 Array overrun is possible. The value of 'j' index could reach 4. harfbuzz-arabic.c 516

As we remember, the getNkoJoining () function can return the value 4. Thus, we refer to the cell of the joining_table array [...] [4]. It is unacceptable. There will be going beyond the array boundary.

Same conditions
 void Node::setPageType(const QString& t) { if ((t == "API") || (t == "api")) pageType_ = ApiPage; else if (t == "howto") pageType_ = HowToPage; else if (t == "overview") pageType_ = OverviewPage; else if (t == "tutorial") pageType_ = TutorialPage; else if (t == "howto") pageType_ = HowToPage; else if (t == "article") pageType_ = ArticlePage; else if (t == "example") pageType_ = ExamplePage; else if (t == "ditamap") pageType_ = DitaMapPage; }

PVS-Studio warning: V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 386, 392. node.cpp 386

Twice performed proving (t == "howto"). I think one of them is superfluous.

Here are a couple of similar warnings:Doing the same thing
 void QBluetoothServiceDiscoveryAgentPrivate::_q_deviceDiscovered( const QBluetoothDeviceInfo &info) { if(mode == QBluetoothServiceDiscoveryAgent::FullDiscovery) { for(int i = 0; i < discoveredDevices.count(); i++){ if(discoveredDevices.at(i).address() == info.address()){ discoveredDevices.removeAt(i); } } discoveredDevices.prepend(info); } else { for(int i = 0; i < discoveredDevices.count(); i++){ if(discoveredDevices.at(i).address() == info.address()){ discoveredDevices.removeAt(i); } } discoveredDevices.prepend(info); } }

PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. qbluetoothservicediscoveryagent.cpp 402

Regardless of the condition, the same actions are performed.

Similarly: pcre_exec.c 5577, ditaxmlgenerator.cpp 1722, htmlgenerator.cpp 388.

Inherited errors


Qt uses several third-party libraries. There are errors in these libraries too. It turns out that these errors are in Qt. I did not describe them in the article, but I decided that I could mention them.

I did not look carefully at the third-party libraries, but I wrote out something: qt-3rdparty.txt .

Note. However, one should not think that, on the other hand, I watched Qt very carefully. The project is large and even a superficial examination is enough to write just such an article.

findings


PVS-Studio is an excellent, powerful analyzer that is able to find errors even in such high-quality and polished projects as the Qt framework.

It will help the development team save a lot of time, finding many errors at an early stage. If you use incremental analysis , then errors will be detected immediately after the files are compiled.

Additional links


  1. We regularly check open-source projects. For example, Tor, Chromium, Clang, Firebird, OpenCV. To all who are interested, come here: "An updated list of open-source projects that we tested with PVS-Studio ."
  2. Here you can download a trial version of PVS-Studio . At the beginning, 20 clicks are given to navigate the warnings. After posting information about yourself, another 200 clicks.


This article is in English.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Checking the Qt 5 Framework .

Slowpoke Question

Read the article and have a question?
Often our articles are asked the same questions. We collected the answers to them here: Answers to questions from readers of articles about PVS-Studio and CppCat, version 2014 . Please review the list.

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

More articles:


All Articles