Protocol Buffers — очень популярный, крутой и качественный продукт, который в основном разрабатывается Google. Это хороший вызов для статического анализатора кода PVS-Studio. Найти хоть что-то — уже достижение. Давайте попробуем.
Пишу о Protocol Buffers (protobuf) в рамках многолетней серии статей о проверке open-source проектов. Библиотека реализует протокол сериализации структурированных данных. Это эффективная бинарная альтернатива текстовому формату XML.
Проект показался интригующим вызовом для анализатора PVS-Studio, поскольку Google очень серьезно подходит к качеству производимого кода на C++. Возьмем, к примеру, активно обсуждаемый в последнее время документ Безопасное использование C++. Кроме того, многие разработчики используют protobuf в своих проектах, а это означает, что продукт protobuf хорошо протестирован. Найти хотя бы пару ошибок в этом проекте — вызов, который мы взяли на себя. Так чего же мы ждем? Пришло время узнать, на что способен PVS-Studio!
Мы никогда раньше специально не проверяли этот проект. Когда-то, года три назад, мы его рассматривали, когда писали цикл статей о проверке Chromium. Мы нашли интересную ошибку в функции проверки данных и описали ее в отдельной статье — 31 февраля.
Честно говоря, когда я писал свою статью в этот раз, у меня был определенный план. Я хотел продемонстрировать новую возможность анализатора — механизм межмодульного анализа C++ проектов — и что он умеет. К сожалению, на этот раз межмодульный анализ не дал новых интересных результатов. С ним или без — было все равно, никаких новых интересных триггеров анализатора в коде. Хотя в этом не было ничего удивительного. В этом проекте вообще сложно что-то найти :).
Итак, давайте посмотрим, какие ошибки ускользнули от внимания разработчиков и вспомогательных инструментов.
Копировать вставить
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] Переменной дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 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] Произошел выход из функции без отпускания ручки ручка. Возможна утечка ресурсов. io_win32.cc 400
Перед выходом из функции вызов метода FindClose(handle) должен закрыть файловый дескриптор handle. Однако этого не происходит, если текст в кодировке UTF-8 не может быть преобразован в 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] Возможно переполнение. Рассмотрим приведение операндов, а не результата. сгенерированный_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] Используется потенциально нулевой смарт-указатель ‘file_generator’. java_kotlin_generator.cc 100
Если переменная generate_immutable_code равна false, то интеллектуальный указатель file_generator остается равным nullptr. Следовательно, нулевой указатель будет разыменован.
По-видимому, переменная generate_immutable_code всегда имеет значение true — иначе ошибка была бы уже обнаружена. Его можно назвать незначительным. Как только кто-то отредактирует код и его логику, нулевой указатель будет разыменован, кто-то заметит и устранит проблему. С другой стороны, этот код содержит, так сказать, мину. И лучше найти его пораньше, чем сидеть и ждать, пока кто-нибудь взорвется на нем в будущем. Цель статического анализа — найти ошибки до того, как они станут опасными.
Скобка стоит на своем месте?
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))Анализатору не нравится этот код по двум причинам:
- V634 [CWE-783] Приоритет операции * выше, чем у операции ‹‹. Возможно, в выражении следует использовать скобки. strutil.cc 1408
- V592 Выражение было дважды заключено в круглые скобки: ((выражение)). Одна пара скобок лишняя или присутствует опечатка. strutil.cc 1408
Вы, вероятно, согласитесь, что эти предупреждения дополняют друг друга. Операторы сдвига и умножения используются вместе. Легко забыть, какой из них имеет более высокий приоритет. А повторяющиеся скобки намекают на то, что автор знал о двусмысленности и хотел ее избежать. Но это не сработало.
Есть два способа понять этот код. Версия первая: код правильный. В этом случае дополнительные скобки только облегчают чтение кода и ни на что не влияют:
uint64 mask = (static_cast<uint64>(1) << ((width - 1) * 4)) | value;Вариант второй: выражение содержит ошибку. Если это так, то дополнительные скобки должны изменить порядок выполняемых операций:
uint64 mask = ((static_cast<uint64>(1) << (width - 1)) * 4) | value;Заключение
Приятно уметь находить недостатки в таком известном и качественном продукте, как protobuf. С другой стороны, наверное, не лучшая идея использовать protobuf для демонстрации возможностей статического анализа кода :). Трудно хвастаться возможностями инструмента, если инструмент может найти только пару ошибок :).
Напомню, что статический анализатор наиболее полезен при регулярном использовании для проверки нового кода — а не для разовых проверок уже протестированных проектов.
Однако нужно с чего-то начинать. Так что рекомендую скачать PVS-Studio, проверить свой проект и заглянуть в Лучшие предупреждения. Скорее всего, вы увидите много вещей, требующих вашего внимания :).
Если у вас самый качественный код — как у protobuf — рекомендую начать использовать анализатор по назначению. Попробуйте интегрировать PVS-Studio в процесс разработки и посмотрите, что он может найти каждый день. Хотите знать, как вы можете сделать это, если у вас большой проект? Кликните сюда".
