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

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
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-c5a344003d477e9d8b166e99e4119de0.png

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

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

0882_Protocol_Buffers_vs_PVS_Studio_ru/image2-76cba80663c9395421fb8286ba50929f.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-297a40e313ac47cd499ca876b6c7efe7.png

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

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

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

Популярные статьи по теме
PVS-Studio для проверки лабораторных работ на C и C++

Дата: 06 Июл 2022

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

Встретил очередной вопрос на Stack Overflow от человека, изучающего язык C++. Количество подобных вопросов можно сократить, используя PVS-Studio. Человек сразу может получить ответ, не отвлекая други…
Игра: найди ошибку в C++ коде

Дата: 29 Июн 2022

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

Авторы анализатора PVS-Studio предлагают вам проверить свою внимательность и развлечься. Попробуйте быстро отыскать баг в фрагменте исходного кода и ткнуть в него мышкой.
Тем, кто учится программировать и решил написать вопрос на Stack Overflow: "Почему код не работает?"

Дата: 28 Июн 2022

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

На сайте Stack Overflow много вопросов от людей, ещё только изучающих языки программирования. Лайфхак: ответы на многие эти вопросы можно получить сразу, запустив анализатор кода. Получится быстрее.
В мире антропоморфных животных: PVS-Studio проверил Overgrowth

Дата: 23 Июн 2022

Автор: Владислав Столяров

Недавно в сети появилась новость о том, что был открыт исходный код игры Overgrowth. Мы не смогли пройти мимо и проверили его качество с помощью PVS-Studio. Давайте же вместе посмотрим, где больше ин…
Как написать рефлексию для C++

Дата: 21 Июн 2022

Автор: Гость

C++ поистине противоречивый язык. Старый добрый С существует аж с 1972 года, С++ появился в 1985 и сохранил с ним обратную совместимость. За это время его не раз хоронили: сперва Java, теперь его пот…

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

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