Не так давно состоялся релиз новой версии свободного графического редактора 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. Пожалуйста, ознакомьтесь со списком.

Комментарии (20)


  1. AngReload
    23.04.2018 16:53
    +1

    Глупый вопрос, конечно: а вы в каком редакторе единорога рисуете? :)


    1. EgorBredikhin
      23.04.2018 17:10

      Обычно мы используем GIMP.


  1. 32bit_me
    23.04.2018 17:35

    Если присмотреться, то можно заметить, что внутри данного сложного условия находится проверка вида (!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


    Ни о каком всегда истинном условии не может быть и речи.


    1. EgorBredikhin
      23.04.2018 19:35
      +1

      Давайте взглянем на код вызываемых методов:

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


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

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

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


      1. Sultansoy
        23.04.2018 20:04
        +1

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


      1. lany
        26.04.2018 03:42

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


  1. dkazakov-dev
    23.04.2018 18:17

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

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

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


    1. zanac
      23.04.2018 22:41

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

      Где о ней можно почитать? Как она называется?


      1. dkazakov-dev
        23.04.2018 22:53

        Про саму технику вот тут:
        en.wikipedia.org/wiki/Resource_acquisition_is_initialization

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

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


        1. zanac
          24.04.2018 06:04

          Я подумал, что это какая-то модификация RAII…


          1. dkazakov-dev
            24.04.2018 12:52

            Ну в том-то и дело, что при RAII с динамическим выделением он выдает варнинг.


  1. Mingun
    23.04.2018 18:32

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


    1. Andrey2008
      23.04.2018 19:05

      Нет. Рефакторинг — это другая задача.


  1. dkazakov-dev
    23.04.2018 18:48

    Ой, а заменять, как написано в примере, вообще нельзя, ибо переменная m_filterWeights[i].weight[j] — это не bool! Анализатор правильно написал, что можно заменить, обернув все в bool, а автор статьи это не учел. Вообще, достаточно странное предупреждение. Зачем менять условие на менее понятное, в котором проще допустить ошибку?


    1. EgorBredikhin
      23.04.2018 20:08
      +1

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


      1. dkazakov-dev
        26.04.2018 00:51

        Кстати, заметил, что 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; }
        


  1. jehy
    23.04.2018 18:50

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


  1. viktprog
    24.04.2018 06:00

        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;

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


    1. EgorBredikhin
      24.04.2018 09:38

      Убрал лишнее, спасибо.


  1. 1eqinfinity
    24.04.2018 10:42
    +1

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