Шестая проверка Chromium, послесловие

    строгий единорог

    В начале 2018 года в нашем блоге появился цикл статей, посвящённый шестой проверке исходного кода проекта Chromium. Цикл включает в себя 8 статей, посвященных ошибкам и рекомендациям по их предотвращению. Две статьи вызвали бурное обсуждение, и до сих пор на почту мне изредка приходят комментарии касательно тем, затронутых в них. Пожалуй, следует дать некоторые дополнительные пояснения и, как говорится, расставить точки над i.

    Прошёл год с момента написания цикла статей, посвященного очередной проверке исходных кодов проекта Chromium:

    1. Chromium: шестая проверка проекта и 250 багов
    2. Красивый Chromium и корявый memset
    3. break и fallthrough
    4. Chromium: утечки памяти
    5. Chromium: опечатки
    6. Chromium: использование недостоверных данных
    7. Почему важно проверять, что вернула функция malloc
    8. Chromium: другие ошибки

    Статьи, посвящённые memset и malloc вызывали и продолжают вызывать дискуссии, которые кажутся мне странными. Видимо возникло некоторое недопонимание из-за того, что я недостаточно чётко сформулировал свои мысли. Я решил вернуться к этим статьям и внести некоторые уточнения.

    memset


    Начнём со статьи про memset, так как здесь всё просто. Возникли споры касательно того, как лучше инициализировать структуры. Достаточно много программистов написало, что лучше давать рекомендацию писать не:

    HDHITTESTINFO hhti = {};

    А так:

    HDHITTESTINFO hhti = { 0 };

    Аргументы:

    1. Конструкцию {0} легче заметить при чтении кода, чем {}.
    2. Конструкция {0} более интуитивно понятна, чем {}. То есть 0 подсказывает, что структура заполняется нулями.

    Соответственно, мне предлагают поменять этот пример инициализации в статье. Я не согласен с аргументами и не планирую вносить в статью правки. Сейчас я обосную свою позицию.

    По поводу заметности. Мне кажется, это дело вкуса и привычки. Не думаю, что наличие 0 внутри фигурных скобок принципиально меняет ситуацию.

    А вот со вторым аргументом я вообще не согласен. Запись вида {0} даёт повод неправильно воспринять код. Например, можно посчитать, что, если заменить 0 на 1, все поля будут инициализированы единицами. Поэтому такой стиль написания скорее вреден, чем полезен.

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

    V1009. Check the array initialization. Only the first element is initialized explicitly.

    Анализатор обнаружил возможную ошибку, связанную с тем, что при объявлении массива указано значение только для одного элемента. Таким образом, остальные элементы будут инициализированы неявно нулём или конструктором по умолчанию.

    Рассмотрим пример подозрительного кода:

    int arr[3] = {1};

    Возможно, программист ожидал, что arr будет состоять из одних единиц, но это не так. Массив будет состоять из значений 1, 0, 0.

    Корректный код:

    int arr[3] = {1, 1, 1};

    Подобная путаница может произойти из-за схожести с конструкцией arr = {0}, которая инициализирует весь массив нулями.

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

    Также не рекомендуется пренебрегать наглядностью кода.

    Например, код для кодирования значений цвета записан следующим образом:

    int White[3] = { 0xff, 0xff, 0xff };
    int Black[3] = { 0x00 };
    int Green[3] = { 0x00, 0xff };

    Благодаря неявной инициализации все цвета заданы правильно, но лучше переписать код более наглядно:

    int White[3] = { 0xff, 0xff, 0xff };
    int Black[3] = { 0x00, 0x00, 0x00 };
    int Green[3] = { 0x00, 0xff, 0x00 };

    malloc


    Прежде чем читать далее, прошу освежить в памяти содержимое статьи "Почему важно проверять, что вернула функция malloc". Эта статья породила много обсуждений и критики. Вот некоторые из дискуссий: reddit.com/r/cpp, reddit.com/r/C_Programming, habr.com. Изредка мне пишут по поводу этой статьи на почту и сейчас.

    Статья критикуется читателями по следующим пунктам:

    1. Если malloc вернула NULL, то лучше сразу завершить работу программы, чем писать кучу if-ов и пытаться как-то обработать нехватку памяти, из-за которой часто выполнение программы всё равно невозможно.

    Я вовсе не призывал до последнего бороться с последствиями нехватки памяти, пробрасывая ошибку всё выше и выше. Если для приложения допустимо завершить работу без предупреждения, то пусть так и будет. Для этого достаточно всего одной проверки сразу после malloc или использования xmalloc (см. следующий пункт).

    Я возражал и предупреждал об отсутствии проверок, из-за которых программа продолжает работать «как ни в чем не бывало». Это совсем другое. Это опасно, так как приводит к неопределённому поведению, порче данных и так далее.

    2. Не рассказано про решение, которое заключается в написании функций-врапперов для выделения памяти с последующей проверкой или использование уже существующих функций, таких как xmalloc.

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

    Функция xmalloc не является частью стандартной библиотеки C (см. "What is the difference between xmalloc and malloc?"). Однако эта функция может быть объявлена в других библиотеках, например, в GNU utils library (GNU libiberty).

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

    void* xmalloc(size_t s)
    {
      void* p = malloc(s);
      if (!p) {
        fprintf (stderr, "fatal: out of memory (xmalloc(%zu)).\n", s);
        exit(EXIT_FAILURE);
      }
      return p;
    }

    Соответственно, вызывая везде функцию xmalloc вместо malloc можно быть уверенным, что в программе не возникнет неопределённого поведения из-за какого-либо использования нулевого указателя.

    К сожалению, xmalloc тоже не является панацеей от всех бед. Надо помнить, что использование xmalloc недопустимо, если речь идёт о написании кода библиотек. Про это я поговорю чуть позже.

    3. Больше всего комментариев было следующего вида: «на практике malloc никогда не возвращает NULL».

    К счастью, не я один понимаю, что это неправильный подход. Мне очень понравился вот этот комментарий в мою поддержку:

    По опыту обсуждения подобной темы складывается ощущение, что в «Интернетах» есть две секты. Приверженцы первой свято уверены в том, что под Linux-ом malloc никогда не возвращает NULL. Приверженцы второй свято уверены в том, что если память в программе выделить не удалось, но ничего уже в принципе сделать нельзя, нужно только падать. Переубедить их никак нельзя. Особенно когда эти две секты пересекаются. Можно только принять это как данность. Причем не суть важно, на каком профильном ресурсе идёт обсуждение.

    Я подумал и решил последовать совету и не буду пытаться переубеждать :). Будем надеяться, что эти группы разработчиков пишут только некритичные программы. Если, например, какие-то данные испортятся в игре или игра упадёт, это не страшно.

    Единственное, что важно – чтобы так не делали разработчики библиотек, баз данных и т.д.

    Воззвание к разработчикам библиотек и ответственного кода


    Если вы разрабатываете библиотеку или иной ответственный код, то всегда проверяйте значение указателя, который вернула функция malloc/realloc, и возвращайте вовне код ошибки, если память выделить не удалось.

    В библиотеках нельзя вызвать функцию exit, если не удалось выделить память. По этой же причине нельзя использовать xmalloc. Для многих приложений недопустимо просто взять и аварийно завершить их работу. Из-за этого, например, может быть испорчена база данных. Могут быть потеряны данные, которые считались много часов. Из-за этого программа может быть подвержена уязвимостям «отказ в обслуживании», когда вместо того, чтобы как-то корректно обработать возрастающую нагрузку, работа многопоточного приложения просто завершается.

    Нельзя предположить, как и в каких проектах будет использована библиотека. Поэтому следует исходить из того, что приложение может решать очень ответственные задачи. И просто «убить» его, вызвав exit, никуда не годится. Скорее всего, такая программа написана с учётом возможности нехватки памяти и может что-то предпринять в этом случае. Например, некая CAD система из-за сильной фрагментации памяти не может выделить достаточный буфер памяти для очередной операции. Но это не повод ей завершиться в аварийном режиме с потерей данных. Программа может дать возможность сохранить проект и перезапустить себя в штатном режиме.

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

    Нельзя надеяться, что если malloc вернёт NULL, то программа упадёт. Произойти может всё что угодно. Как я описывал в статье, программа может писать данные вовсе не по нулевому адресу. В результате могут быть испорчены некие данные, что ведёт к непредсказуемым последствиям. Даже memset опасен. Если заполнение данных идёт в обратном порядке, то вначале портятся некие данные, и только потом программа упадёт. Но падение может произойти слишком поздно. Если в момент работы функции memset в параллельных потоках будут использованы испорченные данные, то последствия могут быть фатальны. Можно получить испорченную транзакцию в базе данных или отправку команды на удаление «ненужных» файлов. Может успеть произойти всё что угодно. Предлагаю читателю пофантазировать самостоятельно, к чему может привести использование мусора в памяти.

    Таким образом, у библиотеки есть только один правильный вариант работы с функциями malloc. Нужно СРАЗУ проверить, что вернула функция, и если это — NULL, то вернуть статус ошибки.

    Дополнительные ссылки


    1. OOM handling.
    2. Fun with NULL pointers: part 1, part 2.
    3. What Every C Programmer Should Know About Undefined Behavior: part 1, part 2, part 3.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Sixth Chromium Check, Afterword.
    PVS-Studio
    775,00
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Похожие публикации

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

      +4
      Могут быть потеряны данные, которые считались много часов

      И тут вспоминаются пакеты типа OpenFOAM, которые при нехватке памяти падают с сегфолтом.

        0

        А что вы предлагаете сделать? Сохранение данных на каждом шаге по времени там и так можно включить (причём можно делать ротацию, чтобы не занимать диск)

        +1
        По терминологии.
        Термин «ответвенный код» в переводе «responsible code» там не употребляем (погуглите — даст «код» в значении правило). Например, фраза «использование strdub() — безответственно» у нас будет иметь смысл в силу культурологических особенностей. А там — нет. А как быть если целые проекты имеются на базе strdub? Вполне успешные, даже с утечками памяти.
        Потому поднятый вами вопрос относится в области требований конкретного проекта, а не поведения программиста в целом. А писать в данном аспекте стоит о характеристиках типа fault-tolerance /отказоустойчивость или robustness /живучесть.

          –7
          В библиотеках нельзя вызвать функцию exit, если не удалось выделить память. По этой же причине нельзя использовать xmalloc. Для многих приложений недопустимо просто взять и аварийно завершить их работу.

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


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


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

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

            +5
            exit() в библиотечном коде не даст почти ничего сделать вызывающему коду, не кинет исключение, не вернет ошибку, не вызовет деструкторы у переменных на стеке. Даже в лог ничего осмысленного записать не выйдет. Плюс если это мультитред, то судя по этому: www.cplusplus.com/reference/cstdlib/exit это может еще и к race condition привести
              0
              В лог — выйдет. В зрелой OS будет поддержка исключений типа линуксовых сигналов.
                0
                Не до конца понял что имеется ввиду. После exit из пользовательских функций вызовется только та, которая зарегистрирована в atexit. Там можно будет послать самому себе SIGTERM или еще что-нибудь, но зачем? Стэктрейс можно сохранить наверное, но обработчик из atexit будет вызван и при корректном завершении, следовательно надо как-то отделить вызов exit из библиотеки от остальных вариантов нормального завершения. Или я что-то не понял
                  0
                  Библиотечная функция наверняка пошлет сигнал. Что можно перехватить и записать в ЛОГ.
                    0
                    Ну это от библиотеки зависит уже, может и не кинуть. Если перехватить, ну да, можно coredump сделать и что-нибудь записать. Правда сигнал может не только от этой функции придти но и откуда-то еще извне, в итоге запись в логе получится не такая информативная, как хотелось бы
                      –4
                      Не кинет — значит нарушит общие требования обработки нештатной ситуации. Дефект. Собственно об этом и статья — что делать в нештатных ситуациях.
              +8
              > Не соглашусь. Именно так происходит в языках с исключениями, и это правильно.
              Вы понимаете, что функция exit и исключения это совершенно разное?
                0
                То, что вы предложили — возвращать код ошибки — тоже плохо и ненадежно, так как нет гарантий, что кто-то будет результат функции проверять.

                https://habr.com/ru/post/324642/ — это стандарт же


                Аналогично нет никаких гарантий что исключение не проглотят.

                  +8
                  Да, нет гарантии, что исключение при нехватке памяти как-то обработают и вообще хотят обрабатывать. Но важно другое. Что делать с проблемой — это решать программе! Когда библиотека позвала exit(), тут особенно уже не развернуться.

                  Ещё раз. Я не прошу и не требую, что каждая программа обязана обрабатывать ситуации нехватки памяти :). Хотя и считаю отсутствие обработки плохим тоном.

                  Я настаиваю на позиции, что что библиотека не имеет права решать за других, что нехватка программы == прекращение работы. Библиотека должна вернуть статус ошибки/сгенерировать исключение. А как уже поступить с этой информацией, решать приложению.
                    +1
                    Я настаиваю на позиции, что что библиотека не имеет права решать за других
                    Золотые слова.

                    Есть разница между «не иметь возможности» и «не иметь желания» %)
                      +1
                      Извиняюсь, я отвечал Sabubu выше и имел ввиду что главное — обработать ошибку. А возвращаемый это код/errno или исключение принципиального значения не имеет.
                      Полностью согласен с тем, что сама программа должна решать что делать с ошибкой. Задача библиотеки ошибку донести, а не убиться об неё.
                  +5
                  Проверьте firefox
                    +2

                    По поводу malloc хочется передать пламенный привет libimagemagick. Там, конечно, исторически сложилось (сначала была cli-утилита, а уже потом из нее сделана библиотека), но это не повод чуть что сегфолтиться и оставлять за собой гигабайты временных файлов.

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

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