Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
GBP
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

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

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Брутальный Protocol Buffers от Google v…

Брутальный Protocol Buffers от Google vs статический анализ кода

05 Ноя 2021

Protocol Buffers — это очень популярный, крутой и качественный проект, развиваемый в основном компанией Google. Это хороший вызов для статического анализатора кода PVS-Studio. Найти хоть что-то — это уже достижение. Попробуем.

0882_Protocol_Buffers_vs_PVS_Studio_ru/image1.png

Продолжая наш многолетний цикл публикаций про проверку открытых проектов, я обратил внимание на проект Protocol Buffers (protobuf). Библиотека реализует протокол сериализации (передачи) структурированных данных. Это эффективная бинарная альтернатива текстовому формату XML.

Проект показался мне интересным вызовом для анализатора PVS-Studio, ведь компания Google весьма основательно подходит к качеству разрабатываемого C++ кода. Взять хотя бы документ "Безопасное использование C++", который недавно активно обсуждался. Дополнительно protobuf используется большим количеством разработчиков в своих проектах и потому хорошо ими протестирован. Найти хотя бы пару ошибок в этом проекте является достижением, которое будет лестно нашей команде. Так чего мы ждём, вперёд!

0882_Protocol_Buffers_vs_PVS_Studio_ru/image2.png

Раньше мы никогда специально не проверяли этот проект. Однажды, три года назад, мы заглянули в него, когда писали цикл статей про проверку проекта Chromium. Мы заметили интересную ошибку в функции проверки даты, которую описали в отдельной заметке "31 февраля".

Признаюсь, у меня была задумка, когда я писал эту статью. Я хотел показать возможности нового механизма межмодульного анализа С++ проектов. К сожалению, в этот раз межмодульный анализ ничего нового не привнёс. Что с ним, что без него — находятся одни и те же интересные места. Впрочем, это неудивительно. В этом проекте вообще сложно что-то найти :).

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

Copy-Paste

void SetPrimitiveVariables(....) {
  ....
  if (HasHasbit(descriptor)) {
    (*variables)["get_has_field_bit_message"] = ....;
    (*variables)["set_has_field_bit_message"] = ....;
    (*variables)["clear_has_field_bit_message"] = ....;
    ....
  } else {
    (*variables)["set_has_field_bit_message"] = "";      // <=
    (*variables)["set_has_field_bit_message"] = "";      // <=
    (*variables)["clear_has_field_bit_message"] = "";
  ....
}

Предупреждение PVS-Studio: V519 [CWE-563] The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 163, 164. java_primitive_field_lite.cc 164

Классическая ошибка, возникшая в процессе копирования строк. Где-то фрагмент строки заменили, где-то нет. В результате ключом два раза является "set_has_field_bit_message".

Если посмотреть код выше, то становится понятно, что на самом деле в else-ветке планировалось написать:

(*variables)["get_has_field_bit_message"] = "";
(*variables)["set_has_field_bit_message"] = "";
(*variables)["clear_has_field_bit_message"] = "";

Утечка файлового дескриптора

ExpandWildcardsResult ExpandWildcards(
    const string& path, std::function<void(const string&)> consume) {
  ....
  HANDLE handle = ::FindFirstFileW(wpath.c_str(), &metadata);
  ....
  do {
    // Ignore ".", "..", and directories.
    if ((metadata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0 &&
        kDot != metadata.cFileName && kDotDot != metadata.cFileName) {
      matched = ExpandWildcardsResult::kSuccess;
      string filename;
      if (!strings::wcs_to_utf8(metadata.cFileName, &filename)) {
        return ExpandWildcardsResult::kErrorOutputPathConversion;       // <=
      }
    ....
  } while (::FindNextFileW(handle, &metadata));
  FindClose(handle);
  return matched;
}

Предупреждение PVS-Studio: V773 [CWE-401] The function was exited without releasing the 'handle' handle. A resource leak is possible. io_win32.cc 400

Перед выходом из функции файловый дескриптор handle должен быть закрыт с помощью вызова FindClose(handle). Однако этого не произойдёт, если не удастся сконвертировать текст из формата UTF-8-encoded в UTF-8. Функция просто завершит работу и вернёт статус ошибки.

Потенциальное переполнение

uint32_t GetFieldOffset(const FieldDescriptor* field) const {
  if (InRealOneof(field)) {
    size_t offset =
        static_cast<size_t>(field->containing_type()->field_count() +
                            field->containing_oneof()->index());
    return OffsetValue(offsets_[offset], field->type());
  } else {
    return GetFieldOffsetNonOneof(field);
  }
}

Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands, not the result. generated_message_reflection.h 140

Складываются два значение типа int и помещаются в переменную типа size_t:

size_t offset = static_cast<size_t>(int_var_1 + int_var_2);

Предполагается, что в случае 64-битной сборки сумма значений двух 32-битных переменных может превысить значение INT_MAX. Поэтому результат помещается в переменную типа size_t, которая будет 64-битной в 64-битной программе. Более того, понимая, что при сложении двух int может произойти переполнение, программистом используется явное приведение типа.

Вот только используется это явное приведение типа неправильно. Оно ни от чего не защищает. И без него сработало бы неявное расширение типа от int к size_t. Так что написанный код ничем не отличается от варианта:

size_t offset = int_var_1 + int_var_2;

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

size_t offset = static_cast<size_t>(int_var_1) + int_var_2;

Разыменование нулевого указателя

bool KotlinGenerator::Generate(....)
{
  ....
  std::unique_ptr<FileGenerator> file_generator;
  if (file_options.generate_immutable_code) {
    file_generator.reset(
        new FileGenerator(file, file_options, /* immutable_api = */ true));
  }

  if (!file_generator->Validate(error)) {
    return false;
  }
  ....
}

Предупреждение PVS-Studio: V614 [CWE-457] Potentially null smart pointer 'file_generator' used. java_kotlin_generator.cc 100

Если переменная generate_immutable_code вдруг окажется равна fasle, то умный указатель file_generator останется равен nullptr. Как следствие, произойдёт разыменование нулевого указателя.

Видимо, пока переменная generate_immutable_code всегда истинна, раз эта ошибка ещё не обнаружена. Её можно счесть несущественной. Как только в процессе редактирования кода логика работы изменится, то произойдёт разыменование нулевого указателя, что будет сразу замечено и исправлено. А с другой стороны, в этом коде находится мина, которую лучше найти и исправить заранее, чем ждать, когда кто-то на ней подорвётся в будущем. Суть статического анализа как раз том, чтобы найти ошибки заранее.

Там ли скобка?

AlphaNum::AlphaNum(strings::Hex hex) {
  char *const end = &digits[kFastToBufferSize];
  char *writer = end;
  uint64 value = hex.value;
  uint64 width = hex.spec;
  // We accomplish minimum width by OR'ing in 0x10000 to the user's value,
  // where 0x10000 is the smallest hex number that is as wide as the user
  // asked for.
  uint64 mask = ((static_cast<uint64>(1) << (width - 1) * 4)) | value;
  ....
}

Нас интересует это подвыражение:

((static_cast<uint64>(1) << (width - 1) * 4))

Код не нравится анализатору сразу по 2 причинам:

  • V634 [CWE-783] The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. strutil.cc 1408
  • V592 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. strutil.cc 1408

Согласитесь, предупреждения дополняют друг друга. Есть совместное использование оператора сдвига и умножения. Легко забыть, какой из этих операторов более приоритетен. А повторяющиеся скобочки намекают на то, что про эту неоднозначность знали и хотели её избежать. Но не получилось.

Есть два варианта. Первый: код корректен. В этом случае дополнительные скобки просто должны упрощать чтение кода, но ни на что не влияют:

uint64 mask = (static_cast<uint64>(1) << ((width - 1) * 4)) | value;

Второй: выражение написано с ошибкой. Тогда дополнительные скобки должны поменять последовательность выполняемых операций:

uint64 mask = ((static_cast<uint64>(1) << (width - 1)) * 4) | value;

Заключение

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

0882_Protocol_Buffers_vs_PVS_Studio_ru/image3.png

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

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

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

Популярные статьи по теме
Любите статический анализ кода!

Дата: 16 Окт 2017

Автор: Андрей Карпов

Я в шоке от возможностей статического анализа кода, хотя сам участвую в разработке инструмента PVS-Studio. На днях я был искренне удивлён тому, что анализатор оказался умнее и внимательнее меня.
Статический анализ как часть процесса разработки Unreal Engine

Дата: 27 Июн 2017

Автор: Андрей Карпов

Проект Unreal Engine развивается - добавляется новый код и изменятся уже написанный. Неизбежное следствие развития проекта - появление в коде новых ошибок, которые желательно выявлять как можно раньш…
Зло живёт в функциях сравнения

Дата: 19 Май 2017

Автор: Андрей Карпов

Возможно, читатели помнят мою статью под названием "Эффект последней строки". В ней идёт речь о замеченной мной закономерности: ошибка чаще всего допускается в последней строке однотипных блоков текс…
Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей

Дата: 21 Ноя 2018

Автор: Андрей Карпов

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

Дата: 20 Мар 2017

Автор: Андрей Карпов

В своей предыдущей статье я писал, что мне не нравится подход, при котором статические анализаторы кода оцениваются с помощью синтетических тестов. В статье приводился пример, воспринимаемый анализат…
Бесплатный PVS-Studio для тех, кто развивает открытые проекты

Дата: 22 Дек 2018

Автор: Андрей Карпов

В канун празднования нового 2019 года команда PVS-Studio решила сделать приятный подарок всем контрибьюторам open-source проектов, хостящихся на GitHub, GitLab или Bitbucket. Им предоставляется возмо…
Эффект последней строки

Дата: 31 Май 2014

Автор: Андрей Карпов

Я изучил множество ошибок, возникающих в результате копирования кода. И утверждаю, что чаще всего ошибки допускают в последнем фрагменте однотипного кода. Ранее я не встречал в книгах описания этого …
Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний

Дата: 31 Июл 2017

Автор: Андрей Карпов

После большой статьи про проверку операционной системы Tizen мне было задано много вопросов о проценте ложных срабатываний и о плотности ошибок (сколько ошибок PVS-Studio выявляет на 1000 строк кода)…
Как PVS-Studio оказался внимательнее, чем три с половиной программиста

Дата: 22 Окт 2018

Автор: Андрей Карпов

PVS-Studio, как и другие статические анализаторы кода, часто выдаёт ложные срабатывания. Но не стоит спешить считать странные срабатывания ложными. Это короткая история о том, как PVS-Studio вновь ок…
PVS-Studio для Java

Дата: 17 Янв 2019

Автор: Андрей Карпов

В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальн…

Комментарии (0)

Следующие комментарии
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять