Проверка исходного кода свободного графического редактора Krita 4.0

    Не так давно состоялся релиз новой версии свободного графического редактора Krita 4.0. Самое время проверить этот проект с помощью PVS-Studio.

    Picture 1


    Введение


    Примечательно, что разработчики уже использовали PVS-Studio в далеком 2015 году на версии Krita 2.9.2 и успешно исправляли с его помощью ошибки. Однако, видимо, с тех пор анализатор не использовали. Во всех наших статьях мы говорим о том, что важны именно регулярные проверки, ведь если бы разработчики продолжали пользоваться PVS-Studio, то ошибки, о которых я расскажу в этой статье, попросту не попали бы в релиз.

    Бесполезный range-based for


    Предупреждение PVS-Studio: V714 Variable row is not passed into foreach loop by a reference, but its value is changed inside of the loop. kis_algebra_2d.cpp 532

    DecomposedMatix::DecomposedMatix(const QTransform &t0)
    {
        ....
        if (!qFuzzyCompare(t.m33(), 1.0)) {
            const qreal invM33 = 1.0 / t.m33();
    
            for (auto row : rows) { // <=
                row *= invM33;
            }
        }
        ....
    }

    В данном примере программист, очевидно, хотел умножить каждый элемент контейнера rows на invM33, однако, этого не произойдёт. На каждой итерации цикла создаётся новая переменная с именем row, которая затем просто уничтожается. Чтобы исправить ошибку необходимо создавать ссылку на элемент, хранящийся в контейнере:

    for (auto &row : rows) {
        row *= invM33;
    }

    Некорректные условия


    Предупреждение PVS-Studio: V547 Expression 'j == 0' is always false. KoColorSpace.cpp 218

    QPolygonF KoColorSpace::estimatedTRCXYY() const
    {
      ....
      for (int j = 5; j>0; j--) {
        channelValuesF.fill(0.0);
        channelValuesF[i] = ((max / 4)*(5 - j));
    
        if (colorModelId().id() != "XYZA") {
          fromNormalisedChannelsValue(data, channelValuesF);
          convertPixelsTo(....);
          xyzColorSpace->normalisedChannelsValue(....);
        }
    
        if (j == 0) {                                 // <=
          colorantY = channelValuesF[1];
          if (d->colorants.size()<2) {
            d->colorants.resize(3 * colorChannelCount());
            d->colorants[i] = ....
              d->colorants[i + 1] = ....
              d->colorants[i + 2] = ....
          }
        }
      }
      ....
    }

    Программа никогда не зайдет в блок под условием j==0, поскольку это условие всегда ложно из-за того, что выше в цикле for накладывается ограничение j > 0.

    Аналогичные предупреждения анализатора:

    • V547 Expression 'x < 0' is always false. kis_selection_filters.cpp 334
    • V547 Expression 'y < 0' is always false. kis_selection_filters.cpp 342

    Предупреждение PVS-Studio: V560 A part of conditional expression is always true. KoTextLayoutArea.cpp 1622

    qreal KoTextLayoutArea::addLine(QTextLine &line,
                                    FrameIterator *cursor,
                                    KoTextBlockData &blockData)
    {
      if (!d->documentLayout->changeTracker()
       || !d->documentLayout->changeTracker()->displayChanges() // <=
       || !d->documentLayout->changeTracker()->...
       || !d->documentLayout->changeTracker()->...
       || !d->documentLayout->changeTracker()->elementById(....)
       || !d->documentLayout->changeTracker()->elementById(....)
       || ....
       || d->documentLayout->changeTracker()->displayChanges()) { // <=
         ....
      }
    }

    Если присмотреться, то можно заметить, что внутри данного сложного условия находится проверка вида (!a || a):

    d->documentLayout->changeTracker()->displayChanges() ||
    !d->documentLayout->changeTracker()->displayChanges()

    Такое условие является всегда истинным, из-за чего вся эта большая проверка становится бессмысленной, о чем и сообщает анализатор.

    Предупреждение PVS-Studio: V547 Expression 'n == 128' is always false. compression.cpp 110
    Предупреждение PVS-Studio: V547 Expression 'n > 128' is always false. compression.cpp 112

    quint32 decode_packbits(const char *src,
                            char* dst,
                            quint16 packed_len,
                            quint32 unpacked_len)
    {
        qint32    n;
        ....
        while (unpack_left > 0 && pack_left > 0)
        {
            n = *src;
            src++;
            pack_left--;
    
            if (n == 128) // <=
                continue;
            else if (n > 128) // <=
                n -= 256;
            ....
        }
        ....
    }

    В данном примере в переменную n типа qint32 записывается значение типа const char, получаемое при разыменовании указателя src, поэтому диапазон допустимых значений переменной n: [-128; 127]. Затем переменная n сравнивается с числом 128, хотя понятно, что результат такой проверки всегда false.

    Примечание: Проект собирается без -funsigned-char.

    Предупреждение PVS-Studio: V590 Consider inspecting the 'state == (- 3) || state != 0' expression. The expression is excessive or contains a misprint. psd_pixel_utils.cpp 335

    psd_status 
    psd_unzip_without_prediction(psd_uchar *src_buf, psd_int src_len,
                                 psd_uchar *dst_buf, psd_int dst_len)
    {
        do {
            state = inflate(&stream, Z_PARTIAL_FLUSH);
            if(state == Z_STREAM_END)
                break;
            if(state == Z_DATA_ERROR || state != Z_OK) // <=
                break;
        }  while (stream.avail_out > 0);
    }

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

        do {
            state = inflate(&stream, Z_PARTIAL_FLUSH);
            if(state != Z_OK)
                break;
        }  while (stream.avail_out > 0);

    Предупреждение PVS-Studio: V547 Expression is always false. SvgTextEditor.cpp 472

    void SvgTextEditor::setTextWeightDemi()
    {
        if (m_textEditorWidget.richTextEdit->textCursor()
              .charFormat().fontWeight() > QFont::Normal
            && m_textEditorWidget.richTextEdit->textCursor()
               .charFormat().fontWeight() < QFont::Normal) { // <=
            setTextBold(QFont::Normal);
        } else {
            setTextBold(QFont::DemiBold);
        }
    }

    Анализатор обнаружил условие вида (a > b && a < b), которое, очевидно, является всегда ложным. Затрудняюсь сказать, что именно хотел написать автор, однако, данный код однозначно является ошибочным и нуждается в исправлении.

    Очепятки


    Предупреждение PVS-Studio: V547 Expression is always true. Probably the '&&' operator should be used here. KoResourceItemChooser.cpp 408

    void KoResourceItemChooser::updatePreview(KoResource *resource)
    {
        ....
        if (image.format() != QImage::Format_RGB32 || // <=
        image.format() != QImage::Format_ARGB32 ||    // <=
        image.format() != QImage::Format_ARGB32_Premultiplied) {
    
            image = image.convertToFormat(....);
        }
        ....
    }

    Программист ошибся и вместо оператора && написал оператор ||, из-за чего все его условие стало бессмысленным, т.к. получилось условие вида: a != const_1 || a != const_2, которое является всегда истинным.

    Предупреждение PVS-Studio: V547 Expression is always true. Probably the '&&' operator should be used here. KoSvgTextShapeMarkupConverter.cpp 1000

    QString KoSvgTextShapeMarkupConverter::style(....)
    {
      ....
      if (format.underlineStyle() != QTextCharFormat::NoUnderline ||
          format.underlineStyle() != QTextCharFormat::SpellCheckUnderline)
      {
          ....
      }
      ....
    }

    Случай аналогичный предыдущему: перепутали логический оператор и вместо && написали ||.

    Предупреждение PVS-Studio: V501 There are identical sub-expressions 'sensor(FUZZY_PER_DAB, true)' to the left and to the right of the '||' operator. kis_pressure_size_option.cpp 43

    void KisPressureSizeOption::lodLimitations(....) const
    {
      if (sensor(FUZZY_PER_DAB, true) || sensor(FUZZY_PER_DAB, true)) {
          l->limitations << KoID("size-fade", i18nc("...."));
      }
    
      if (sensor(FADE, true)) {
          l->blockers << KoID("...."));
      }
    }

    Анализатор обнаружил ситуацию, когда слева и справа от оператора || находятся одинаковые выражения. Если взглянуть на перечисление DynamicSensorType:

    enum DynamicSensorType {
        FUZZY_PER_DAB,
        FUZZY_PER_STROKE,
        SPEED,
        FADE,
        ....
        UNKNOWN = 255
    };

    то становится ясно, что справа, скорее всего, хотели написать FUZZY_PER_STROKE, а не FUZZY_PER_DAB.

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

    Ошибки из-за Copy-Paste


    Picture 6



    Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: d->paragraphStylesDotXmlStyles.values(). KoTextSharedLoadingData.cpp 594

    QList<KoParagraphStyle *> 
    KoTextSharedLoadingData::paragraphStyles(bool stylesDotXml) const
    {
        return stylesDotXml ? 
            d->paragraphStylesDotXmlStyles.values() :
            d->paragraphStylesDotXmlStyles.values(); // <=
    }

    Здесь, скорее всего, скопировали блок then в тернарном операторе и забыли изменить имя вызываемого метода, из-за чего, в независимости от условия, всегда будет возвращаться одно значение.

    Судя по предыдущему методу:

    KoParagraphStyle *
    KoTextSharedLoadingData::paragraphStyle(const QString &name,
                                            bool stylesDotXml) const
    {
        return stylesDotXml ? 
            d->paragraphStylesDotXmlStyles.value(name) :
            d->paragraphContentDotXmlStyles.value(name);
    }

    в блоке else нужно написать paragraphContentDotXmlStyles, вместо paragraphStylesDotXmlStyles.

    Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: qFloor(axis). kis_transform_worker.cc 456

    void mirror_impl(KisPaintDeviceSP dev, qreal axis, bool isHorizontal)
    {
        ....
        int leftCenterPoint = qFloor(axis) < axis ? qFloor(axis) :
                                                    qFloor(axis); // <=
        int leftEnd = qMin(leftCenterPoint, rightEnd);
    
        int rightCenterPoint = qFloor(axis) < axis ? qCeil(axis) :
                                                     qFloor(axis);
        int rightStart = qMax(rightCenterPoint, leftStart);
        ....
    }

    Еще одно срабатывание, очень похожее на предыдущее. Возможно, в блоке then первого тернарного оператора хотели написать qCeil(axis), а не qFloor(axis), либо же условие здесь вообще лишнее.

    Предупреждение PVS-Studio: V656 Variables 'vx', 'vy' are initialized through the call to the same function. It's probably an error or un-optimized code. Check lines: 218, 219. KarbonSimplifyPath.cpp 219

    bool KarbonSimplifyPath::isSufficentlyFlat(QPointF curve[4])
    {
      qreal ux = 3 * curve[1].x() - 2 * curve[0].x() - curve[3].x();
      qreal uy = 3 * curve[1].y() - 2 * curve[0].y() - curve[3].y();
      qreal vx = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x(); // <=
      qreal vy = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x(); // <=
      ....
    }

    Данный код выглядит весьма подозрительно, так как, скорее всего, при написании формулы для vy скопировали предыдущую строку, но забыли поменять вызовы x() на y(). В случае, если ошибки здесь все-таки нет, лучше переписать код так:

    qreal vx = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x();
    qreal vy = vx;

    Предупреждение PVS-Studio: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 675, 679. KoTableCellStyle.cpp 679

    void KoTableCellStyle::loadOdfProperties(
        KoShapeLoadingContext &context,
        KoStyleStack &styleStack)
    {
      ....
      if (styleStack.hasProperty(KoXmlNS::style, "print-content"))
      {
        setPrintContent(styleStack.property(KoXmlNS::style,
                          "print-content") == "true");
      }
    
      if (styleStack.hasProperty(KoXmlNS::style, "repeat-content")) // <=
      {
        setRepeatContent(styleStack.property(KoXmlNS::style,
                           "repeat-content") == "true");
      }
    
      if (styleStack.hasProperty(KoXmlNS::style, "repeat-content")) // <=
      {
        setRepeatContent(styleStack.property(KoXmlNS::style,
                           "repeat-content") == "true");
      }
      ....
    }

    Два раза выполняется одна и та же проверка. В этом методе либо скопировали лишнего, либо что-то перепутали. Если ошибки здесь нет, то стоит удалить дублирующийся код.

    Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. kis_processing_applicator.cpp 227

    void KisProcessingApplicator::applyVisitorAllFrames(....)
    {
        KisLayerUtils::FrameJobs jobs;
    
        if (m_flags.testFlag(RECURSIVE)) {
            KisLayerUtils::updateFrameJobsRecursive(&jobs, m_node); // <=
        } else {
            KisLayerUtils::updateFrameJobsRecursive(&jobs, m_node); // <=
        }
        
        ....
    }

    Скорее всего, здесь скопировали код из блока then в блок else и забыли поменять вызываемый метод. Судя по коду проекта, в ветке else наверняка хотели написать KisLayerUtils::updateFrameJobs.

    Дублирующееся условие (ошибка в условии)


    Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 255, 269. KoInlineTextObjectManager.cpp 255

    void 
    KoInlineTextObjectManager::documentInformationUpdated(
    const QString &info, const QString &data)
    {
        if (info == "title") // <=
            setProperty(KoInlineObject::Title, data);
        else if (info == "description")
            setProperty(KoInlineObject::Description, data);
        else if (info == "abstract")
            setProperty(KoInlineObject::Comments, data);
        else if (info == "subject")
            setProperty(KoInlineObject::Subject, data);
        else if (info == "keyword")
            setProperty(KoInlineObject::Keywords, data);
        else if (info == "creator")
            setProperty(KoInlineObject::AuthorName, data);
        else if (info == "initial")
            setProperty(KoInlineObject::AuthorInitials, data);
        else if (info == "title") // <=
            setProperty(KoInlineObject::SenderTitle, data);
        else if (info == "email")
            setProperty(KoInlineObject::SenderEmail, data);
        ....
    }

    Здесь два раза выполняется одна и та же проверка. Скорее всего, во втором случае нужно было написать что-то вроде «sender-title».

    Ошибки при работе с константами из перечисления


    Предупреждение PVS-Studio: V768 The enumeration constant 'BatchMode' is used as a variable of a Boolean-type. KisMainWindow.cpp 811

    bool KisMainWindow::openDocument(const QUrl &url, OpenFlags flags)
    {
        if (!QFile(url.toLocalFile()).exists()) {
            if (!flags && BatchMode) {              // <=
                QMessageBox::critical(0,
                                      i18nc("....", "Krita"),
                                      i18n("....", url.url()));
            }
            ....
        }
        ....
    }

    BatchMode — это элемент перечисления OpenFlag со значением 0x2:

    enum OpenFlag {
        None = 0,
        Import = 0x1,
        BatchMode = 0x2,
        RecoveryFile = 0x4
    };

    Однако в данном примере с ним работают, как будто это переменная. В результате получается, что часть условия всегда истинна.

    При этом в коде выше с BatchMode работают корректно:

    if (flags & BatchMode) {
        newdoc->setFileBatchMode(true);
    }

    Из этого можно сделать вывод, что хотели написать нечто подобное:

    bool KisMainWindow::openDocument(const QUrl &url, OpenFlags flags)
    {
        if (!QFile(url.toLocalFile()).exists()) {
            if (!(flags & BatchMode)) {            // <=
                QMessageBox::critical(0,
                                      i18nc("....", "Krita"),
                                      i18n("....", url.url()));
            }
            ....
        }
        ....
    }


    Предупреждение PVS-Studio: V768 The enumeration constant 'State_Active' is used as a variable of a Boolean-type. KisOpenPane.cpp 104

    void paint(....) const override
    {
        QStyledItemDelegate::paint(painter, option, index);
    
        if(!(option.state & (int)(QStyle::State_Active &&  // <=
                                  QStyle::State_Enabled))) // <=
        {
            ....
        }
    }

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

    void paint(....) const override
    {
        QStyledItemDelegate::paint(painter, option, index);
    
        if(!(option.state & (int)(QStyle::State_Active |
                                  QStyle::State_Enabled)))
        {
            ....
        }
    }

    Подозрительные повторные присваивания


    Предупреждение PVS-Studio: V519 The 'value' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 61, 66. kis_draggable_tool_button.cpp 66

    int KisDraggableToolButton::continueDrag(const QPoint &pos)
    {
        ....
    
        if (m_orientation == Qt::Horizontal) {
            value = diff.x(); // <=
        } else {
            value = -diff.y(); // <=
        }
    
        value = diff.x() - diff.y(); // <=
    
        return value;
    }

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

    Предупреждение PVS-Studio: V519 The 'uf.f' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 263, 265. lut.h 265
    Предупреждение PVS-Studio: V519 The 'uf.f' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 271, 273. lut.h 273

    LutKey<float>(float min, float max, float precision) :
        m_min(min), m_max(max), m_precision(precision)
    {
        ....
        if(m_min > 0 && m_max > 0)
        {
            uf.f = m_min;                // <=
            m_tMin_p = uf.i >> m_shift;
            uf.f = m_max;                // <=
            m_tMax_p = uf.i >> m_shift;
            m_tMin_n = m_tMax_p;
            m_tMax_n = m_tMax_p;
        } 
        else if( m_max < 0)
        {
            uf.f = m_min;                // <=
            m_tMax_n = uf.i >> m_shift;
            uf.f = m_max;                // <=
            m_tMin_n = uf.i >> m_shift;
            m_tMin_p = m_tMax_n;
            m_tMax_p = m_tMax_n;
        }
        ....
    }

    Переменной uf.f два раза подряд присваиваются разные значения. Это подозрительно, и вполне возможно, что хотели присвоить значение какой-то другой переменной.

    Возможно, пропущен else


    Предупреждение PVS-Studio: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. SvgStyleWriter.cpp 82

    void SvgStyleWriter::saveSvgBasicStyle(KoShape *shape,
                                           SvgSavingContext &context)
    {
        if (!shape->isVisible(false)) {
            ....
        } if (shape->transparency() > 0.0) { // <=
            ....
        }
    }

    Здесь, возможно, забыли ключевое слово else. Даже если ошибки здесь нет, стоит поправить форматирование кода, чтобы не смущать анализатор и других программистов.

    Аналогичное предупреждение:
    • V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. transform_stroke_strategy.cpp 166

    Проблемы с нулевыми указателями


    Picture 3



    Предупреждение PVS-Studio: V595 The 'l' pointer was utilized before it was verified against nullptr. Check lines: 428, 429. kis_node_manager.cpp 428

    void KisNodeManager::moveNodeAt(....)
    {
        ....
        KisLayer *l = qobject_cast<KisLayer*>(parent.data());
        KisSelectionMaskSP selMask = l->selectionMask(); // <=
        if (m && m->active() && l && l->selectionMask()) // <=
        selMask->setActive(false);
        ....
    }

    Здесь указатель l сначала разыменовывают и только потом проверяют его на nullptr.

    Аналогичные предупреждения анализатора:

    • V595 The 'gradient' pointer was utilized before it was verified against nullptr. Check lines: 164, 166. kis_gradient_chooser.cc 164
    • V595 The 'm_currentShape' pointer was utilized before it was verified against nullptr. Check lines: 316, 325. ArtisticTextTool.cpp 316
    • V595 The 'painter()' pointer was utilized before it was verified against nullptr. Check lines: 87, 89. kis_grid_paintop.cpp 87
    • V595 The 'm_optionsWidget' pointer was utilized before it was verified against nullptr. Check lines: 193, 202. kis_tool_move.cc 193
    • ....

    Предупреждение PVS-Studio: V1004 The 'sb' pointer was used unsafely after it was verified against nullptr. Check lines: 665, 670. KisView.cpp 670

    void KisView::slotSavingStatusMessage(const QString &text,
                                          int timeout,
                                          bool isAutoSaving)
    {
        QStatusBar *sb = statusBar();
        if (sb) // <=
            sb->showMessage(text, timeout);
    
        KisConfig cfg;
    
        if (sb->isHidden() || // <=
            (!isAutoSaving && cfg.forceShowSaveMessages()) ||
            (cfg.forceShowAutosaveMessages() && isAutoSaving)) {
    
            viewManager()->showFloatingMessage(text, QIcon());
        }
    }

    Анализатор считает небезопасным подобное использование указателя sb после проверки его на nullptr. И действительно, в случае, если указатель будет нулевым (а это допускается, раз такое условие написано выше), то при вызове sb->isHidden() может произойти разыменование нулевого указателя. В качестве исправления можно добавить проверку на nullptr и во второе условие, либо еще как-то обработать эту ситуацию.

    Аналогичное предупреждение анализатора:

    • V1004 The 'd->viewManager' pointer was used unsafely after it was verified against nullptr. Check lines: 338, 365. KisView.cpp 365

    Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'slot' might take place. kis_spriter_export.cpp 568

    KisImportExportFilter::ConversionStatus KisSpriterExport::convert(
        KisDocument *document,
        QIODevice *io, 
        KisPropertiesConfigurationSP /*configuration*/)
    {
        ....
        SpriterSlot *slot = 0;                                   // <=
    
        // layer.name format: "base_name bone(bone_name) slot(slot_name)"
        if (file.layerName.contains("slot(")) {
            int start = file.layerName.indexOf("slot(") + 5;
            int end = file.layerName.indexOf(')', start);
            slot->name = file.layerName.mid(start, end - start); // <=
            slot->defaultAttachmentFlag = ....                   // <=
        }
        ....
    }

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

    Утечки памяти


    Предупреждение PVS-Studio: V773 The function was exited without releasing the 'svgSymbol' pointer. A memory leak is possible. SvgParser.cpp 681

    bool SvgParser::parseSymbol(const KoXmlElement &e)
    {
        ....
    
        KoSvgSymbol *svgSymbol = new KoSvgSymbol();         // <=
    
        // ensure that the clip path is loaded in local coordinates system
        m_context.pushGraphicsContext(e, false);
        m_context.currentGC()->matrix = QTransform();
        m_context.currentGC()->currentBoundingBox = QRectF(0.0, 0.0,
                                                           1.0, 1.0);
    
        QString title = e.firstChildElement("title").toElement().text();
    
        KoShape *symbolShape = parseGroup(e);
    
        m_context.popGraphicsContext();
    
        if (!symbolShape) return false;                     // <=
        ....
    }

    В этом примере при выходе из метода забыли освободить память, выделенную под svgSymbol. Это утечка памяти. В проекте много подобных утечек, но они достаточно однотипны, так что останавливаться на них я не буду.

    Аналогичные предупреждения анализатора:

    • V773 The function was exited without releasing the 'ppmFlow' pointer. A memory leak is possible. kis_ppm_import.cpp 249
    • V773 The function was exited without releasing the 'keyShortcut' pointer. A memory leak is possible. kis_input_manager_p.cpp 443
    • V773 The function was exited without releasing the 'layerRecord' pointer. A memory leak is possible. psd_layer_section.cpp 109
    • V773 The function was exited without releasing the 'filterStack' pointer. A memory leak is possible. FilterEffectResource.cpp 139
    • ....

    Проверки на nullptr после new


    Предупреждение PVS-Studio: V668 There is no sense in testing the 'charStyle' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CharacterGeneral.cpp 153

    bool KoPathShape::separate(QList<KoPathShape*> & separatedPaths)
    {
        ....
    
        Q_FOREACH (KoSubpath* subpath, d->subpaths) {
            KoPathShape *shape = new KoPathShape();
            if (! shape) continue; // <=
            ....
        }
    }

    Эта рубрика в наших статьях, кажется, уже стала постоянной. Бессмысленно проверять указатель на nullptr, если память была выделена с помощью оператора new. При невозможности выделить память, оператор new генерирует исключение std::bad_alloc(), а не возвращает nullptr. Чтобы исправить этот код, можно добавить обработчик исключения, либо использовать new (std::nothrow).

    Аналогичные предупреждения анализатора:

    • V668 There is no sense in testing the 'factory' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. TestKoShapeFactory.cpp 36
    • V668 There is no sense in testing the 'parStyle' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ParagraphGeneral.cpp 199
    • V668 There is no sense in testing the 'spline' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. multi_bspline_create.cpp 460
    • V668 There is no sense in testing the 'm_currentStrategy' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ConnectionTool.cpp 333
    • ....

    Рефакторинг


    Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!nodeJuggler' and 'nodeJuggler'. kis_node_manager.cpp 809

    if (!nodeJuggler ||                           // <=
        (nodeJuggler &&                           // <=
         (nodeJuggler->isEnded() ||
          !nodeJuggler->canMergeAction(actionName)))) {
            ....
    }

    Такая проверка может быть упрощена:

    if (!nodeJuggler ||
        (nodeJuggler->isEnded() ||
         !nodeJuggler->canMergeAction(actionName))) {
            ....
    }

    Аналогичное предупреждение анализатора:

    • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!m_currentFilterConfigWidget' and 'm_currentFilterConfigWidget'. kis_filter_option.cpp 111

    Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: !iterator.atEnd() &&!iterator.atEnd() KoTextDebug.cpp 867

    void KoTextDebug::dumpFrame(const QTextFrame *frame, QTextStream &out)
    {
        ....
        
        QTextFrame::iterator iterator = frame->begin();
    
        for (; !iterator.atEnd() && !iterator.atEnd(); ++iterator) { // <=
            ....
        }
        
        ....
    }

    Стоит проверить условие цикла for на наличие ошибок. Если ошибок нет, стоит убрать дублирующую проверку.

    Аналогичное предупреждение анализатора:

    • V501 There are identical sub-expressions to the left and to the right of the '&&' operator: !iterator.atEnd() &&!iterator.atEnd() KoTextDebug.cpp 909

    Предупреждение PVS-Studio: V799 The 'cmd' variable is not used after memory has been allocated for it. Consider checking the use of this variable. kis_all_filter_test.cpp 154

    bool testFilter(KisFilterSP f)
    {
      ....
      KisTransaction * cmd = 
        new KisTransaction(kundo2_noi18n(f->name()), dev); // <=
    
      // Get the predefined configuration from a file
      KisFilterConfigurationSP  kfc = f->defaultConfiguration();
    
      QFile file(QString(FILES_DATA_DIR) +
                 QDir::separator() + f->id() + ".cfg");
      if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) {
        //dbgKrita << "creating new file for " << f->id();
        file.open(QIODevice::WriteOnly | QIODevice::Text);
        QTextStream out(&file);
        out.setCodec("UTF-8");
        out << kfc->toXML();
      } else {
        QString s;
        QTextStream in(&file);
        in.setCodec("UTF-8");
        s = in.readAll();
        //dbgKrita << "Read for " << f->id() << "\n" << s;
        kfc->fromXML(s);
      }
      dbgKrita << f->id();// << "\n" << kfc->toXML() << "\n";
    
      f->process(dev, QRect(QPoint(0,0), qimage.size()), kfc);
    
      QPoint errpoint;
    
      delete cmd; // <=
    
      ....
    }

    Здесь выделили и освободили память под объект cmd, но так ни разу его и не использовали.

    Предупреждение PVS-Studio: V732 Unary minus operator does not modify a bool type value. Consider using the '!' operator. kis_equalizer_slider.cpp 75

    QRect KisEqualizerSlider::Private::boundingRect() const
    {
        QRect bounds = q->rect().adjusted(0, 0, -isRightmost, -1);
        return bounds;
    }

    В данном примере переменная isRightmost имеет тип bool. Используя унарный минус, переменную неявно преобразовывают к типу int и передают получившееся число в метод adjusted(). Такой код усложняет понимание. Явное лучше, чем неявное, так что, думаю, стоит переписать этот фрагмент так:

    QRect KisEqualizerSlider::Private::boundingRect() const
    {
        QRect bounds = q->rect().adjusted(0, 0, isRightmost ? -1 : 0, -1);
        return bounds;
    }

    Аналогичные предупреждения анализатора:

    • V732 Unary minus operator does not modify a bool type value. Consider using the '!' operator. kis_equalizer_button.cpp 66
    • V732 Unary minus operator does not modify a bool type value. Consider using the '!' operator. kis_duplicateop.cpp 222

    Заключение


    В заключение мне хочется обратиться к разработчикам Krita и предложить им бесплатно возобновить использование нашего анализатора.

    С тех пор, как разработчики последний раз использовали PVS-Studio, у нас появились версии для Linux и MacOS (сам я проверял этот проект в Linux), а также значительно улучшился анализ.

    Кроме того, приглашаю всех желающих скачать и попробовать PVS-Studio на коде своего проекта.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Egor Bredikhin. Check of the Krita 4.0 Free Source Code Graphics Editor

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.

    PVS-Studio

    225,56

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

    Поделиться публикацией
    Комментарии 22
      +3
      Глупый вопрос, конечно: а вы в каком редакторе единорога рисуете? :)
        +2
        Обычно мы используем GIMP.
        +1
        Если присмотреться, то можно заметить, что внутри данного сложного условия находится проверка вида (!a || a):

        d->documentLayout->changeTracker()->displayChanges() ||
        !d->documentLayout->changeTracker()->displayChanges()

        Такое условие является всегда истинным, из-за чего вся эта большая проверка становится бессмысленной, о чем и сообщает анализатор.


        Нет же. Здесь происходит вызов displayChanges(), и если резутьтат равен false, то происходит вызов displayChanges() второй раз. Вообще не факт, что он также будет false. Это условие абсолютно не обязано быть истинным.

        Для примера, вот код:

        int cond() {
            if(foo() || !foo())
            {}
        }


        Вот результат компиляции:

        cond():
          sub rsp, 8
          call foo()
          test al, al
          jne .L2
          call foo()
        .L2:
          add rsp, 8
          ret


        Ни о каком всегда истинном условии не может быть и речи.
          +3
          Давайте взглянем на код вызываемых методов:

          bool KoChangeTracker::displayChanges() const
          {
              return d->displayChanges;
          }


          KoChangeTracker *KoTextDocumentLayout::changeTracker() const
          {
              return d->changeTracker;
          }
          

          Как мы видим, changeTracker() просто возвращает указатель из приватного поля d, а displayChanges() просто возвращает bool из приватного поля d.
          При этом, между вызовами эти значения не изменяются. Анализатор понимает это и выдает предупреждение.
          Т.е. в данном случае анализатор производит анализ тел вызываемых методов и собирает информацию о модифицируемых переменных, а вовсе не ограничивается простым паттерн-матчингом вида x || !x.

          Код условия целиком

            +5
            Очень классное поведение для анализатора. Но в статье про исходный код вызвваемых методов ничего сказано не было, поэтому в принципе комментарий выше был правильным. Но поведение анализатор выше всяких похвал.
              0

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

            +2
            Спасибо, что потестировали и отрепортили на багзиллу! Я посмотрю. При беглом просмотре часть багов явно реальные.

            Оправдание 1:
            compression.cpp 110 — это не бага, а защитный код, который, возможно, стоит переделать на ассерт. Код PSD сжатия был скопирован из неподдерживаемой более библиотеки, поэтому лишние меры предосторожности не повредят.

            False positive 2:
            kis_all_filter_test.cpp 154 — это классика жанра, которую уже даже в стандарте С++ обсуждают. Объект cmd реализует RAII для транзакции над полотном изображения, у него очень нетривиальные конструктор с деструктором. Было бы круто поменять варнинг на что-то вроде «зачем выпендриваетесь, создайте уж объект в стеке».

              0
              kis_all_filter_test.cpp 154 — это классика жанра

              Где о ней можно почитать? Как она называется?
                +1
                Про саму технику вот тут:
                en.wikipedia.org/wiki/Resource_acquisition_is_initialization

                Обсуждение про стандарт было вот тут (я не помню, чем кончилось в итоге; вроде не сошлись на универсальном заменителе):
                groups.google.com/a/isocpp.org/forum/#!topic/std-proposals/a4CRu2KONZ8

                PS:
                По хорошему, конечно, мы не должны были создавать эту переменную с куче, а просто сделать это в стеке. В этом смысле, хорошо, что анализатор нас предупреждает (такое выделение памяти без QScopedPointer опасно), но просто текст варнинга говорит совсем о другом :)
                  0
                  Я подумал, что это какая-то модификация RAII…
                    0
                    Ну в том-то и дело, что при RAII с динамическим выделением он выдает варнинг.
              0

              А у PVS-Studio есть автоматические рефакторинги? А то, как у вас в главе Рефакторинг если сделают оптимизацию макроса SANITY_ZEROS, как у вас написано, так еще один баг появится (! лишний справа от !=)

                0
                Нет. Рефакторинг — это другая задача.
                0
                Ой, а заменять, как написано в примере, вообще нельзя, ибо переменная m_filterWeights[i].weight[j] — это не bool! Анализатор правильно написал, что можно заменить, обернув все в bool, а автор статьи это не учел. Вообще, достаточно странное предупреждение. Зачем менять условие на менее понятное, в котором проще допустить ошибку?
                  +2
                  Да, здесь я сам накосячил, извините :)
                  Пример неудачный, заменил его другим.
                    0
                    Кстати, заметил, что PVS-Studio совсем не понимает define'ов, которые разворачиваются в какие-либо сложные конструкции со вложением. Там в логе не менее десятка false-positive репортов на конструкцию вроде такой:

                        LodDataStructImpl *dst = dynamic_cast<LodDataStructImpl*>(_dst);
                    
                        KIS_SAFE_ASSERT_RECOVER_RETURN(dst);
                        KIS_SAFE_ASSERT_RECOVER_RETURN(
                            dst->lodData->levelOfDetail() == defaultBounds->currentLevelOfDetail());
                          //↑ V522 There might be dereferencing of a potential null pointer 'dst'.
                    

                    Сами ассерты имеют такой вид:
                    #define KIS_SAFE_ASSERT_RECOVER(cond) \
                        if (!(cond) && (kis_safe_assert_recoverable(#cond,__FILE__,__LINE__), true))
                    #define KIS_SAFE_ASSERT_RECOVER_RETURN(cond) KIS_SAFE_ASSERT_RECOVER(cond) { return; }
                    
                      0
                      Насколько я знаю, PVS анализирует препроцессированный код(.i) который содержит у себя уже подставленный код вместо макросов. Это с одной стороны дает возможность отлавливать дополнительные баги в использовании макросов, что не раз приводилось в статьях, с другой стороны много false-positive.
                        0
                        Хм… ну тогда у меня только одно предположение: либо я, либо PVS-Studio не монимает результат выполнения оператора «запятая»:

                        (kis_safe_assert_recoverable(#cond,__FILE__,__LINE__), true)
                        


                        По моей логике, выражение под if'ом будет всегда эквивалентно: if (!(cond)), что гарантирует отсутствие разыменования в приведенном примере.
                  +3
                  Спасибо вам огромное, ребят. Каждый пост приносит ощущение, что кто-то заботится о том, чтобы мир стал немного лучше. А обзор криты это вообще чудесно — недавно переехал на неё полностью и счастлив, чудесная штука.
                    0
                        do {
                            state = inflate(&stream, Z_PARTIAL_FLUSH);
                            if(state == Z_STREAM_END)
                                break;
                            if(state != Z_OK)
                                break;
                        } while (stream.avail_out > 0);

                    Это, очевидно, приводит к аналогичному:


                            if(state == Z_STREAM_END || state != Z_OK)
                                break;

                    которое тоже избыточно.

                      0
                      Убрал лишнее, спасибо.
                      +2
                      Спасибо! Надеюсь, разработчики это прочтут. Каждый раз при обновлении Криты надеюсь, что вот теперь наконец перееду на нее совсем, но нет.

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

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