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

    Docotic.Pdf и PVS-studio

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

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

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

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

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


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

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



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

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

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


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

    В окне результатов появилась информация о 81 High, 109 Medium и 175 Low предупреждениях. Если посчитать частоту, то получается 0.06 High предупреждений / файл, 0.09 Medium предупреждений / файл, и 0.14 Low предупреждений / на файл. Или
    0.74 High предупреждения на тысячу строк кода, 0.99 Medium предупреждений на тысячу строк кода, и 1.59 Low предупреждений на тысячу строк кода.

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

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

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


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

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

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

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



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


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

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

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


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

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

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

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

    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.

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

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

    if (cstr_index.tile_index == null)
    {
        if (cstr_index.tile_index[0].tp_index == null)
        {
            // ..
        }
    }
    

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

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

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

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

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

    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 вызов конструктора базового класса забыли исправить.

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

    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();
    }
    

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

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

    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;
    

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

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

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

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

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


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

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

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

    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);
    

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

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

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

    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% от всех предупреждений.

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

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

    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 в цикле.

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

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

    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 признано необоснованным

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

    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 признаны важными

    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. Perhaps, this is a typo and 'X' variable should be used instead of 'Y'“
    1 из 1 признано важным
    Share post

    Similar posts

    Comments 51

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

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

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

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


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

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


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

            +2
            вроде бы из-за того, что freq[256] = 1, c1 как минимум будет равен 256, но конечно лучше реальные исходники посмотреть, непонятно что могли «упростить» в процессе обрезания кода для вставки в пост.
              0
              Реальные исходники этой части кода посмотреть можно.
                +3
                А почему бы тогда сразу не проинициализировать с1 значением 256?
                  0
                  А вот это, как по мне, хорошее замечание. И тогда можно еще перед циклом назначать 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 в каких-то странных случаях может ухудшать производительность, но думаю это маловероятно.
                    0
                    И всё-таки я был невнимателен: там еще есть внешний цикл for (;;), и поэтому freq[256] == 1 только первый раз, а второй раз это уже будет равно 1+freq[c2] из прошлого цикла. Но тогда и понятно от чего жалуется PVS: чисто формально, если добиться freq[c2] == 1000000000L, то на втором проходе внешнего бесконечного цикла уже возможна ситуация, когда нельзя будет попасть внутрь if, также подобная ситуация может возникнуть если в процессе работы цикла останутся только элементы равные 0, или больше 1000000000L. Так-что формально анализатор всё-таки прав. Плюс в примере в статье нужно добавить внешний бесконечный цикл и условия по которому он обрывается.

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

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

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

                    Ложные срабатывания по V3081, V3134 обязательно посмотрим и скорее всего быстро поправим. По поводу же V3125, это известная проблема нашего C# анализатора сейчас — необходимо доработать механизмы dataflow и символьных вычислений, чтобы он смог понимать такие случаи. Здесь наш C# анализатор отстаёт от С/C++ анализатора, который это всё уже умеет. К сожалению, пока руки никак до этого не доходили, но надеемся, что до конца года (или в начале следующего) сможем и по этому направлению что-то сделать.

                    По поводу проверок возвращаемых значений методов, которые не могут вернуть null — нам уже отписывали подобные замечания\пожелания. Сейчас я склоняюсь к тому, чтобы во многом согласиться с вашими коллегами, которые агитировали за удаление этих избыточных проверок, тем более планируется расширить возможности анализатора диагностировать потенциальные null reference exception, и если контракт у таких методов когда-нибудь поменяется, статический анализатор также поможет вам обнаружить такие потенциальные исключения. Сейчас же я думаю, что мы просто понизим уровень подобных предупреждений, как некритичных.
                      0
                      Почему бы тогда не написать
                      freq[256] = 1;
                      // ....
                      c1 = 256;
                      // ....
                      freq[c1] += freq[c2];
                        0
                        Потому-что там еще есть внешний цикл, который в первой итерации поменяет freq[256] на 1+freq[c2]. Итого: из кода в статье «выкинули» важный кусок кода, и скорее всего анализатор всё-же прав

                        Upd: возможно я вас неправильно понял — я подумал что парвый цикл с поиском c1 вы совсем выкинули, но наверное подразумеваете, что он остаётся.
                          –1
                          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]++;
                              }
                          }
                            0

                            Кажется, вы на пустом месте усложнили алгоритм.


                            А еще у вас всегда c1 будет равно c2, что является ошибкой. Там условие && i != c1 не просто так стояло...

                              –1
                              Вот так:
                              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;
                                      }
                                  }
                              }
                                0
                                Неужели этот код считается понятнее исходного?
                                  0
                                  Не, в нем на один проход по массиву меньше.
                                    0
                                    Экономия на спичках. Вот если бы переписать его через две очереди — это и правда было бы быстрее.
                            0
                            В итоге, согласно принципу YAGNI, решили не держаться за проверки и удалили их. Все предупреждения были перенесены из теоретических/формальных в обоснованные.

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

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

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

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


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


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

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

                                  Andrey2008, а этот комментарий уже к Вам.


                                  Возвращаясь к той же функции, она содержит уязвимость buffer overflow, которая в C/C++ привела бы к не очень приятным последствиям: первая итерация идёт до table.Length, который может быть больше, чем newSize. Конечно, CSharp выдаст исключение, но в данном случае это является недостатком конкретной версии статического анализатора, не выдавшего здесь ошибку.


                                  Поправьте меня, если вы выдаёте это предупреждение безопасности в данном случае

                                    +2
                                    Такой дефект мы не ищем. И не уверен, что можно его искать с приемлемым уровнем ложных срабатываний. Плохая идея искать, что что-то не проверяется. Более подробно про нашу позицию на эту тему.
                                      –3

                                      помечать это, как возможную ошибку всё же стоит. В C/C++ подобные ошибки — это большая часть векторов атак на различные системы. В языках высокого уровня выход за границы массива пораждает исключение, что может привести к остановке в работе программы в лучшем случае, а в худшем — привести в неправильное состояние.


                                      Это нормально, если программист этого хочет, а если не ожидает, то хватать окошки "У программы необработанное исключение, выйти или продолжить?" (если программист позволит не выйти из программы сразу) — не самая приятная времяпрепровождение. Особенно если программа очень важна для бизнеса, и пользователь не смог сохранить свою работу.

                                        –4

                                        Благодарю за Ваш ответ и Вашу оценку к моему комментарию. Они мне добавляют возможности по поиску возможных уязвимостей. Для информации, проведите статистический анализ, сколько CVE связано с buffer & stack overflow.

                                      0

                                      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);
                                          }
                                      }
                                        0

                                        Именно об этом и написал :) Благодарю за пример

                                          –2

                                          К сожалению, PVS Studio не считает это возможной уязвимостью. Но это их право

                                            +3
                                            Дело не в том, считаем мы или не считаем это уязвимостью. Дело в том, можно ли выявлять такие ошибки, сохраняя низкий уровень ложных срабатываний. Большая ошибка считать, что чем сообщений больше, тем лучше.

                                            Что можем, то ищем. Примеры. Более того, разработана и совершенствуется специализированная диагностика V1010 для поиска использования непроверенных данных. В том числе, она может выявлять и выход за границу массива. Подробнее про V1010.
                                              0

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

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

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

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

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

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

                                                    0
                                                    Можно какую-нибудь конкретику, пожалуйста? Кроме «не те выводы и по не тем статьям»? Вот давайте откроем статьи на хабре через поиск по '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к за год), попутно получив еще ряд полезных инструментов для повседневной разработки.
                                                    С чем вы конкретно не согласны?
                                                      +1
                                                      Статьи, демонстрирующей разнообразные аспекты PVS-Studio, временами публикуются. Примеры:

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

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

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

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

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

                                              Only users with full accounts can post comments. Log in, please.