Comments 51
P.S. Основное предназначение Standalone — это мониторинг запуска C и C++ компиляторов для сбора информации о строках запуска. Далее собранная информация используется для проверки файлов. В общем, это позволяет быстро проверить любой проект, не встраивая анализатор в систему сборки: www.viva64.com/ru/m/0031 Плюс Standalone можно использовать для просмотра отчётов.
c1 = -1;Видимо не зря тут анализатор переживает…
… // Вот тут с с1 может и не случиться ничего, зависит от условий
freq[c1] += freq[c2];
Там есть такие начальные условия
freq[256] = 1;
v = 1000000000L;
поэтому внутрь if
поток управления должен попасть.
но по-моему очевидно, что любой статический анализатор не может полностью эмулировать поток управления, поэтому такие ошибки неизбежны.
freq[256] = 1
, c1
как минимум будет равен 256
, но конечно лучше реальные исходники посмотреть, непонятно что могли «упростить» в процессе обрезания кода для вставки в пост.v = 1
, вместо 1000000000L
. Правда вот теперь я начинаю сомневаться и подозревать себя в невнимательности — уж очень странно получается…Bobrovsky, что скажете: справедливы наши с paluke замечания?
P.S. а если предположить что элементы freq на самом деле не отрицательные, то
c1
почти всегда будет 256. А если элементы вообще только положительные, то совсем всегда… И в итоге — просто всегда, т.к. нулевые значения игнорируются (if (freq[i] != 0 && freq[i] <= v)
), т.е. первый цикл вообще бесполезен и можно сразу сделать c1=256
. Так могут элементы freq быть отрицательными или нет? Если судить из «названия», то это частоты, и они отрицательными быть не должны.P.P.S. Соответственно если значения не отрицательные, то и во втором цикле, при
v == 1
можно делать break
тем самым немного «оптимизировав код». Правда я больше питонист и не в курсе — может в C# break в каких-то странных случаях может ухудшать производительность, но думаю это маловероятно.for (;;)
, и поэтому freq[256] == 1
только первый раз, а второй раз это уже будет равно 1+freq[c2]
из прошлого цикла. Но тогда и понятно от чего жалуется PVS: чисто формально, если добиться freq[c2] == 1000000000L
, то на втором проходе внешнего бесконечного цикла уже возможна ситуация, когда нельзя будет попасть внутрь if, также подобная ситуация может возникнуть если в процессе работы цикла останутся только элементы равные 0, или больше 1000000000L. Так-что формально анализатор всё-таки прав. Плюс в примере в статье нужно добавить внешний бесконечный цикл и условия по которому он обрывается.И считаю, что всё же стоит подумать: как сделать чтобы при первом проходе c1 определялось без цикла.
Но это не мешает проинициализировать с1 значением 256. И эту инициализацию можно даже вынести из внешнего цикла, а сам цикл поиска значения с1 унести в самый конец этого самого внешнего «бесконечного» цикла. Вот тогда и получится определить с1 на первом проходе без цикла.
Не надо переносить поиск c1 в конец цикла! Это, может быть, и правда будет чуть быстрее и понятнее для роботов, но зато текущая версия понятнее для людей.
Всего-то нужно указать в комментариях характеристики цикла:
// invariant: sum of all items in freq array = const
// precondition: freq array contains as least one non zero item
// precondition: all items <= 256
// postcondition: freq array contains only one non zero item
После этого вопросов вида "может ли не найтись c1" возникать уже не должно...
Вопрос — были ли у вас дубли с предупреждениями решарпера?
Ложные срабатывания по V3081, V3134 обязательно посмотрим и скорее всего быстро поправим. По поводу же V3125, это известная проблема нашего C# анализатора сейчас — необходимо доработать механизмы dataflow и символьных вычислений, чтобы он смог понимать такие случаи. Здесь наш C# анализатор отстаёт от С/C++ анализатора, который это всё уже умеет. К сожалению, пока руки никак до этого не доходили, но надеемся, что до конца года (или в начале следующего) сможем и по этому направлению что-то сделать.
По поводу проверок возвращаемых значений методов, которые не могут вернуть null — нам уже отписывали подобные замечания\пожелания. Сейчас я склоняюсь к тому, чтобы во многом согласиться с вашими коллегами, которые агитировали за удаление этих избыточных проверок, тем более планируется расширить возможности анализатора диагностировать потенциальные null reference exception, и если контракт у таких методов когда-нибудь поменяется, статический анализатор также поможет вам обнаружить такие потенциальные исключения. Сейчас же я думаю, что мы просто понизим уровень подобных предупреждений, как некритичных.
freq[256] = 1;
// ....
c1 = 256;
// ....
freq[c1] += freq[c2];
freq[256]
на 1+freq[c2]
. Итого: из кода в статье «выкинули» важный кусок кода, и скорее всего анализатор всё-же правUpd: возможно я вас неправильно понял — я подумал что парвый цикл с поиском c1 вы совсем выкинули, но наверное подразумеваете, что он остаётся.
for (;;)
{
/* Find the smallest nonzero frequency, set c1 = its symbol */
/* In case of ties, take the larger symbol number */
//c1 = -1;
//v = 1000000000L;
c1=256;
for (i=0;i<=256;i++){
if(freq[i]>0){
c1=i;
v=freq[i];
break;
}
}
for (i = c1+1; i <= 256; i++)
{
if (freq[i] != 0 && freq[i] <= v)
{
v = freq[i];
c1 = i;
}
}
/* Find the next smallest nonzero frequency, set c2 = its symbol */
/* In case of ties, take the larger symbol number */
//c2 = -1;
//v = 1000000000L;
c2=256;
for (i=0;i<=256;i++){
if(freq[i]>0){
c2=i;
v=freq[i];
break;
}
}
for (i = c2; i <= 256; i++)
{
//if (freq[i] != 0 && freq[i] <= v && i != c1)
if (freq[i] != 0 && freq[i] <= v && i)
{
v = freq[i];
c2 = i;
}
}
/* Done if we've merged everything into one frequency */
//if (c2 < 0)
if (c1==c2)
break;
/* Else merge the two counts/trees */
freq[c1] += freq[c2];
freq[c2] = 0;
/* Increment the codesize of everything in c1's tree branch */
codesize[c1]++;
while (others[c1] >= 0)
{
c1 = others[c1];
codesize[c1]++;
}
others[c1] = c2; /* chain c2 onto c1's tree branch */
/* Increment the codesize of everything in c2's tree branch */
codesize[c2]++;
while (others[c2] >= 0)
{
c2 = others[c2];
codesize[c2]++;
}
}
Кажется, вы на пустом месте усложнили алгоритм.
А еще у вас всегда c1 будет равно c2, что является ошибкой. Там условие && i != c1
не просто так стояло...
freq[256] = 1;
c1 = 256;
for (;;)
{
c2 = -1;
v = 1000000000L;
for (i = 0; i <= 256; i++)
{
if (freq[i] != 0 && freq[i] <= v && i != c1)
{
v = freq[i];
c2 = i;
}
}
/* Done if we've merged everything into one frequency */
if (c2 < 0)
break;
/* Else merge the two counts/trees */
freq[c1] += freq[c2];
freq[c2] = 0;
/* Increment the codesize of everything in c1's tree branch */
codesize[c1]++;
while (others[c1] >= 0)
{
c1 = others[c1];
codesize[c1]++;
}
others[c1] = c2; /* chain c2 onto c1's tree branch */
/* Increment the codesize of everything in c2's tree branch */
codesize[c2]++;
while (others[c2] >= 0)
{
c2 = others[c2];
codesize[c2]++;
}
v = 1000000000L;
for (i = 0; i <= 256; i++)
{
if (freq[i] != 0 && freq[i] <= v)
{
v = freq[i];
c1 = i;
}
}
}
В итоге, согласно принципу YAGNI, решили не держаться за проверки и удалили их. Все предупреждения были перенесены из теоретических/формальных в обоснованные.
У нас обычно такое правило — вызовам методов внутри одного класса доверяем и параметры не перепроверяем, а вот все приходящее снаружи — проверяем обязательно. Что-то типа зон доверия.
Это касается только приватных методов?
Согласен, в этом случае это имеет смысл и анализатор должен убрать часть проверок.
С другой стороны, я в разных языках встречаю интересные защитные паттерны, такие как в приведённом примере из Java ниже. Причиной тому, что через reflection можно добраться почти куда угодно, и некоторые фреймворки этим пользуются. В CSharp тоже такое встречал, но не так часто, как в Java.
final class UtilityClass {
private UtilityClass(){ throw IllegalStateException(); }
public static doSomething(){}
}
www.jetbrains.com/help/resharper/Reference__Code_Annotation_Attributes.html
resharper-plugins.jetbrains.com/packages/ReSharper.ImplicitNullability
Спасибо за публикацию, у меня есть один комментарий по Вашему выводу про предупреждение о цикле. (V3081 „The 'X' counter is not used inside a nested loop. Consider inspecting usage of 'Y' counter“).
Мне с довольно большим опытом потребовалось потратить около 30 секунд, чтобы понять, что Ваш код действительно не будет бросать исключения. Сколько уйдёт времени для человека с меньшим опытом — не ясно. Поэтому я бы, всё же, переписал этот метод, положив сомнительный for
внутрь условия.
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;
}
Andrey2008, а этот комментарий уже к Вам.
Возвращаясь к той же функции, она содержит уязвимость buffer overflow, которая в C/C++ привела бы к не очень приятным последствиям: первая итерация идёт до table.Length
, который может быть больше, чем newSize. Конечно, CSharp выдаст исключение, но в данном случае это является недостатком конкретной версии статического анализатора, не выдавшего здесь ошибку.
Поправьте меня, если вы выдаёте это предупреждение безопасности в данном случае
помечать это, как возможную ошибку всё же стоит. В C/C++ подобные ошибки — это большая часть векторов атак на различные системы. В языках высокого уровня выход за границы массива пораждает исключение, что может привести к остановке в работе программы в лучшем случае, а в худшем — привести в неправильное состояние.
Это нормально, если программист этого хочет, а если не ожидает, то хватать окошки "У программы необработанное исключение, выйти или продолжить?" (если программист позволит не выйти из программы сразу) — не самая приятная времяпрепровождение. Особенно если программа очень важна для бизнеса, и пользователь не смог сохранить свою работу.
Благодарю за Ваш ответ и Вашу оценку к моему комментарию. Они мне добавляют возможности по поиску возможных уязвимостей. Для информации, проведите статистический анализ, сколько CVE связано с buffer & stack overflow.
eirnym, зато если подать newSize < table.Length, то всё уже не так хорошо.
class Program
{
private static void realloc(ref int[][] table, int newSize)
{
int[][] newTable = new int[newSize][]; // newTable.Length == 5
int existingSize = 0;
if (table != null)
{
existingSize = table.Length;
for (int i = 0; i < existingSize; i++)
newTable[i] = table[i]; // i == 5: newTable[5] = table[5] An unhandled exception of type 'System.IndexOutOfRangeException' occurred
}
for (int i = existingSize; i < newSize; i++)
newTable[i] = new int[4];
table = newTable;
}
static void Main(string[] args)
{
int[][] t = new int[10][];
for (int i = 0; i < t.Length; i++)
t[i] = new int[10];
realloc(ref t, 5);
}
}
Именно об этом и написал :) Благодарю за пример
К сожалению, PVS Studio не считает это возможной уязвимостью. Но это их право
Что можем, то ищем. Примеры. Более того, разработана и совершенствуется специализированная диагностика V1010 для поиска использования непроверенных данных. В том числе, она может выявлять и выход за границу массива. Подробнее про V1010.
Решарпер стоит примерно такую же сумму, только не в месяц, а в год, и ловит это все так же хорошо. При том, что статичский анализатор — не основная его функциональность.
Я не знаю другие возможности вашего анализатора, я сужу по статье. А в ней указаны лишь возможности статического анализа и делается вывод, что это весьма полезный продукт.
И по данному критерию при учете цены в 6 раз меньше, он проигрывает решарперу очень сильно, особенно учитывая, что он делает анализ прямо на ходу.
И да, как и у вашего продукта, у него тоже есть поддержка.
Я не знаю другие возможности вашего анализатора, я сужу по статье. А в ней указаны лишь возможности статического анализа и делается вывод, что это весьма полезный продукт.Вероятно, вы делаете не те выводы и не по тем статьям. Этот материал — профит для конкретного проекта и команды разработчиков. На основе обзора сделан вывод о потенциальной пользе продукта для аналогичных проектов. А познакомиться со всеми возможностями анализатора вы можете только на официальном сайте.
- текущая статья — ребята взяли анализатор и показали, какие ошибки им нашли;
- habr.com/company/pvs-studio/blog/334554 речь идет про методику расчета ошибок автором статьи, который чуть выше мне говорил, что статический анализатор — это не только поиск ошибок;
- habr.com/company/pvs-studio/blog/418891 — ошибки, найденные в андроиде;
- habr.com/company/pvs-studio/blog/147401 — отзывы о работе инструмента. Цитата автора: «Инструмент представляет собой статический анализатор кода, выявляющий ошибки и другие недостатки в исходных текстах программ.» Дальше идут отзывы и сравнение с другими инструментами, которые так же ищут ошибки.
- и так далее
из чего, я думаю, можно сделать вывод, что вы делаете таки упор на поиск ошибок, а не на дополнительные возможности?
Ну и вернемся таки к текущей статье, все таки про нее комментарии.
Вы совершенно прав, в ней указан профит для конкретного проекта и команды. И сделаны выводы о потенциальной пользе, да.
А я всего лишь дополнил, что конкретной такой же профит для конкретно этой же команды (а так же потенциальный для других) можно получить в режиме реального времени другим инструментом в 6 раз дешевле (4к в месяц против 8к за год), попутно получив еще ряд полезных инструментов для повседневной разработки.
С чем вы конкретно не согласны?
- PVS-Studio и SonarQube
- PVS-Studio и HTML формат отчётов
- PVS-Studio как SAST решение
- Запуска PVS-Studio в Docker
- и т.д.
К сожалению, непонятно как их написать много, чтобы не наскучить читателям. Вот и получается, что они теряются в статьях по проверки проектов.
Ничего страшного в этом нет. Если кто-то хочет узнать о возможностях PVS-Studio, то можно обратиться к подробной документации.
За флудом можно пропустить и важные вещи
False positives. Из-за ошибок в реализации конкретной проверки или каком-то недостатке анализатора. Таких было выдано 14 штук, что составляет около 7% от всех предупреждений.
Реальных ошибок было найдено 16 штук, что составляет около 8% от всех предупреждений. Но при этом еще было выдано 103 штуки обоснованных предупреждений, что составляет около 54% от всех предупреждений.
То есть полезного было 64%, а очень полезного — 8%. Весьма прилично, на мой взгляд.
Docotic.Pdf: Какие проблемы PVS-Studio обнаружит в зрелом проекте?