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

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

![0882_Protocol_Buffers_vs_PVS_Studio_ru/image1.png](https://import.viva64.com/docx/blog/0882_Protocol_Buffers_vs_PVS_Studio_ru/image1.png)

Продолжая наш многолетний [цикл публикаций](https://pvs-studio.ru/ru/blog/inspections/) про проверку открытых проектов, я обратил внимание на проект [Protocol Buffers](https://github.com/protocolbuffers/protobuf) \(protobuf\)\. Библиотека реализует протокол сериализации \(передачи\) структурированных данных\. Это эффективная бинарная альтернатива текстовому формату XML\.

Проект показался мне интересным вызовом для анализатора PVS\-Studio, ведь компания Google весьма основательно подходит к качеству разрабатываемого C\+\+ кода\. Взять хотя бы документ "[Безопасное использование C\+\+](https://habr.com/ru/company/pvs-studio/blog/580762/)", который недавно активно [обсуждался](https://old.reddit.com/r/cpp/comments/pkru4h/safer_usage_of_c_in_chrome/)\. Дополнительно protobuf используется большим количеством разработчиков в своих проектах и потому хорошо ими протестирован\. Найти хотя бы пару ошибок в этом проекте является достижением, которое будет лестно нашей команде\. Так чего мы ждём, вперёд\!

![0882_Protocol_Buffers_vs_PVS_Studio_ru/image2.png](https://import.viva64.com/docx/blog/0882_Protocol_Buffers_vs_PVS_Studio_ru/image2.png)

Раньше мы никогда специально не проверяли этот проект\. Однажды, три года назад, мы заглянули в него, когда писали цикл статей про проверку проекта Chromium\. Мы заметили интересную ошибку в функции проверки даты, которую описали в отдельной заметке "[31 февраля](https://pvs-studio.ru/ru/blog/posts/cpp/0550/)"\.

Признаюсь, у меня была задумка, когда я писал эту статью\. Я хотел показать возможности нового [механизма межмодульного анализа С\+\+ проектов](https://pvs-studio.ru/ru/blog/posts/cpp/0851/)\. К сожалению, в этот раз межмодульный анализ ничего нового не привнёс\. Что с ним, что без него — находятся одни и те же интересные места\. Впрочем, это неудивительно\. В этом проекте вообще сложно что\-то найти :\)\.

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

## Copy\-Paste

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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\-ветке планировалось написать:

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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\. Функция просто завершит работу и вернёт статус ошибки\.

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v1028/) \[CWE\-190\] Possible overflow\. Consider casting operands, not the result\. generated\_message\_reflection\.h 140

Складываются два значение типа _int_ и помещаются в переменную типа _size\_t_:

```cpp
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_\. Так что написанный код ничем не отличается от варианта:

```cpp
size_t offset = int_var_1 + int_var_2;
```

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

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_ всегда истинна, раз эта ошибка ещё не обнаружена\. Её можно счесть несущественной\. Как только в процессе редактирования кода логика работы изменится, то произойдёт разыменование нулевого указателя, что будет сразу замечено и исправлено\. А с другой стороны, в этом коде находится мина, которую лучше найти и исправить заранее, чем ждать, когда кто\-то на ней подорвётся в будущем\. Суть статического анализа как раз том, чтобы найти ошибки заранее\.

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

```cpp
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;
  ....
}
```

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

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

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

* [V634](https://pvs-studio.ru/ru/docs/warnings/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](https://pvs-studio.ru/ru/docs/warnings/v592/) The expression was enclosed by parentheses twice: \(\(expression\)\)\. One pair of parentheses is unnecessary or misprint is present\. strutil\.cc 1408

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

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

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

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

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

## Заключение

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

![0882_Protocol_Buffers_vs_PVS_Studio_ru/image3.png](https://import.viva64.com/docx/blog/0882_Protocol_Buffers_vs_PVS_Studio_ru/image3.png)

Следует помнить, что пользу статический анализатор приносит при [регулярном использовании](https://pvs-studio.ru/ru/blog/posts/cpp/0639/) для проверки свежего кода, а не при разовых проверках уже оттестированных проектов\.

Тем не менее, нужно с чего\-то начать\. Поэтому предлагаю [скачать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/try-free/), проверить проект и взглянуть на самые [лучшие предупреждения](https://pvs-studio.ru/ru/docs/manual/6532/)\. Скорее всего, вы увидите много для себя интересного :\)\.

Если же ваш код столь же качественный, как protobuf, то предлагаем сразу перейти к полноценному сценарию использования инструмента\. Попробуйте внедрить PVS\-Studio в процесс разработки и посмотреть, что интересного будет находиться каждый день\. Как это сделать в случае, если у вас большой проект, описано [здесь](https://pvs-studio.ru/ru/blog/posts/0743/)\.