Pull to refresh

Comments 51

Спасибо за публикацию. Благодарим за критические замечания в вводной части статьи. Очень полезно получить взгляд на наш инструмент со стороны. Это будет нам поводом подумать, как сделать интерфейс лучше и понятнее. Также попробуем разобраться, что случилось со шрифтами.

P.S. Основное предназначение Standalone — это мониторинг запуска C и C++ компиляторов для сбора информации о строках запуска. Далее собранная информация используется для проверки файлов. В общем, это позволяет быстро проверить любой проект, не встраивая анализатор в систему сборки: www.viva64.com/ru/m/0031 Плюс Standalone можно использовать для просмотра отчётов.
> P.S. Основное предназначение Standalone — это мониторинг запуска C и C++ компиляторов для сбора информации о строках запуска.

Возможно, если вы напишете об этом в интерфейсе программы (например, вместо не очень нужного там блога), меньше пользователей будет сбито с толку.
c1 = -1;
… // Вот тут с с1 может и не случиться ничего, зависит от условий
freq[c1] += freq[c2];
Видимо не зря тут анализатор переживает…

Там есть такие начальные условия


freq[256] = 1;
v = 1000000000L;

поэтому внутрь if поток управления должен попасть.


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

вроде бы из-за того, что freq[256] = 1, c1 как минимум будет равен 256, но конечно лучше реальные исходники посмотреть, непонятно что могли «упростить» в процессе обрезания кода для вставки в пост.
А почему бы тогда сразу не проинициализировать с1 значением 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 определялось без цикла.
На самом деле, вместо 1000000000L там должно быть что-то типа Int64.MaxValue, не должно быть в массиве элементов больше, чем это значение.
Но это не мешает проинициализировать с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" возникать уже не должно...

Ваши замечания справедливы для случая произвольных значений в массиве freq. На практике, алгоритм работает с массивом не-отрицательных значений. Каждый элемент массива не больше 256.
V3083 (возможный NRE при вызове евента) — попробуйте всегда использовать myevent?.Invoke(...) — о подобных проблемах не придётся думать.
Вопрос — были ли у вас дубли с предупреждениями решарпера?
Спасибо за совет. Дублей не было, потому что Решарпером мы не пользуемся.
или можно всегда ивенты объявлять так:
public event EventHandler myevent = delegate { };

и вызывать вообще без проверки на null.
Спасибо за статью!

Ложные срабатывания по 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, решили не держаться за проверки и удалили их. Все предупреждения были перенесены из теоретических/формальных в обоснованные.

У нас обычно такое правило — вызовам методов внутри одного класса доверяем и параметры не перепроверяем, а вот все приходящее снаружи — проверяем обязательно. Что-то типа зон доверия.

Это касается только приватных методов?

Да. Насколько я помню, это тема из защитного программирования (defensive programming)

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


С другой стороны, я в разных языках встречаю интересные защитные паттерны, такие как в приведённом примере из Java ниже. Причиной тому, что через reflection можно добраться почти куда угодно, и некоторые фреймворки этим пользуются. В CSharp тоже такое встречал, но не так часто, как в Java.


final class UtilityClass {
     private UtilityClass(){ throw IllegalStateException(); }
     public static doSomething(){}
}

Спасибо за публикацию, у меня есть один комментарий по Вашему выводу про предупреждение о цикле. (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.

И сейчас вы говорите о противоположном, чем было сказано Вами ранее: "Такой дефект мы не ищем". Вы ищете, но обрабатываете не все случаи, и это хорошо.

Я помню что вы уже отвечали по поводу анализатора для php. Хотя все равно напомню что очень бы хотелось.

Решарпер стоит примерно такую же сумму, только не в месяц, а в год, и ловит это все так же хорошо. При том, что статичский анализатор — не основная его функциональность.

Статический анализатор кода, это далеко не только сообщения, но и инфраструктура. Например, это интеграция с SonarQube и т.д. Плюс поддержка. Это стоит денег.

Я не знаю другие возможности вашего анализатора, я сужу по статье. А в ней указаны лишь возможности статического анализа и делается вывод, что это весьма полезный продукт.
И по данному критерию при учете цены в 6 раз меньше, он проигрывает решарперу очень сильно, особенно учитывая, что он делает анализ прямо на ходу.
И да, как и у вашего продукта, у него тоже есть поддержка.

Я не знаю другие возможности вашего анализатора, я сужу по статье. А в ней указаны лишь возможности статического анализа и делается вывод, что это весьма полезный продукт.
Вероятно, вы делаете не те выводы и не по тем статьям. Этот материал — профит для конкретного проекта и команды разработчиков. На основе обзора сделан вывод о потенциальной пользе продукта для аналогичных проектов. А познакомиться со всеми возможностями анализатора вы можете только на официальном сайте.

Можно какую-нибудь конкретику, пожалуйста? Кроме «не те выводы и по не тем статьям»? Вот давайте откроем статьи на хабре через поиск по 'pvs':
  • текущая статья — ребята взяли анализатор и показали, какие ошибки им нашли;
  • 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, временами публикуются. Примеры:

К сожалению, непонятно как их написать много, чтобы не наскучить читателям. Вот и получается, что они теряются в статьях по проверки проектов.

Ничего страшного в этом нет. Если кто-то хочет узнать о возможностях PVS-Studio, то можно обратиться к подробной документации.
Кстати, всех интересующихся PVS-Studio, приглашаю подписаться на блог компании PVS-Studio. Дело в том, что некоторые статьи про новшества в анализаторе мы публикуем только там. Добавлять их в другие хабы на наш взгляд неуместно. Вот сегодня, например, я опубликовал там заметку "Релиз PVS-Studio 6.26", из которой можно узнать, что м поддержали Waf, GNU Arm Embedded Toolchain, Arm Embedded GCC compiler и т.д.
А какой общий поциент (%), т.е отношение реальных ошибок к общей сумме выданных замечаний?

За флудом можно пропустить и важные вещи
Как я понимаю, по мнению автора статьи их 7%.
False positives. Из-за ошибок в реализации конкретной проверки или каком-то недостатке анализатора. Таких было выдано 14 штук, что составляет около 7% от всех предупреждений.
Не хотел отвечать, но теперь вынужден исправить.

Реальных ошибок было найдено 16 штук, что составляет около 8% от всех предупреждений. Но при этом еще было выдано 103 штуки обоснованных предупреждений, что составляет около 54% от всех предупреждений.

То есть полезного было 64%, а очень полезного — 8%. Весьма прилично, на мой взгляд.
Sign up to leave a comment.

Articles