В очередной раз анализатор PVS-Studio оказался внимательнее человека

    Возьми баг

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

    В предыдущий раз я написал аналогичную заметку, изучая исходный код проекта StarEngine: 2D Game Engine. Теперь анализатор показал своё превосходство надо мной в ходе проверки фреймворка Qt.

    Последний раз мы проверяли фреймворк Qt в 2014 году. Прошло много времени, проект изменился, а в анализаторе PVS-Studio появилось много новых диагностик. Значит, вполне можно написать очередную статью, чем я и занялся.

    Выписывая интересные примеры ошибок, я повстречал вот такой код:

    QWindowsCursor::CursorState QWindowsCursor::cursorState()
    {
      enum { cursorShowing = 0x1, cursorSuppressed = 0x2 };
      CURSORINFO cursorInfo;
      cursorInfo.cbSize = sizeof(CURSORINFO);
      if (GetCursorInfo(&cursorInfo)) {
        if (cursorInfo.flags & CursorShowing)   // <= V616
      ....
    }

    Для этого кода PVS-Studio выдал предупреждение:

    V616 CWE-480 The 'CursorShowing' named constant with the value of 0 is used in the bitwise operation. qwindowscursor.cpp 669

    Для проверки использовалась нестабильная версия PVS-Studio, поэтому моя вера в анализатор дрогнула. «Эх, что-то мы сломали в механизмах обработки неименованных перечислений», — вздохнул я, и выписал этот случай в багтрекер как ошибку, приводящую к ложному срабатыванию.

    Я был абсолютно уверен, что ошибается именно анализатор. Ведь всего несколькими строчками выше написано, что константа CursorShowing равна 1.

    При этом я старался быть внимательным! Я несколько раз просмотрел код, чтобы убедиться, что анализатор неправ. Я оформил этот фрагмент кода и соответствующее сообщение как ошибку в багтрекере.

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

    При подробном изучении ситуации выяснилось, что выше объявляется именованная константа cursorShowing, а в условии используется константа CursorShowing. Разница только в первой букве! В одном месте она строчная, а в другом прописная.

    Почему код компилируется? Потому, что константа CursorShowing тоже существует. Вот её объявление:

    class QWindowsCursor : public QPlatformCursor
    {
    public:
      enum CursorState {
        CursorShowing,
        CursorHidden,
        CursorSuppressed
      };
      ....
    }

    Как видите, константа CursorShowing равна 0. Поэтому анализатор PVS-Studio абсолютно прав, сообщая, что условие (cursorInfo.flags & CursorShowing) не имеет смысла. Условие является всегда ложным.

    Анализатор нашёл замечательную опечатку. Любите статический анализ кода! :)


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Once again the PVS-Studio analyzer has proved to be more attentive than a person.

    PVS-Studio

    394,10

    Ищем ошибки в C, C++ и C# на Windows, Linux, macOS

    Поделиться публикацией
    Комментарии 65
      +10
      А была бы CursorShowing в перечислении на втором месте? Тут скорее требуется предупреждение о том, что какие-то наименования идентификаторов отличаются только регистром, точнее «созвучны».
        +12
        Видимо, программирование на го обучило меня всегда смотреть на первую букву, но я ошибку заметил сразу.

        А вообще да, бывает, что сначала «анализатор не прав», а потом бьёшь себя по лбу. Поэтому предпочитаю, чтобы мои проекты все анализаторы проходили «на ноль».
          +3
          Я так понимаю религия не позволила использовать уже объявленный в <winuser.h>
          #define CURSOR_SHOWING 0x00000001

          Почему такой код вообще прошел ревью.
            +7

            Qt тащемта мультиплатформенный.

              +6
              Фрагмент кода из сугубо виндового файла, так что тут как раз вполне логично использовать системные дефайны, они же используют виндовую структуру CURSORINFO
              +1
              Да… и это ещё дополнительные проблемы вносит. Если в API сейчас написано 1, то завтра там может быть 42 :)
                +3
                Нет, не может. Эти константы вкомпилируются в тело программы, а совместимость под Windows — это святое.

                Вот в Linux — да, могло бы быть.
                  +1
                  А можно про поломку ABI линуксом? Вообще говоря, ядро обязуется поддерживать любой релизный ABI так долго, как его используют (и не ломать).
                    0
                    ABI и заголовочные файлы это разные вещи
                    В новых заголовочных файлах может быть какая-то функция перенаправлена на новую реализацию с новыми константами. То есть старый софт со старым ABI будет вызывать старую функцию со старыми константами, а новоскомпилированный — под тем же названием новую функцию с другими (бывает что с другими форматами структур вообще), которые в новом .h естественно учтены.
                      +1
                      В ядре такого нет. Новые ioctl/syscall и флаги могут появляться, старые остаются неизменными.
                      0
                      Тут вечная путаница. Между Linux (ядром) и GNU/Linux (полной операциокой). Ядро/glibc/libstd++ — у них с совместимостью если не прекрасно, то, по крайней мере «очень хорошо». Xы/Wayland — чуть похуже, но в целом терпимо.

                      А вот то, что выше (Gtk+ и прочее) — это тихой ужас.
                        0
                        Ну так gtk — это всего лишь одна из библиотек. Хочешь — ставишь, хочешь — не ставишь. Она даже не linux-specific, если уж на то пошло.
                          0
                          Хочешь — ставишь, хочешь — не ставишь.
                          Прекрасная идея. И как вы собираетесь писать программу, которая будет адекватно выглядеть у человека с Ubuntu или Fedora? Правильно реагировать на смену темы (светлая/тёмная/etc), на тыкание на иконку в трее (как вы её вообще туда поместите, кстати?) и так далее.

                          В том-то и дело, что для человка который пишет программу для себя и не собирается её распространять — в Linux всё нормально.

                          Но как только вы попытаетесь сделать хоть что-нибудь для людей — всё, катастрофа, совместимости нет, то что должно занимать часы превращается в многолетнюю борьбу и отбивает всякую охоту что-либо «для этих идиотов» делать…
                            0
                            Qt возьму. Там, кстати, с совместимостью даже слишком хорошо.
                              0
                              Если вы так в этом уверены, то сделайте хотя бы совсем смешную программу: просто иконка в трее, вы по ней кликаете, открывается менюшка, в которой есть пункт «hello», выбрав который вы откроете окно, где написано… ну пусть тоже «hello». Только с условием, что она будет работать во всех популярных дистрибутивах, которые ныне ещё поддерживаются: от RHEL5 до Ubuntu 18.04/Fedora 28.

                              В Windows такое делается за час и работает (если правильно скомпилировать) и в Windows XP и в Windows 2010. А если взять компилятор постарше — то будет и в Windows 95 работать с теми же исходниками.

                              А куда нас «великолепная совместимость» Qt доведёт?
                                0
                                современный Qt не поддерживает Win XP
                                  0
                                  А зачем для поддержки Windows XP нужна Qt? Там можно и на MFC и на чистом Win API всё сделать.
                      0
                      Так то да, я в общем смысле утрирую, но переопределять константы API тем более таких монстров как ОС это очень большое кю. Также ещё большим кю является использование API по принципу «работает же!», а не по принципу «API был так задуман разработчиками, его надо только так использовать». И если с константами действительно всё более менее хорошо, то вот с API может быть не очевидно и если сегодня функция пустышка не делает ничего, то завтра из-за отсутствия вызова «бесполезной» функции всё сломается.
                        0
                        Вот с API может быть не очевидно и если сегодня функция пустышка не делает ничего, то завтра из-за отсутствия вызова «бесполезной» функции всё сломается.
                        И это пример плохого API. Не нужно надеяться, что кто-то будет читать доку. Если функцию нужно обязательно вызывать — нужно сделать так, чтобы её невызов «наказывался» как-нибудь.
                          0
                          Ну, это типичная ситуация с API ОС, например, вчера что то в ядре было однопоточным или прибито гвоздями или ещё что и функция get_some возвращала указатель на константный кусок памяти, а return_some не делала ничего, а сейчас там развитие, многопоточность, и get_some возвращает копию куска памяти а return_some освобождает память.
                            0
                            И когда ты так сделаете, то обнаружите что return_some не вызывается достаточным числом программ для того, чтобы вам пришлось реализовать функцию get_some_fast and return_some_fast, которые всё будут делать так, как вы когда-то задумывали, а не так как оно реально работало.

                            А функции get_some и пустую return_some реализовать как-нибудь так, чтобы всё работало.

                            Контракты должны проверяться. Это истина, проверенная временем. Нельзя просто в документации про них написать и надеяться на то, что их будут соблюдать. Не будут. И не по злому умыслу, а потому что errare humanum est.

                            Если вы рассчитываете, что у вас в будущем будет по return_some память освобождаться… поставьте там, хотя бы, счётчик. И напишите строк 20 кода, который будет проверять что всё корректно используется.

                            Да, собственно, раз уж вы про soname завели речь. Пожалуйста, полюбуйтесь:
                            void* soinfo::to_handle() {
                              if (get_application_target_sdk_version() < __ANDROID_API_N__ || !has_min_version(3)) {
                                return this;
                              }
                              return reinterpret_cast<void*>(get_handle());
                            }
                            Почему bionic возвращает handle как указатель для старых приложений и/или для библиотек, у который version_ имеет версию ниже 3? Откуда, чёрт побери, вообще могут взяться version_ с версией ниже 3, если у нас в конструкторе написано?
                            
                            #define SOINFO_VERSION 4
                            ...
                            version_ = SOINFO_VERSION;
                            Что вообще происходит?

                            А очень просто: разработчики приложений, обнаружив, что dlopen возвращает ссылку на soinfo с радостью начали это использовать. И вставлять свои soinfo в этот список.

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

                            Ну а дальше — имеем то, что имеем…

                            Не будет разработчики читать доку и использовать API «как описано». Они будут «действовать по наитию», а в сложных случаях — читать год (даже если нет исходников всегд можно дизассемблировать). И будут делать программу минимальными усилиями.

                            Если после прочтения кода станет ясно, что есть два способа: «правильный» на 100 строк и неправильный на 3… будут использовать неправильный способ — и вы ничего с этим не сможете поделать… ваша задача сделать так, чтобы «правильный» способ занимал 30 строк, а «неправильный»… ну скажем, 300 — тогда всё будет хорошо.
                  –3
                  А вот не было бы регистрозависимости все было бы хорошо)
                    +6
                    и ТоГдА БЫ каждый пИсАл НАЗВАНИЯ методов КаК еМу ВзДУМАЕтСя
                      0
                      Уж ЛуЧше ТАк, ЧеМ ТрУДНОуловиМый баг. Ах да — есть такая штука, как автоформатирование кода.
                        0
                        Я работаю с регистронезависимым языком и ни разу не видел ничего кроме CamelStyle. Если есть регламент, его придерживаются все.
                        +3

                        Было бы хорошо, если бы ещё enum-ы в C/C++ не замусоривали пространство имён. Ибо должно быть CursorState::Showing и никак иначе. Очень надеюсь, что обычный enum станет deprecated в одной из будущих версий C++.

                          +3
                          Для того, чтобы обычный enum смог стать deprecated нужно, чтобы был действенный механизм переключения. Пока такого нет. auto_ptr смогли выпилить потому только, что он был слишком ущербен и его, по большому счёту, никто и никогда не использовал.
                            0
                            О, эта ерунда ещё в Delphi мне мешала. Когда на C# переходил, сначала удивился, что тип перечисления обязательно надо указывать, а потом понял, что только так и можно жить.
                              +1
                              Было бы хорошо, если бы ещё enum-ы в C/C++ не замусоривали пространство имён. Ибо должно быть CursorState::Showing и никак иначе.

                              enum class ведь решает проблему, а обратную совместимость вряд ли станут так гробить.
                            +3

                            Не люблю я "верблюжьи имена". Всегда пишу только в нижнем регистре с разделением подчеркиваниями.

                              0

                              Ну, с Qt еще куда ни шло. До очередного мажорного релиза, они, разумеется, вряд ли смогут поменять naming convention (учитывая, что он еще гвоздями к QtQuick прибит).


                              А вот кошмар во FreeRTOS — это да, страх и ужас, на который сами разработчики говорят "извините, но мы боимся ломать код всех наших клиентов, потому терпите"

                                +4
                                такоеНаписание яТоже неПонимаю, какМинимум этоНекрасиво.
                                Пишу или_так, ИлиТак.
                                писать_всё_в одном_стиле конечно_тоже вариант, но_глазу_не за_что_ухватиться при_разборе более-менее длинного_выражения.
                                Я, например, комбинирую, названия_классов и ИменаОбъектов.с_именами_функций().
                                  +2
                                  Я, например

                                  вот именно, что вы, а не все. А ведь помимо регистровой разницы еще бывают SmurfSmurf SmurfNaming, m_lpcwstrВенгерка, тлкСглсн (CamelCase/snake_case/beercase вариации), различные Способы выденения_ _членов this->класса, СПрефиксы СТипа ССущности (E — Enums, T — Types, etc.). Еще больше усугубляет проблему тот факт, что в стандарте верхний регистр не задействован, а некоторые функции в beercase вместо snake_case. Не хватает единообразия во всем этом зоопарке.
                                    0
                                    На C++ что, всегда так принято именовать константы? Уж лучше бы капсом их именовать — хоть и вырвиглазно, зато на такие грабли никто не наступит.

                                    Как же просто всё в Java: ClassName, varName, methodName, CONST_NAME.

                                    Хотя в общем и целом можно константы именовать так же, как и обычные переменные. Тогда не будет видно, что это константа, но зато сразу единый стиль для любой переменной (константной или нет).
                                      +1
                                      Как же просто всё в Java: ClassName, varName, methodName, CONST_NAME

                                      CONST_NAME пришло в Java качестве одного из стилей C.
                                      На мой взгляд, для Java это смотрится достаточно чужеродно, хотя хорошо уже то, что это стандарт.

                                      Больше импонирует C#-подход, когда константы выносятся в отдельный статический класс и именуются в PascalCase:
                                      Path.DirectorySeparatorChar
                                      Path.AltDirectorySeparatorChar

                                      Хотя иногда, как в случае того же Path, констатанты могут смешиваться с методами в одном классе.

                                      Хуже, когда в C# пытаются переносить Java-стандарт объявления констант (встречал в паре проектов): для C# это совсем чужеродно и размывает уже имеющиеся в языке стандарты.
                                        –1
                                        Как будет выглядеть единственная константа в классе? Всегда нужно мышкой идентификатор ласкать, чтобы отличить константу от свойства? А как быть без полноценной IDE (GitHub, BitBucket и т.п.)?
                                +4
                                Я думал там какой-то хитрый UB будет, а там всего лишь опечатка (достаточно заметная, кстати). При чуть более удачном стечении обстоятельств, ее бы заметил и компилятор: совпадение имен с точностью до регистра одного символа — это редкость и плохой стиль (вот тут бы статическому анализатору ругнуться). При чуть менее — ее не заметил бы и PVS — достаточно было бы этой константе оказаться не на нулевой позиции в енуме.
                                  +1
                                  Но в этом-то всё и дело! Сколько б ты ни был готов, всё равно самые мелкие мелочи не так уж и заметны — в какой-то момент они лопаются, ты хлопаешь себя по лбу со словами «вот я дурак», а до этого — в упор не видишь.
                                    0
                                    Так и PVS не может похвастаться тем, что ловит ошибки такого рода. А на побитовое «или» с нулем или что-то похожее мне и бесплатный clang analyzer ругался.
                                      0
                                      Да. Нету в мире абсолюта, кроме водки абсолют.
                                  +3
                                  Неужели IDE не подсвечивает использования имени? Стало бы сразу видно, что константы не используются
                                    –3
                                    Если использовать IDE с плагином PVS-Studio, то подсветит) Правда его не везде ещё «завезли», но мы работаем в этом направлении.
                                      +3
                                      KDevelop и CLion точно подсвечивают. Ровно по этой причине семантическая подсветка очень полезна.
                                      +8

                                      Я бы заменил в заголовке слово "человека" на слово "Андрея".

                                        –3
                                        Но ведь «Андрей» — ещё тот «Человечище»! По крайней мере в сфере анализа кода. :)
                                          +10
                                          Ляп-то в PVS-студио исправили? Он собственно в том, что в сообщении нету номера строки и имени файла, где объявлена константа. Отсюда и проблемы у пользователей, включая самих разработчиков.
                                        +2
                                        Признаться, слабый повод заводить тикет и тем более — писать об этом статью.
                                          +3
                                          Ну как сказать. Я повидал много *****. И раз, я так ошибся, значит в этом что-то есть. :)
                                          0

                                          О, до Qt добрались!
                                          А с Qt Creator работает?
                                          Есть несколько десятков живых проектов на Qt под Linux — было бы здорово их проверить.

                                            0
                                            Смотрите на нашем сайте раздел Быстрый старт/Любой проект. Быстрый анализ проекта из консоли без интеграции. В примере команды конвертера указан формат .tasks, который сделан специально для открытия в QtCreator. Предупреждения анализатора просматриваются в Issues Window.
                                            +1
                                            Заметил ошибку с третьего прочтения, даже при условии что обычно работаю с регистронезависимым кодом.
                                            Должно быть три предупреждения:
                                            1. Имя не соответствует соглашению о наименовании
                                            2. Имя имеет регистронезависимое совпадение
                                            3. Ну и что там сейчас про константу
                                              0
                                              Если с такими предупреждениями сунуться в незнакомый проект, который до сих пор форматом имён не заморачивался, то не выплывите.
                                                0
                                                И автоформатер с учётом коллизий имён!
                                                В любом случае такие вещи должны быть настраиваемыми.
                                              –1
                                              Подскажите, а зачем в условии вообще часть "& CursorShowing", если CursorShowing это константа и результат всего выражения зависит ТОЛЬКО от cursorInfo.flags?
                                                +3
                                                это битовая маска, результат зависит от результата вычисления побитового и
                                                +1

                                                Я добровольно-принудительно задаю правила по всем именованиям в решарпере для всего проекта. Да и вообще считаю хорошей практикой использовать инструменты для проверки codestyle. В моём случае решарпер бы сразу подсветил опасное место.

                                                  0
                                                  Используй венгерскую нотацию, Люк!
                                                    +1
                                                    Так это два enum. Нотация не спасет
                                                    +1
                                                    Вот за что и не люблю enum в C++ — можно указать члена без явного указания его пространства имён.
                                                      +1

                                                      Теперь есть enum class, который напоминает enum из C#

                                                      0
                                                      А что там с java версией?
                                                        0
                                                        Скоро начнём рассылать непубличную бета версию тем, кто высказал желание её попробовать. Можно присоединиться. Для этого напишите нам (выберите пункт «Хочу анализатор для Java»).
                                                          0
                                                          Уже присоединился(давно, когда еще в первый раз на хабре про это было написано). Скоро — это хорошо)).
                                                        0
                                                        Ну в строке
                                                        enum { cursorShowing = 0x1, cursorSuppressed = 0x2 };
                                                        кстати коде-стайл нарушен, значения енумов в Qt именуются с БольшойБуквы.
                                                        Тут не анализатор нужен, а clang-tidy/clang-formatter который должен бить по рукам тем кто не по код-стайл пишет.
                                                          +1
                                                          А вот и заметка про новую проверку Qt.

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

                                                          Самое читаемое