Открытый исходный код

В статьях мы регулярно повторяем важную мысль: статический анализатор должен использоваться регулярно. В этом случае многие ошибки выявляются на самом раннем этапе, а их исправление максимально дёшево. Однако теория это одно, но намного лучше подкреплять слова практическими примерами. Рассмотрим несколько свежих ошибок, появившихся в новом коде проекта 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 constrainedto 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 или аналогичного инструмента.

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

Фрагмент второй: 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, помогает выявить очередную интересную ошибку. Сегодня как раз такая статья, в которой мы рассмотрим баг в одной из реализаций стандарта кодирования двоичных данных Base64.

Всё началось с проверки свежей версии библиотеки Qt 6. Про это была отдельная классическая статья, где я описал 77 найденных ошибок. Так получилось, что вначале я решил бегло полистать отчёт, ещё не пряча предупреждения, относящиеся к сторонним библиотекам. Другими словами, я не отключил в настройках предупреждения, относящиеся к \src\3rdparty. И так вышло, что я сразу наткнулся на интересный пример ошибки в библиотеки Open Asset Import Library, про которую я решил сделать эту отдельную маленькую заметку.

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

Теперь перейдём, собственно, к ошибке, обнаруженной в Open Asset Import Library (assimp). Файл: \src\3rdparty\assimp\src\code\FBX\FBXUtil.cpp.

std::string EncodeBase64(const char* data, size_t length){    // calculate extra bytes needed to get a multiple of 3    size_t extraBytes = 3 - length % 3;    // number of base64 bytes    size_t encodedBytes = 4 * (length + extraBytes) / 3;    std::string encoded_string(encodedBytes, '=');    // read blocks of 3 bytes    for (size_t ib3 = 0; ib3 < length / 3; ib3++)    {        const size_t iByte = ib3 * 3;        const size_t iEncodedByte = ib3 * 4;        const char* currData = &data[iByte];        EncodeByteBlock(currData, encoded_string, iEncodedByte);    }    // if size of data is not a multiple of 3,    // also encode the final bytes (and add zeros where needed)    if (extraBytes > 0)    {        char finalBytes[4] = { 0,0,0,0 };        memcpy(&finalBytes[0], &data[length - length % 3], length % 3);        const size_t iEncodedByte = encodedBytes - 4;        EncodeByteBlock(&finalBytes[0], encoded_string, iEncodedByte);        // add '=' at the end        for (size_t i = 0; i < 4 * extraBytes / 3; i++)            encoded_string[encodedBytes - i - 1] = '=';    }    return encoded_string;}

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

1. 31 февраля;
2. Использование машинного обучения в статическом анализе исходного кода программ;
3. Как внедрить статический анализатор кода в legacy проект и не демотивировать команду.

Ok, продолжим. Перед нами реализация алгоритма кодирования строки байт в кодировку Base64. Это стандарт кодирования двоичных данных при помощи только 64 символов. Алфавит кодирования содержит текстово-цифровые латинские символы A-Z, a-z и 0-9 (62 знака) и 2 дополнительных символа, зависящих от системы реализации. Каждые 3 исходных байта кодируются 4 символами.

Если осталось закодировать только один или два байта, то в результате получаются только первые два или три символа строки, а выходная строка дополняется двумя или одним символами "=". Это предотвращает добавление дополнительных битов к восстановленным данным. Вот этот момент как раз реализован в рассматриваемой функции неправильно.

Если вы нашли ошибку, вы молодец. Если нет, то это тоже нормально. Нужно вникать в код, чтобы заметить, что что-то идёт не так. Анализатор про это "что-то не то" сообщает предупреждением: V547 [CWE-571] Expression 'extraBytes > 0' is always true. FBXUtil.cpp 224

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

// calculate extra bytes needed to get a multiple of 3size_t extraBytes = 3 - length % 3;

Программист планировал вычислить, сколько дополнительных байт входных данных нужно обработать, если их общее количество не равно 3. Для этого нужно просто поделить количество обрабатываемых байт по модулю 3. Правильный вариант инициализации переменной:

size_t extraBytes = length % 3;

Тогда, если обрабатывается, например, 5 байт, то получаем 5 % 3 = 2, и нужно дополнительно обработать 2 байта. Если на вход поступило 6 байт, то ничего отдельно обрабатывать не нужно, так как 6 % 3 = 0.

Но программист перемудрил и написал бессмысленный код:

size_t extraBytes = 3 - length % 3;

И как раз при анализе этого кода анализатору и понадобился механизм анализа потока данных. Какое бы значение не находилось в переменной length, после деления по модулю будет получено значение в диапазоне [0..2]. Анализатор PVS-Studio умеет работать с диапазонами, точными значениями и множествами. Т. е. речь идёт про Value Range Analysis. В данном случае будет использован именно диапазон значений.

Продолжим вычисления:

size_t extraBytes = 3 - [0..2];

Получается, что переменная extraBytes никогда не будет равна нулю. Анализатор вычислит следующий возможный диапазон её значений: [1..3].

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

if (extraBytes > 0)

Это простой, но красивый пример, когда анализ потока данных позволил вычислить диапазон значений переменной, проследить, что она не изменяется и, наконец, то, что условие всегда истинно.

Конечно, некорректность работы функции не ограничивается тем, что будет выполнять фрагмент кода, который выполняться не должен. Там вообще всё идёт вкривь и вкось. Предположим, что требуется закодировать 6 символов. В этом случае выходная строка должна содержать 8 символов. Давайте быстренько прикинем, как поведёт себя рассмотренная функция.

// calculate extra bytes needed to get a multiple of 3size_t extraBytes = 3 - length % 3; // 3-6%3 = 3// number of base64 bytessize_t encodedBytes = 4 * (length + extraBytes) / 3; // 4*(6+3)/3 = 12std::string encoded_string(encodedBytes, '=');

Уже получилось, что выходная строка будет содержать 12 символов, а не 8. Дальше тоже всё будет работать неправильно даже нет смысла вдаваться в подробности.

Вот так легко и просто статический анализ нашёл ошибку в коде. А представьте, ведь кто-то будет мучиться и отлаживаться, чтобы понять, почему неправильно происходит кодирование символов в кодировку Base64. Здесь мы, кстати, подходим к вопросу качества используемых сторонних библиотек, который я рассматривал в публикации "Почему важно проводить статический анализ открытых библиотек, которые вы добавляете в свой проект".

Попробуйте внедрить регулярное использование PVS-Studio в ваш процесс разработки, чтобы находить многие ошибки на самом раннем этапе. Вам понравится :). Если вы разрабатываете открытый проект, то анализатор можно использовать бесплатно. Спасибо за внимание и безбажного вам кода.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Why PVS-Studio Uses Data Flow Analysis: Based on Gripping Error in Open Asset Import Library.