PVS-Studio, Blender: цикл заметок о пользе регулярного использования статического анализа

    PVS-Studio мониторит код Blender


    В статьях мы регулярно повторяем важную мысль: статический анализатор должен использоваться регулярно. В этом случае многие ошибки выявляются на самом раннем этапе, а их исправление максимально дёшево. Однако теория – это одно, но намного лучше подкреплять слова практическими примерами. Рассмотрим несколько свежих ошибок, появившихся в новом коде проекта Blender.


    Недавно мы настроили регулярную проверку проекта Blender, о чём мой коллега рассказал в статье "Just for fun: команда PVS-Studio придумала мониторить качество некоторых открытых проектов". В дальнейшем планируем начать мониторить ещё некоторые интересные проекты.


    Сразу скажу, что мы не ставим перед собой задачу найти как можно больше ошибок. Целью является периодическое написание небольших заметок (таких как эта), в которых мы будем на практике показывать достоинства регулярного анализа кода. Другими словами, мы иногда будем описывать некоторые интересные ошибки в новом коде, найденные при очередном ночном запуске PVS-Studio, и тем самым популяризировать правильное использование методологии статического анализа кода.


    Итак, давайте посмотрим, что найдено в свежем коде проекта Blender.


    Фрагмент первый: double-checked locking


    typedef struct bNodeTree {
      ....
      struct NodeTreeUIStorage *ui_storage;
    } bNodeTree;
    
    static void ui_storage_ensure(bNodeTree &ntree)
    {
      /* As an optimization, only acquire a lock if the UI storage doesn't exist,
       * because it only needs to be allocated once for every node tree. */
      if (ntree.ui_storage == nullptr) {
        std::lock_guard<std::mutex> lock(global_ui_storage_mutex);
        /* Check again-- another thread may have allocated the storage
           while this one waited. */
        if (ntree.ui_storage == nullptr) {
          ntree.ui_storage = new NodeTreeUIStorage();
        }
      }
    }

    Предупреждение PVS-Studio. V1036: Potentially unsafe double-checked locking. node_ui_storage.cc 46


    Перед нами неправильная реализация блокировки с двойной проверкой. Для пояснения проблемы процитирую фрагмент статьи "C++ and the Perils of Double-Checked Locking", написанной Scott Meyers и Andrei Alexandrescu ещё в 2004 году. Как видите, проблема давно известна, но это не защищает разработчиков от того, чтобы наступать на одни и те же грабли. Хорошо, что анализатор PVS-Studio помогает выявлять подобные проблемы :). Итак, фрагмент из статьи:


    Consider again the line that initializes pInstance: pInstance = newSingleton;

    This statement causes three things to happen:

    Step 1: Allocate memory to hold a Singleton object.

    Step 2: Construct a Singleton object in the allocated memory.

    Step 3: Make pInstance point to the allocated memory.

    Of critical importance is the observation that compilers are not constrained to perform these steps in this order! In particular, compilers are sometimes allowed to swap steps 2 and 3. Why they might want to do that is a question we'll address in a moment. For now, let's focus on what happens if they do.

    Consider the following code, where we've expanded pInstance's initialization line into the three constituent tasks we mentioned above and where we've merged steps 1 (memory allocation) and 3 (pInstance assignment) into a single statement that precedes step 2 (Singleton construction). The idea is not that a human would write this code. Rather, it's that a compiler might generate code equivalent to this in response to the conventional DCLP source code (shown earlier) that a human would write.

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


    Подобные ошибки очень коварны! Они могут очень редко проявлять себя. Программа вроде как работает, проходит все тесты и так далее. Но время от времени она неожиданно падает у пользователей, и понять причину может быть крайне проблематично. Воспроизвести такую ошибку может быть очень сложно. Другим словами, исправление этой ошибки по жалобе от пользователей может обойтись в 1000 раз дороже, чем правка кода, сделанная по результату анализа кода с помощью PVS-Studio или аналогичного инструмента.


    Примечание 1. Ошибка может и не существовать сейчас в двоичном коде, и всё зависит от компилятора, и ключей оптимизации. Однако, даже если сейчас всё работает хорошо, это не значит, что так будет всегда. Ошибка может проявить себя при смене компилятора/ключей оптимизации.


    Примечание 2. Читатели обратили внимание, что описание проблемы блокировки с двойной проверкой устарело. В C++17 язык гарантированно выполняет все побочные эффекты, связанные с выражением new T перед выполнением эффектов самого присваивания (operator =). Другими словами, начиная с C++17, эту часть можно назвать "уже пофиксили, это не баг". Однако, запись всё равно не является атомарной, и возможно возникновение состояния гонки. Чтобы этого избежать, указатель должен быть объявлен как атомарный: std::atomic<NodeTreeUIStorage *> ui_storage\.


    Фрагмент второй: realloc


    static void icon_merge_context_register_icon(struct IconMergeContext *context,
                                                 const char *file_name,
                                                 struct IconHead *icon_head)
    {
      context->read_icons = realloc(context->read_icons,
        sizeof(struct IconInfo) * (context->num_read_icons + 1));
      struct IconInfo *icon_info = &context->read_icons[context->num_read_icons];
      icon_info->head = *icon_head;
      icon_info->file_name = strdup(path_basename(file_name));
      context->num_read_icons++;
    }

    Анализатор PVS-Studio выдаёт здесь два предупреждения, и это правильно. Здесь действительно допущено сразу две ошибки различного плана.


    Первая: V701: realloc() possible leak: when realloc() fails in allocating memory, original pointer 'context->read_icons' is lost. Consider assigning realloc() to a temporary pointer. datatoc_icon.c 252


    Если память не удастся выделить, функция realloc вернёт значение NULL. Нулевой указатель будет записан в переменную context->read_icons, а её предыдущее значение будет потеряно. Раз предыдущее значение указателя потеряно, то и невозможно освободить ранее выделенный блок памяти, на который ссылался этот указатель. Произойдёт утечка памяти.


    Вторая: V522: There might be dereferencing of a potential null pointer 'context->read_icons'. Check lines: 255, 252. datatoc_icon.c


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


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


    Фрагмент третий: разыменование указателя до проверки


    static int node_link_invoke(bContext *C, wmOperator *op, const wmEvent *event)
    {
      ....
      bNodeLinkDrag *nldrag = node_link_init(bmain, snode, cursor, detach);
      nldrag->last_picked_multi_input_socket_link = NULL;
      if (nldrag) {
        op->customdata = nldrag;
      ....
    }

    Предупреждение PVS-Studio: V595: The 'nldrag' pointer was utilized before it was verified against nullptr. Check lines: 1037, 1039. node_relationships.c


    Один из самых частых паттернов ошибок (proof). В начале указатель nldrag разыменовывается. Но из следующего условного оператора становится видно, что на самом деле этот указатель может быть нулевым.


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


    Кстати, нашлась ещё одна такая-же ошибка, но описывать её неинтересно. Приведу только сообщение: V595: The 'seq' pointer was utilized before it was verified against nullptr. Check lines: 373, 385. strip_add.c


    Заключение


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


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio, Blender: Series of Notes on Advantages of Regular Static Analysis of Code.

    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

    Комментарии 14

      0
      Давно читаю эти разборы, но только сейчас пришла в голову мысль попросить проверить kdenlive. Как у пользователя, есть большое подозрение, что материала вам там будет предостаточно :)
        –1

        А почему вы описываете только пользу? А как же вред, чтоб ROI посчитать?
        Помню только раз это предлагали, но стоит тогда в фак добавить что-ли =)

        0

        Насчёт "неправильной реализации блокировки с двойной проверкой" — здесь она вполне себе правильная до тех пор, пока в тех местах, в которых идёт дальнейшая работа с ui_storage (и которые здесь не показаны), до обращения к нему, как положено, захватывается соответствующий мутекс. Поскольку ui_storage_ensure() не возвращает указатель или ссылку на ui_storage для дальнейшего использования где-то ещё, то именно в этом конкретном коде нет риска, что будет обращение к внутренностям ui_storage до его полной инициализации из-за реордеринга, некогерентности кешей и прочего.

          +1

          Хотя да, я не совсем прав — не обратил внимания, что сам указатель ui_storage не atomic, это проблема. Он должен быть atomic с доступом хотя бы memory_order_relaxed — дополнительные fences и ограничения на ордеринг тут по идее не нужны, мутекса достаточно, а вот операции с самим указателем все-таки должны быть по-хорошему атомарными.

            0
            дополнительные fences и ограничения на ордеринг тут по идее не нужны, мутекса достаточно

            Проблема в том, что в конструкции
            p = new P();

            происходят три действия:
            1. Выделение памяти
            2. Конструирование объекта по полученному адресу
            3. Запись адреса в переменную

            И при определённых условиях компилятор может переставить запись в переменную до конструирования объекта. Как следствие первое чтение (не под мьютексом) считает адрес объекта, который ещё не сконструирован, и его использование приведёт к неопределённому поведению.

            Теперь предположим, что мы сделаем так, как вы сказали
            p.store(new P(), std::memory_order_relaxed);

            Может ли теперь компилятор переупорядочить действия?
            Всё же здесь вызывается функция с результатом new-expression в качестве аргумента, но ведь и компилятор может в тело этой функцию «подсмотреть» и понять, что ей важно именно побитовое значение адреса, а вовсе не корректно сконструированный объект по этому адресу. Так что с этой точки зрения вроде как может переупорядочить.
            С другой стороны это атомарная переменная, и компилятор понимает, что вполне законно её чтение в другом потоке, а вот там уже вполне могут использовать адрес для обращения к объекту (иначе зачем нам эта переменная?). С этой стороны получается, что переупорядочивание невозможно.

            Но тут есть и второй источник переупорядочивания — процессор. И вот он вполне может при отсутствии должного барьера памяти сперва исполнить запись адреса в переменную, а уже потом — запись поля объекта из конструктора. Что опять таки приводит нас к потенциальному неопределённому поведению. Насколько я понимаю, такое переупорядочивание запись-запись невозможно на x86/x64, но вполне возможно на других архитектурах.

            Так что придётся всё же чётко указать наши намерения
            p.store(new P(), std::memory_order_release);

            Но тогда возникает вопрос: а читать с memory_order_relaxed можно?
            Ну, когда мы читаем во второй раз под мьютексом — можно. Мьютекс обеспечит нам необходимые барьеры оптимизации и памяти, потому что изменяем переменную мы тоже под мьютексом. А в первый раз?

            А вот в первый раз не можем, нам потребуется memory_order_acquire. Дело в том, что при сохранении переменной процессор всё ещё может переупорядочить записи указателя и полей в объекте. Просто потом он исполнит инструкцию, синхронизирующую видимое состояние памяти для разных ядер/процессоров (обеспечит когерентность кэша). И вот если другой поток прочитает значение указателя до исполнения этой инструкции, то вполне может «увидеть» его значение, но не значение полей объекта — и опять наше любимое неопределённое поведение. В модели памяти C++ это выражается отношением «синхронизируется-с» (synchronizes-with). И такое отношение есть между memory_order_acquire и memory_order_release, но memory_order_relaxed не синхронизируется ни с чем.

            Так что код из статьи надо переделать вот так
            typedef struct bNodeTree {
              ....
              std::atomic<NodeTreeUIStorage*> ui_storage;
            } bNodeTree;
            
            static void ui_storage_ensure(bNodeTree &ntree)
            {
              /* As an optimization, only acquire a lock if the UI storage doesn't exist,
               * because it only needs to be allocated once for every node tree. */
              if (ntree.ui_storage.load(std::memory_order_aquire) == nullptr) {
                std::lock_guard<std::mutex> lock(global_ui_storage_mutex);
                /* Check again-- another thread may have allocated the storage
                   while this one waited. */
                if (ntree.ui_storage.load(std::memory_order_relaxed) == nullptr) {
                  ntree.ui_storage.store(new NodeTreeUIStorage(), std::memory_order_release);
                }
              }
            }


            На данную тему есть интересная статья — Double-Checked Locking is Fixed In C++11. Да и вообще в этом блоге много чего интересного на тему многопоточности и атомарных переменных.
              0
              Дело в том, что при сохранении переменной процессор всё ещё может переупорядочить записи указателя и полей в объекте. Просто потом он исполнит инструкцию, синхронизирующую видимое состояние памяти для разных ядер/процессоров (обеспечит когерентность кэша)

              Тут я, конечно, соврал. Пишущий поток не переупорядочит запись и выполнит синхронизацию перед записью атомарной переменной. Дело в другом — читающий поток должен выполнить инструкцию синхронизации, чтобы увидеть не только значение указателя, но и остальных данных. Для memory_order_relaxed это необязательно ввиду отсутствия соответствующего отношения «синхронизируется-с».
                0
                Как следствие первое чтение (не под мьютексом)

                Какое «первое чтение (не под мьютексом)»? Там нет никакого чтения. Покажите мне в обсуждаемом коде чтение содержимого по адресу, на который указывает ui_storage, не под мьютексом:

                static void ui_storage_ensure(bNodeTree &ntree)
                {
                  /* As an optimization, only acquire a lock if the UI storage doesn't exist,
                   * because it only needs to be allocated once for every node tree. */
                  if (ntree.ui_storage == nullptr) {
                    std::lock_guard<std::mutex> lock(global_ui_storage_mutex);
                    /* Check again-- another thread may have allocated the storage
                       while this one waited. */
                    if (ntree.ui_storage == nullptr) {
                      ntree.ui_storage = new NodeTreeUIStorage();
                    }
                  }
                }
                

                Этого чтения там нет. В каком бы порядке ни выполнялись те действия, о которых вы пишете, ничего плохого в данной конкретной функции произойти не может (ну если не считать неатомарности самого указателя). В других местах — там да, будет нужен мутекс, о чем я уже написал. Но это в других местах, которых тут нет.
                  0
                  Какое «первое чтение (не под мьютексом)»? Там нет никакого чтения. Покажите мне в обсуждаемом коде чтение содержимого по адресу, на который указывает ui_storage, не под мьютексом

                  Ну, так вот же:
                  if (ntree.ui_storage == nullptr)

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

                  Ну, как скажете. Ваш код — вам и отлаживать. Но я вас предупредил.
            0
            Прочитал статью про блокировку с двойной проверкой. Для меня это был взрыв мозга. Огромное спасибо автору!
              0
              Как вы вообще оцениваете качество кода Blender? Вы назвали несколько найденных ошибок из свежих изменений, и мне любопытно, насколько их в принципе много было во всем коде проекта, какие наиболее частые?
                0
                Сложно сказать. Я вижу поток ошибок и недочётов, но мне просто неинтересно на них как-то реагировать. А ждал, когда будет одновременно парочка интересных багов, про которые можно сделать заметку. Например, сейчас ткнул мышкой в один из недавних прогонов от 25 февраля 2021. Вижу 3 предупреждения. Два мне с ходу непонятны, с ними требуется разбираться, а мне это скучно (лень). Для простоты будем считать их ложными срабатываниями. Остаётся:
                typedef enum {
                  ....
                  T_OVERRIDE_CENTER = 1 << 18,
                  ....
                } eTFlag;
                ....
                if (t->flag | T_OVERRIDE_CENTER) {

                Предупреждение PVS-Studio: V617: Consider inspecting the condition. The 'T_OVERRIDE_CENTER' argument of the '|' bitwise operation contains a non-zero value. transform_convert.c. 85

                Это явная ошибка. Из-за того что перепутали оператор, условие будет всегда истинным. Правильный вариант:
                if (t->flag & T_OVERRIDE_CENTER) {

                Но ранее я посмотрел на эту ошибку и закрыл отчёт. Не буду же я с каждой такой ошибкой бегать и всем её показывать :). Мне будет скучновато всё подобное описывать, а подписчикам – читать.

                Сообщил я разработчикам проекта? Нет. Это их работа – обеспечивать качество кода. Я не могу помочь всем. Вокруг меня тысячи открытых проектов, в которых каждый день появляются новые баги. Даже если я захочу, много я не направлю. Намного продуктивнее популяризировать методологию статического анализа кода. Тогда такие ошибки сразу будут находить сами разработчики проектов.

                Если я постоянно вижу всё новые и новые ошибки в Blender, почему я не говорю, что качество кода низкое? Ответ: проект большой. А чем больше проект, тем больше ошибок появляется в коде. Так что я не берусь судить в данном случае. Впрочем, статический анализ этому проекту явно не помешает.

                P.S. Я немного побуду злодеем. Я не буду агитировать разработчиков Blender начать использовать PVS-Studio. А то я лишусь возможности находить ошибки в этом интересном проекте, и мне придётся искать другой проект :).

                  0
                  > и мне любопытно, насколько их в принципе много было во всем коде проекта, какие наиболее частые?
                  Много. Но я их не изучал. Кстати, мы и раньше много находили: www.viva64.com/ru/b/0145, www.viva64.com/ru/b/0413
                  +1
                  Примечание 2. Читатели обратили внимание, что описание проблемы блокировки с двойной проверкой устарело. В C++17 язык гарантированно выполняет все побочные эффекты, связанные с выражением new T перед выполнением эффектов самого присваивания (operator =). Другими словами, начиная с C++17, эту часть можно назвать «уже пофиксили, это не баг». Однако, запись всё равно не является атомарной, и возможно возникновение состояния гонки. Чтобы этого избежать, указатель должен быть объявлен как атомарный: std::atomic<NodeTreeUIStorage *> ui_storage

                  Поскольку я читал статью раньше, то вчера во время написания комментария не заметил это примечание.

                  Дело в том, что эти изменения в C++17 для случая, когда мы можем заметить разницу в состоянии абстрактной машины C++ в зависимости от последовательности вычислений. Например, код
                  i = ++i + 1;

                  приводил к неопределённому поведению до C++17, поскольку вычисления инкремента i и присваивания не упорядочивались относительно друг друга. Но если взять код
                  p = new A();

                  то заметим ли мы разницу в состоянии абстрактной машины, если сначала произойдёт присвоение указателя, а уже потом — инициализация объекта?

                  Да, мы можем заметить разницу, если 1) конструктор выбросит исключение или 2) в конструкторе будут вычисления на основе текущего значения p.
                  Но если ни одно из этих условий не выполнено, то различий не будет. Или же компилятор встроит вызов конструктора так, что выброс исключения и/или вычисления на основе текущего p будут перед присваиванием p, а все остальные вычисления после, то разницы мы так же не заметим.

                  Ну, а пока мы не видим различий в поведении программы, то руки компилятора в плане оптимизации развязаны, и он всё ещё может осуществить присваивание указателя до инициализации полей. И не только может, но и делает.

                  Но даже если компилятор сгенерирует команды в ожидаемом нами порядке, и мы даже сделаем указатель атомарным с доступом memory_order_relaxed, этого недостаточно. Нам всё же необходимо, чтобы другие потоки увидели значение указателя не раньше, чем правильно инициализированные поля объекта, на который он указывает. А для этого необходимы барьеры памяти aquire — release.

                  Так что изменения в C++17 ничего в данном вопросе не исправили. Они просто уменьшили количество ситуаций, в которых может произойти неопределённое поведение, но проблема в данном случае не в нём.

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

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