Третья проверка Qt 5 с помощью PVS-Studio

    PVS-Studio & Qt

    Время от времени наша команда повторно проверяет проекты, про которые мы уже писали статьи. Очередным таким перепроверенным проектом стал Qt. Последний раз мы проверяли его с помощью PVS-Studio в 2014 году. Начиная с 2014 года проект начал регулярно проверяться с помощью Coverity. Это интересно. Давайте посмотрим, удастся ли нам теперь найти какие-то интересные ошибки с помощью PVS-Studio.

    Qt


    Предыдущие статьи:


    В этот раз был проверен Qt Base (Core, Gui, Widgets, Network, ...) и Qt5 super module. Про Qt Creator мы позже планируем написать отдельную статью. Для проверки использовался статический анализатор PVS-Studio, пробную версию которого вы можете скачать с сайта.

    На мой субъективный взгляд, код Qt стал более качественным. За годы с момента последней проверки, в анализаторе PVS-Studio появилось много новых диагностик. Несмотря на это, в ходе обзорного изучения предупреждений я выявил не так уж много ошибок для проекта такого размера. Ещё раз повторю, что это моё индивидуальное впечатление. Каких-то специальных исследований на тему плотности ошибок ни тогда, ни сейчас я не делал.

    Положительно на качестве кода, скорее всего, сказались регулярные проверки с помощью статического анализатора Coverity. С 2014 года с помощью Coverity начал проверяться проект Qt (qt-project), а с 2016 — Qt Creator (qt-creator). Моё мнение: если вы разрабатываете открытый проект, то Coverity Scan может стать хорошим бесплатным решением, которое существенно улучшит качество и надёжность ваших проектов.

    Впрочем, как читатель может догадаться, если бы я ничего интересного не заметил в отчёте PVS-Studio, то и статьи бы не было :). А раз есть статья, то есть и дефекты. Давайте посмотрим на них. Всего я выписал 96 ошибок.

    Неудачный Copy-Paste и опечатки


    Начнём с классики жанра, когда причиной ошибки является невнимательность. Эти ошибки недооцениваются программистами. Тем, кто ещё не читал, рекомендую посмотреть две эти статьи:


    Эти ошибки межязыковые. Например, во второй статье приводится масса примеров ошибок в функциях сравнения, написанных на языках C, C++ и C#. Сейчас, реализуя в PVS-Studio поддержку языка Java, мы встречаем всё те же паттерны ошибок. Вот, например, ошибка, найденная нами недавно в библиотеке Hibernate:

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

    Если присмотреться, то оказывается, что поле purchaseSequence сравнивается само с собой. Правильный вариант:

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

    В общем, всё как всегда, и анализатору PVS-Studio предстоит «разгребать авгиевы конюшни» в Java-проектах. Кстати, приглашаем всех желающих принять участие в тестировании Beta-версии PVS-Studio for Java, которая должна появиться в ближайшее время. Для этого напишите нам (выберите пункт «Хочу анализатор для Java»).

    Теперь вернёмся к ошибкам в проекте Qt.

    Дефект N1

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

    Предупреждение PVS-Studio: V501 CWE-571 There are identical sub-expressions 'QWindowsContext::user32dll.getWindowDpiAwarenessContext' to the left and to the right of the '&&' operator. qwindowscontext.cpp 150

    Какого-то специального пояснения помимо сообщения анализатора здесь не требуется. Мне кажется, что выражение должно было быть таким:

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

    Дефект N2, N3

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

    Предупреждение PVS-Studio: V501 CWE-571 There are identical sub-expressions to the left and to the right of the '&&' operator: !waitingReaders &&!waitingReaders qreadwritelock.cpp 632

    Ошибка находится внутри условия макроса Q_ASSERT, поэтому она незначительна. Но всё равно это ошибка. Дважды проверяется переменная waitingReaders. А какую-то другую переменную, видимо, забыли проверить.

    Идентичная ошибка обнаруживает себя в 625 строке файла qreadwritelock.cpp. Да здравствует Copy-Paste! :)

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

    Скорее всего, блок текста был скопирован, но поправить его забыли.

    Дефект N5

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

    Предупреждение PVS-Studio: V768 CWE-571 The enumeration constant 'WindingFill' is used as a variable of a Boolean-type. qpainterpath.cpp 2479

    Согласитесь, это красивый ляп! Q_ASSERT ничего не проверяет, так как условие всегда истинно. Истинно условие из-за того, что именованная константа Qt::WindingFill равна 1.

    Дефект 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;
      ....
    }

    Прежде чем читать предупреждение, попробуйте заметить опечатку самостоятельно. Добавив картинку, я помогу вам не прочитать сообщение анализатора сразу :).

    Время подумать


    Предупреждение PVS-Studio: V560 CWE-570 A part of conditional expression is always false: currentType == QMetaType::Char. qvariant.cpp 3529

    Условие «currentType == QMetaType::Char» проверяется в первом if. Если условие выполняется, то переменной currentType присваивается значение QMetaType::UInt. Следовательно, далее переменная currentType уже никак не может быть равна QMetaType::Char. Поэтому анализатор и сообщает, что во втором if подвыражение «currentType == QMetaType::Char» всегда ложно.

    На самом деле, второй if должен быть таким:

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


    Примечание по диагностики V560

    В отчёте оказалось много предупреждений V560. Однако я не стал их более просматривать, как только нашёл интересный случай для статьи, который был рассмотрен выше как дефект N6.

    Подавляющее большинство сообщений V560 нельзя назвать ложными, но и проку от них нет. Другими словами, описывать их в статье неинтересно. Чтобы было понятно, что именно я имею в виду, рассмотрим один такой случай.

    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-Stuidio: V560 CWE-571 A part of conditional expression is always true: doc. qtextdocument.cpp 2992

    Анализатор абсолютно прав, что указатель doc при повторной проверке всегда не равен nullptr. Но это не ошибка, просто программист перестраховался. Можно упростить код, написав:

    if (doc->docHandle()) {

    Дефект N7

    И последний случай, который можно классифицировать как опечатку. Ошибка возникает из-за путаницы в именах констант, которые отличаются только регистром первой буквы.

    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: V616 CWE-480 The 'CursorShowing' named constant with the value of 0 is used in the bitwise operation. qwindowscursor.cpp 669

    Более подробно я уже разбирал эту ошибку в отдельной маленькой заметке: "В очередной раз анализатор PVS-Studio оказался внимательнее человека".

    Дефекты безопасности


    На самом деле, все ошибки, которые рассматриваются в этой статье, можно назвать дефектами безопасности. Все они классифицируются согласно Common Weakness Enumeration (см. CWE ID в сообщениях анализатора). Если ошибки классифицируются как CWE, то они потенциально представляют угрозу с точки зрения безопасности. Более подробно это объясняется на странице PVS-Studio SAST.

    Однако ряд ошибок так и хочется выделить в отдельную группу. Давайте взглянем на них.

    Дефект N8, N9

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

    Предупреждения PVS-Studio:

    • V530 CWE-252 The return value of function 'SetSecurityDescriptorOwner' is required to be utilized. qlocalserver_win.cpp 167
    • V530 CWE-252 The return value of function 'SetSecurityDescriptorGroup' is required to be utilized. qlocalserver_win.cpp 168

    Существуют различные функции, связанные с разграничением доступа. Функции SetSecurityDescriptorOwner и SetSecurityDescriptorGroup относятся к их числу.

    С такими функциями надо работать очень аккуратно. Например, обязательно надо проверять статус, который они возвращают. Что будет, если вызов этих функций закончится неудачей? Гадать не надо, надо написать код для обработки такого случая.

    Не обязательно есть возможность воспользоваться отсутствием проверки и превратить подобные ошибки в уязвимости. Однако это в любом случае не место для риска, и надо написать более безопасный код.

    Дефект N10

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

    Предупреждение PVS-Studio: V530 CWE-252 The return value of function 'InitializeAcl' is required to be utilized. qlocalserver_win.cpp 144

    Ситуация аналогична рассмотренной выше.

    Дефект N11, N12

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

    Предупреждение PVS-Studio: 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

    Компилятор удалит вызов функции memset. Уже много раз в статьях я разбирал эту ситуацию. Повторяться не хочется. Отсылаю к статье "Безопасная очистка приватных данных".

    И ещё одна ошибка находится в том же файле sha1.cpp, в строке 247.

    Нулевые указатели


    Пришло время поговорить об указателях. На эту тему нашлось весьма много ошибок.

    Дефект N13

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

    Предупреждение PVS-Studio: V595 CWE-476 The 'str' pointer was utilized before it was verified against nullptr. Check lines: 2118, 2119. qbytearray.cpp 2118

    Классическая ситуация, когда указатель в начале используется, а затем проверяется на равенство nullptr. Это очень распространённый паттерн ошибки, и мы встречаем его регулярно практически во всех проектах.

    Дефект 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: V595 CWE-476 The 'mobj' pointer was utilized before it was verified against nullptr. Check lines: 2671, 2672. qmetaobject.cpp 2671

    На всякий случай я привожу тело функции priv. Почему-то иногда читатели начинают придумывать ситуации, при которых код будет работать. Не понимаю, откуда берётся это недоверие и желание увидеть в ошибке хитрую фичу :). Например, кто-то может в комментариях предположить, что priv — это макрос вида:

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

    Тогда всё будет работать.

    Чтобы не было место подобным дискуссиям, я стараюсь приводить фрагменты кода, где предоставлена вся информация, подтверждающая наличие ошибки.

    Итак, указатель modj разыменовывается, а затем проверяется.

    Дальше на сцену выходит «могучий и ужасный» Copy-Paste. Из-за чего в точности такая же ошибка обнаруживается в функции isScoped:

    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: V595 CWE-476 The 'mobj' pointer was utilized before it was verified against nullptr. Check lines: 2683, 2684. qmetaobject.cpp 2683

    Дефект N16-N21

    Рассмотрим ещё один пример и, думаю, достаточно.

    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: V595 CWE-476 The 'fragment.d' pointer was utilized before it was verified against nullptr. Check lines: 2238, 2241. qtextcursor.cpp 2238

    Всё то же самое. Обратите внимание на последовательность работы с указателем, хранящимся в переменной fragment.d.

    Другие ошибки этого типа:

    • V595 CWE-476 The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1846, 1848. qapplication.cpp 1846
    • V595 CWE-476 The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1858, 1860. qapplication.cpp 1858
    • V595 CWE-476 The 'reply' pointer was utilized before it was verified against nullptr. Check lines: 492, 502. qhttpnetworkconnectionchannel.cpp 492
    • V595 CWE-476 The 'newHandle' pointer was utilized before it was verified against nullptr. Check lines: 877, 883. qsplitter.cpp 877
    • V595 CWE-476 The 'widget' pointer was utilized before it was verified against nullptr. Check lines: 2320, 2322. qwindowsvistastyle.cpp 2320
    • На самом деле ошибок больше. Мне быстро надоело изучать предупреждения V595, а для статьи я уже выписал достаточное количество фрагментов кода.

    Дефект N22-N33

    Есть код, где проверяется указатель, который возвращает оператор new. Особенно это забавно на фоне того, что есть масса мест, где не проверяется результат работы функции malloc (см. следующую группу ошибок).

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

    Проверка указателя не имеет смысла, так как в случае ошибки выделения памяти будет сгенерировано исключение std::bad_alloc. Если хочется, чтобы при нехватке памяти оператор new возвращал nullptr, то следовало написать:

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

    Анализатор знает про такой вариант использования оператора new и не выдал бы в этом случае предупреждение.

    Другие ошибки: приведу их файлом qt-V668.txt.

    Дефект N34-N70

    Как и обещал, теперь очередь ошибок, когда не проверяют результат вызова функций malloc, calloc, strdup и т.д. Эти ошибки серьезнее, чем кажется на первый взгляд. Подробнее: "Почему важно проверять, что вернула функция malloc".

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

    Предупреждение PVS-Studio: V522 CWE-690 There might be dereferencing of a potential null pointer 'nodes'. Check lines: 138, 136. makefiledeps.cpp 138

    Указатель используют без предварительной проверки.

    Все эти ошибки однотипны, поэтому я не буду на них останавливаться подробнее. Приведу остальные предупреждения списком: qt-V522-V575.txt.

    Логические ошибки в условиях


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

    Функция обязана выполнять следующее действие «Replace non-printable characters with dash». Однако она этого не делает. Взглянем внимательно вот на это условие:

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

    Оно не имеет смысла. Символ не может одновременно быть меньше '\040' и больше '\176'. В условии необходимо использовать оператор '||'. Правильный вариант кода:

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

    Дефект N72

    Аналогичная ошибка, из-за которой не повезёт Windows-пользователям.

    #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

    Анализатор выдаёт сразу два предупреждения:

    • V590 CWE-571 Consider inspecting the 'c < 'a' && c > 'z' && c < 'A' && c > 'Z'' expression. The expression is excessive or contains a misprint. qdir.cpp 77
    • V560 CWE-570 A part of conditional expression is always false: c > 'z'. qdir.cpp 77

    Логическая ошибка находится в условии:

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

    Как я понимаю, программист хотел найти символ, который не является буквой латинского алфавита. В этом случае условие должно быть таким:

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

    Дефект 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: V616 CWE-480 The 'QAbstractItemView::NoSelection' named constant with the value of 0 is used in the bitwise operation. itemviews.cpp 976

    Именованная константа QAbstractItemView::NoSelection равна нулю. Поэтому подвыражение (selectionMode & QAbstractItemView::NoSelection) не имеет смысла. Всегда будет получаться 0.

    Мне кажется, здесь следовало написать так:

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

    Дефект N74

    Следующий код мне понять трудно. Он неправильный, но каким он должен быть я не знаю. Комментарий к функции тоже мне не помогает.

    // 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: V547 CWE-570 Expression 'message' is always false. qwindowscontext.cpp 802

    Программист, наверное, предполагает, что функция FormatMessage изменит значение указателя message. Но это не так. Функция FormatMessage не может изменить значение указателя, так как он передаётся в функцию по значению. Вот прототип этой функции:

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


    Потенциальные утечки памяти


    Дефект 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: 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

    Увеличение буфера реализовано опасным образом. Если функция realloc не сможет выделить память, она вернёт значение NULL. Этот NULL будет сразу помещён в переменную children и не останется возможности как-то освободить буфер, выделенный ранее. Произойдёт утечка памяти.

    Аналогичные ошибки: qt-701.txt.

    Разное


    Дефект N93

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

    Нельзя сдвигать отрицательное значение INT_MIN. Это неуточнённое поведение, и полагаться на результат такой операции нельзя. Старшие биты могут оказаться как равными 0, так и 1.

    Дефект 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: 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

    Проверка (signal < 0) указывает на то, что значение аргумента signal может быть отрицательным. Однако этот аргумент ранее используется для индексации массива. Получается, что проверка выполняется слишком поздно. Работа программы уже будет нарушена.

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

    Выделю суть ошибки:

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

    Дефект 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

    Всё то же самое, что и в предыдущем случае. Обратите внимание на переменную done.

    Заключение


    Даже поверхностно просматривая отчёт, я выписал почти 100 ошибок. Я доволен результатами работы PVS-Studio.

    Конечно, подобные редкие проверки кода не имеют ничего общего с улучшением качества и надёжности кода. Они только демонстрируют возможности анализатора кода. Инструменты статического анализа должны применяться регулярно. В этом случае они снижают стоимость исправления ошибок и защищают приложения от многих потенциальных уязвимостей.

    Спасибо за внимание. Чтобы быть в курсе наших новых публикаций, приглашаю подписаться на один из наших каналов:
    1. VK.com: pvsstudio_rus
    2. «Олдскульный» RSS: viva64-blog-ru
    3. Twitter: @pvsstudio_rus
    4. Instagram: @pvsstudio_rus
    5. Telegram: @pvsstudio_rus



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. A Third Check of Qt 5 with PVS-Studio

    PVS-Studio

    463,00

    Ищем ошибки в C, C++ и C# на Windows, Linux, macOS

    Поделиться публикацией
    Комментарии 24
      0
      А багрепорт уже создан? Интересно было бы посмотреть: исправят?
      0

      А сколько стоит лицензия?
      About $60 for a developer per month
      About $30 for a developer per month
      Сколько будет стоить лицензия, чтобы поставить на билд-сервер, чтобы проверял автоматом после каждого пуша? 2,5 разработчика в команде, Qt, Linux.

        +2
        Сразу отмечу, что мы не продаём лицензии для отдельных людей/серверов. Для маленьких команд (2,5 разработчика) можно использовать бесплатный вариант лицензирования. Если такой вариант не подходит для проекта, то предлагаем обсудить возможные варианты лицензирования в индивидуальном порядке.
          0

          Спасибо, попробую бесплатный вариант.
          Мы не отдельные люди-серверы, у нас довольно большое предприятие, но именно на С++ разрабатывают 2,5 человека.
          И не зная цен сложно убеждать начальство выделить деньги на лицензию.

            0
            А почему так? Маленькие команды и отдельные разработчики с радостью бы купили подобное, если бы это стоило недорого, настраивалось в 2 клика и прекрасно интегрировалось в рабочие инструменты.

            Я работаю в экосистеме другого языка и пользуюсь 3 такими инструментами (1 из них платный, в составе IDE), 2 из них стоят в том числе и хуком на CI.
        +2
        Неправильное использование realloc() — классика :) Но кто ж читает маны…
          0

          Мне в студенчестве ещё первый начальник сказал — всегда теперь обращаю внимание. Сработало лучше мана :)
          А вообще, кмк если бы чаще были последствия, то бы больше внимания обращали. А поскольку эта штука очень редко и в основном незаметно стреляет, никто и не замечает...

            0
            Ну, это чисто сишная вешь, сейчас все на плюсах, да на других языках, с GC :)
            (впрочем, я и сам недавно смачно вляпался в man 7 signal-safety...)
          0
          Почему существует только x64 версия PVS-Studio?
          Занимаюсь embedded, для чего вполне хватает 4-ядерного AMD
            0
            Исчезающее малое количество разработчиков программируют в наши дни, используя для своей работы 32-битную ОС.
            P.S. Прошу не спутать разработку приложений для 32-битных систем и собственно сам процесс работы (написания кода). Я про второе.
            0
            Спасибо за проверку Qt!
            Дефект N74 — вроде бы всё ок, почему анализатор ругается?
              TCHAR *message = nullptr;
              FormatMessage(
                FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM,
                NULL, DWORD(comError.Error()), MAKELANGID(LANG_NEUTRAL,SUBLANG_DEFAULT),
                message, 0, NULL);

            Как раз таки вызывается FormatMessageW и передаётся указатель на message.
            Флаг FORMAT_MESSAGE_ALLOCATE_BUFFER говорит что message нужно инициализировать.
            Вроде всё просто же?
              0
              (Уже написали ниже, что ошибка действительно есть, но не та, про которую написал анализатор).
              +1
              Насчет FormatMessage(). Как написано в описании флага FORMAT_MESSAGE_ALLOCATE_BUFFER в этом случае нужно передавать не адрес буфера, а адрес указателя, куда будет помещен адрес буфера. Т.е. нужно передавать не message, а (LPTSTR)&message. В этом ошибка.
                0
                Qt вообще очень любят повсюду ставить инты (дефекты 94 и 95), по крайне непонятной причине…
                  0
                  Всегда было интересно, как работает проверка на «identical subexpressions»? Явно же не простым текстовым сравнением. Небось SMT solver'ы используете?
                    +1
                    Solver-ы не используются. Обыкновенный код сравнения поддеревьев, учитывающий возможные перестановки операндов и понимающий, что A[0+1] и A[1] это одно и тоже. Ну и на самом деле всё немного сложнее :).
                    0
                    OpenJDK будете перепроверять?
                      0
                      Да мы ко многим проектам возращаемся. Но без фанатизма. Про Qt забыли на 4.5 года, например.
                      0
                      Интересно, если посмотреть в тот же qimagescale.cpp, то там гораздо больше примеров V668, но PVS Studio на них не срабатывает. Дело в том, что проверка на nullptr после new происходит на сразу после new, а в коде вызывающем функцию, которая делает new а потом возвращает этот указатель. В общем, фич-реквест в PVS Stuido — научиться анализировать call stack для V668.
                        0
                        Кстати, Дефект N94 не является дефектом. Индекс действительно может быть отрицательным. И массив, в котором лезут в отрицательный элемент, на самом деле не массив, а наследник вектора, который перегружает operator[] чтобы обрабатывать этот случай.
                        Отрицательный индекс сигнала означает «все сигналы» что бы это ни значило…
                        Другой дело, что код запутанный выходит.
                          0

                          Дефект N13 — не то чтобы совсем дефект, скорее выбор программиста, который писал код или может код ревью делал
                          Из документации qstrlen:


                          A safe strlen() function.
                          
                          Returns the number of characters that precede the terminating '\0', or 0 if str is nullptr.

                          На всякий случай посмотрел определение функции:


                          inline uint qstrlen(const char *str)
                          { return str ? uint(strlen(str)) : 0; }

                          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                          Самое читаемое