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

05 Ноя 2021

Теги:

#Cpp

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

05 Ноя 2021

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

Copy-Paste
Утечка файлового дескриптора
Потенциальное переполнение
Разыменование нулевого указателя
Там ли скобка?
Заключение

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

#Cpp