Хабрахабр

Docotic.Pdf: Какие проблемы PVS-Studio обнаружит в зрелом проекте?

Docotic.Pdf и PVS-studio

И о PVS-Studio мы наслышаны. Качество для нас важно. Pdf и узнать, что еще можно улучшить. Все это привело к желанию проверить Docotic.

Pdf — библиотека общего назначения для работы с PDF файлами. Docotic. NET runtime. Написана на C#, нет unsafe кода, нет внешних зависимостей кроме . NET 4+, так и под . Работает как под . NET Standard 2+.

Для статического анализа мы постоянно используем Code Analysis и StyleCop. Библиотека в разработке чуть больше 10 лет и в ней 110 тысяч строк кода без учета тестов, примеров и прочего. Наши клиенты из разных стран и разных индустрий доверяют качеству библиотеки. Несколько тысяч автоматических тестов охраняют нас от регрессий.

Какие проблемы обнаружит PVS-Studio?

Установка и первое впечатление

Скачал пробную версию с сайта PVS-Studio. Приятно удивил небольшой размер установщика. Установил с настройками по умолчанию: analysis engines, отдельная среда PVS-Studio, интеграция в Visual Studio 2017.

На мгновение задумался, что же нужно запустить. После установки ничего не запустилось, а в меню Пуск добавились два ярлыка с одинаковыми иконками: Standalone и PVS-Studio. Масштаб 200%, выставленный для моей Windows, поддерживается криво. Запустил Standalone и был неприятно удивлен интерфейсом. Название, Единорог и список Actions обрезаны при любом размере окна. Часть текста слишком мелкая, часть текста не помещается в отведенное для него место. Даже при полноэкранном.

Внезапно, в меню Файл не нашел такой возможности. Ну да ладно, решил открыть файл своего проекта. Спасибо, подумал я, попробую-ка я лучше другой вариант. Там мне предложили только открывать отдельные файлы. Масштаб 200% опять дал о себе знать. Запустил PVS-Studio — мне показали окно с размытым текстом. Хорошо, открыл Студию. Текст сообщал: ищите меня в «Трех Коронах» ищите меню PVS-Studio в Visual Studio.

Действительно, есть меню PVS-Studio, а в нем есть возможность проверить «Current Project». Открыл solution. Снизу в Студии всплыло окно с результатами анализа. Сделал нужный мне проект текущим и запустил проверку. Сначала возникло ощущение, что проверка не началась или сразу кончилась. В фоне появилось еще и окно с прогрессом проверки, но я его не сразу обнаружил.

Результат первой проверки

Анализатор проверил все 1253 файла проекта за примерно 9 минут и 30 секунд. К концу проверки счетчик файлов менялся не так быстро, как в начале. Возможно, есть некоторая нелинейная зависимость длительности проверки от числа проверяемых файлов.

Если посчитать частоту, то получается 0. В окне результатов появилась информация о 81 High, 109 Medium и 175 Low предупреждениях. 09 Medium предупреждений / файл, и 0. 06 High предупреждений / файл, 0. Или
0. 14 Low предупреждений / на файл. 99 Medium предупреждений на тысячу строк кода, и 1. 74 High предупреждения на тысячу строк кода, 0. 59 Low предупреждений на тысячу строк кода.

NET при его 256 тысячах строк кода анализатор нашел 15 High, 151 Medium и 32 Low предупреждений. Вот в этой статье указано, что в CruiseControl.

Pdf было выдано больше предупреждений в каждой из групп. Получается, что в процентном соотношении для Docotic.

Что же найдено?

Предупреждения Low я решил оставить без внимания на данном этапе.

На мой взгляд они примерно об одном. Отсортировал предупреждения по колонке Code и получилось, что абсолютным рекордсменом по частоте стали V3022 «Expression is always true/false» и V3063 «A part of conditional expression is always true/false if it is evaluated». Относительная частота = 48%. В сумме эти два предупреждения дают 92 случая из 190.

Я ожидал V3072 «The 'A' class containing IDisposable members does not itself implement IDisposable» и V3073 «Not all IDisposable members are properly disposed. Логика деления на High и Medium не совсем ясна. Но это вкусовщина, конечно. Call 'Dispose' when disposing 'A' class» в группе High, например.

Check lines: N1, N2» помечена два раза как High и один раз как Medium. Удивило, что V3095 «The object was used before it was verified against null. Баг?

Доверяй, но проверяй

Настало время проверить, насколько обоснованы полученные предупреждения. Найдены ли какие-нибудь реальные ошибки? Есть ли некорректные предупреждения?

Я разделил найденные предупреждения на группы, приведенные ниже.

Важные предупреждения

Их исправление повысило стабильность работы, решило проблемы с утечками памяти и т.п. Реальные ошибки/несовершенства.

Таких было выдано 16 штук, что составляет около 8% от всех предупреждений.

Приведу некоторые примеры.

Check variables 'color', 'indexed'» V3019 «Possibly an incorrect variable is compared to null after type conversion using 'as' keyword.

public override bool IsCompatible(ColorImpl color)
{ IndexedColorImpl indexed = color as IndexedColorImpl; if (color == null) return false; return indexed.ColorSpace.Equals(this);
}

Как можно видеть, вместо indexed, с null сравнивается переменная color. Это неправильно и может привести к NRE.

Consider inspecting 'cstr_index.tile_index'» V3080 «Possible null dereference.

Небольшой фрагмент для иллюстрации:

if (cstr_index.tile_index == null)

}

Очевидно, что в первом условии подразумевалось != null. В текущем виде код при каждом вызове кинет NRE.

Consider assigning event to a local variable before invoking it.» V3083 «Unsafe invocation of event 'OnProgress', NullReferenceException is possible.

public void Updated()
{ if (OnProgress != null) OnProgress(this, new EventArgs());
}

Предупреждение помогло исправить потенциальное исключение. Почему оно может возникнуть? На Stackoverflow есть хорошее объяснение.

The '0' index is pointing beyond 'v' bound»
V3106 «Possibly index is out of bounds.

var result = new List<FontStringPair>();
for (int i = 0; i < text.Length; ++i)
{ var v = new List<FontStringPair>(); createPairs(text[i].ToString(CultureInfo.InvariantCulture)); result.Add(v[0]);
}

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

V3117 «Constructor parameter 'validateType' is not used

Предупреждение было выдано для кода, похожего на такой

public SomeClass(IDocument document, bool validateType = true) : base(document, true)
{ m_provider = document;
}

Само предупреждение не кажется важным. Но проблема серьезнее, чем кажется на первый взгляд. В процессе добавления optional параметра validateType вызов конструктора базового класса забыли исправить.

Perhaps, this is a typo and 'range' variable should be used instead of 'domain'“
V3127 „Two similar code fragments were found.

private void fillTransferFunction(PdfStreamImpl function)
{ // .. PdfArrayImpl domain = new PdfArrayImpl(); domain.AddReal(0); domain.AddReal(1); function.Add(Field.Domain, domain); PdfArrayImpl range = new PdfArrayImpl(); range.AddReal(0); range.AddReal(1); function.Add(Field.Range, domain); // ....
}

Возможно, предупреждение не будет выдано, если части кода будут чуть сильнее отличаться. Но в данном случае ошибка при использовании copy-paste была обнаружена.

Теоретические/формальные предупреждения

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

Я приведу примеры для случаев, заслуживающих внимания. Таких было выдано 57 штук, что составляет около 30% от всех предупреждений.

V3013 „It is odd that the body of 'BeginText' function is fully equivalent to the body of 'EndText' function (166, line 171)“

public override void BeginText()
{ m_state.ResetTextParameters();
} public override void EndText()
{ m_state.ResetTextParameters();
}

У обеих функций тела на самом деле одинаковые. Но это правильно. И так ли уж странно, если тела функций из одной строки совпадают?

The value of 'c1' index could reach -1“
V3106 „Possible negative index value.

freq[256] = 1;
// ....
c1 = -1;
v = 1000000000L;
for (i = 0; i <= 256; i++)
{ if (freq[i] != 0 && freq[i] <= v) { v = freq[i]; c1 = i; }
} // ....
freq[c1] += freq[c2];

Согласен, кусочек не самого понятного алгоритма я привел. Но, по-моему, в данном случае анализатор зря переживает.

V3107 „Identical expression 'neighsum' to the left and to the right of compound assignment“

Предупреждение вызвано вполне банальным кодом:

neighsum += neighsum;

Да, его можно переписать через умножение. Но ошибки тут нет.

The expression is incorrect or it can be simplified.“
V3109 „The 'l_cblk.data_current_size' sub-expression is present on both sides of the operator.

/* Check possible overflow on size */
if ((l_cblk.data_current_size + l_seg.newlen) < l_cblk.data_current_size)
{ // ...
}

Комментарий в коде поясняет замысел. Снова ложная тревога.

Обоснованные предупреждения

Их исправление положительно повлияло на читабельность кода. То есть сократило ненужные условия, проверки и т.п. Влияние на то, как код работает — неочевидно.

Таких было выдано 103 штуки, что составляет около 54% от всех предупреждений.

Perhaps this is a mistake“
V3008 „The 'l_mct_deco_data' variable is assigned values twice successively.

if (m_nb_mct_records == m_nb_max_mct_records)
{ ResizeMctRecords(); l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;
} l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;

Анализатор прав: присваивание внутри if не нужно.

V3009 „It is odd that this method always returns one and the same value“

private static bool opj_dwt_decode_tile(opj_tcd_tilecomp_t tilec, OPJ_UINT32 numres)
{ if (numres == 1U) return true; // ... return true;
}

По совету анализатора метод был изменен и больше ничего не возвращает.

V3022 „Expression '!add' is always true“

private void addToFields(PdfDictionaryImpl controlDict, bool add)
{ // ... if (add) { // .. return; } if (!add) { // ... } // ...
}

Действительно, нет смысла во втором if. Условие всегда будет истинным.

V3029 „The conditional expression of the 'if' statements situated alongside each other are identical“

if (stroke) extGState.OpacityStroke = opacity; if (stroke) state.AddReal(Field.CA, opacity);

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

The '||' operator is surrounded by opposite expressions“ V3031 „An excessive check can be simplified.

Вот это кошмарное условие:

if (!(cp.m_enc.m_tp_on != 0 && ((!opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) && (t2_mode == J2K_T2_MODE.FINAL_PASS)) || opj_codec_t.OPJ_IS_CINEMA(cp.rsiz))))
{ // ...
}

После изменений стало гораздо лучше

if (!(cp.m_enc.m_tp_on != 0 && (opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) || t2_mode == J2K_T2_MODE.FINAL_PASS)))
{ // ...
}

V3063 „A part of conditional expression is always true if it is evaluated: x != null“
V3022 „Expression 'x != null' is always true“

Правильно ли так поступать — вопрос неоднозначный. Сюда я отнес предупреждения о том, что проверка на null не имеет смысла. Ниже я подробнее описал суть вопроса.

Необоснованные предупреждения

False positives. Из-за ошибок в реализации конкретной проверки или каком-то недостатке анализатора.

Таких было выдано 14 штук, что составляет около 7% от всех предупреждений.

Consider inspecting usage of 'j' counter“ V3081 „The 'i' counter is not used inside a nested loop.

Немного упрощенный вариант кода, для которого было выдано это предупреждение:

for (uint i = 0; i < initialGlyphsCount - 1; ++i)
{ for (int j = 0; j < initialGlyphsCount - i - 1; ++j) { // ... }
}

Очевидно, что i используется во вложенном цикле.

V3125 „The object was used after it was verified against null“

Код, для которого выдается такое предупреждение:

private static int Compare_SecondGreater(cmapEncodingRecord er1, cmapEncodingRecord er2)
{ if (er1 == er2) return 0; if (er1 != null && er2 == null) return 1; if (er1 == null && er2 != null) return -1; return er1.CompareTo(er2);
}

er1 не может быть равен null в момент вызова CompareTo().

Другой код, для которого выдается такое предупреждение:

private static void realloc(ref int[][] table, int newSize)
{ int[][] newTable = new int[newSize][]; int existingSize = 0; if (table != null) existingSize = table.Length; for (int i = 0; i < existingSize; i++) newTable[i] = table[i]; for (int i = existingSize; i < newSize; i++) newTable[i] = new int[4]; table = newTable;
}

table не может быть равно null в цикле.

255] bits is greater than the size of 'UInt32' type of expression '(uint)1'“ V3134 „Shift by [32..

Кусочек кода, для которого выдается такое предупреждение:

byte bitPos = (byte)(numBits & 0x1F);
uint mask = (uint)1 << bitPos;

Видно, что bitPos может иметь значение из диапазона [0..31], но анализатор считает, что она может иметь значение из диапазона [0..255], что неверно.

Другие подобные случаи приводить не буду, так как они эквивалентны.

Дополнительные мысли по поводу некоторых проверок

Мне показалось нежелательным предупреждать, что 'x != null' is always true в случаях, когда x — это результат вызова некоторого метода. Пример:

private X GetX(int y)
{ if (y > 0) return new X(...); if (y == 0) return new X(...); throw new ArgumentOutOfRangeException(nameof(x));
} private Method()
{ // ... X x = GetX(..); if (x != null) { // ... }
}

Да, формально анализатор прав: x всегда будет не равно null, потому что GetX либо вернет полноценный instance, либо кинет исключение. Но улучшит ли код удаление проверки на null? Что если GetX изменится позже? Обязан ли Method знать реализацию GetX?

Высказывалось мнение, что у текущего метода есть контракт, по которому он не должен возвращать null. Внутри команды мнения разделились. А если контракт меняется — надо обновлять и вызывающий код. И нет смысла писать избыточный код „на будущее“ при каждом вызове.

В поддержку такого мнения приводилось такое суждение: проверять на null — это как оборачивать каждый вызов в try/catch на случай если метод в будущем начнет бросать исключения.

Все предупреждения были перенесены из теоретических/формальных в обоснованные. В итоге, согласно принципу YAGNI, решили не держаться за проверки и удалили их.

Буду рад прочитать ваше мнение в комментариях.

Выводы

Статический анализ — полезная штука. PVS-Studio позволяет найти реальные ошибки.

Но все-таки PVS-Studio нашел реальные ошибки в проекте, где уже используется Code Analysis. Да, есть и необоснованные / некорректные предупреждения. Наш продукт достаточно хорошо покрыт тестами, его так или иначе тестируют наши клиенты, но robots do it better статический анализ все-таки приносит пользу.

Напоследок немного статистики.

Топ-3 необоснованных предупреждений

V3081 „The 'X' counter is not used inside a nested loop. Consider inspecting usage of 'Y' counter“
1 из 1 признано необоснованным

Check lines: N1, N2“
9 из 10 признаны необоснованными V3125 „The object was used after it was verified against null.

V3134 „Shift by N bits is greater than the size of type“
4 из 5 признаны необоснованными

Топ-3 важных предупреждений

V3083 „Unsafe invocation of event, NullReferenceException is possible. Consider assigning event to a local variable before invoking it“
5 из 5 признаны важными

V3020 „An unconditional 'break/continue/return/goto' within a loop“
V3080 „Possible null dereference“
2 из 2 признаны важными

Perhaps, this is a typo and 'X' variable should be used instead of 'Y'“
1 из 1 признано важным V3019 „It is possible that an incorrect variable is compared with null after type conversion using 'as' keyword“
V3127 „Two similar code fragments were found.

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть