EgorBredikhin Apr 23 2018 at 15:39

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

17 min

15K

PVS-Studio corporate blogOpen source*C++*Graphic design*

+37

Comments 22

AngReload Apr 23 2018 at 16:53

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

EgorBredikhin Apr 23 2018 at 17:10

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

32bit_me Apr 23 2018 at 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

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

EgorBredikhin Apr 23 2018 at 19:35

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

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

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

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

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

Sultansoy Apr 23 2018 at 20:04

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

tagir_valeev Apr 26 2018 at 03:42

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

dkazakov-dev Apr 23 2018 at 18:17

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

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

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

zanac Apr 23 2018 at 22:41

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

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

dkazakov-dev Apr 23 2018 at 22:53

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

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

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

zanac Apr 24 2018 at 06:04

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

dkazakov-dev Apr 24 2018 at 12:52

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

Mingun Apr 23 2018 at 18:32

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

Andrey2008 Apr 23 2018 at 19:05

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

dkazakov-dev Apr 23 2018 at 18:48

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

EgorBredikhin Apr 23 2018 at 20:08

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

dkazakov-dev Apr 26 2018 at 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; }

maksqwe Apr 26 2018 at 13:00

Насколько я знаю, PVS анализирует препроцессированный код(.i) который содержит у себя уже подставленный код вместо макросов. Это с одной стороны дает возможность отлавливать дополнительные баги в использовании макросов, что не раз приводилось в статьях, с другой стороны много false-positive.

dkazakov-dev Apr 26 2018 at 13:11

Хм… ну тогда у меня только одно предположение: либо я, либо PVS-Studio не монимает результат выполнения оператора «запятая»:

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

По моей логике, выражение под if'ом будет всегда эквивалентно: if (!(cond)), что гарантирует отсутствие разыменования в приведенном примере.

jehy Apr 23 2018 at 18:50

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

viktprog Apr 24 2018 at 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;

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

EgorBredikhin Apr 24 2018 at 09:38

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

1eqinfinity Apr 24 2018 at 10:42

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