Protocol Buffers е много популярен, готин и висококачествен продукт, който е разработен предимно от Google. Това е добро предизвикателство за анализатора на статичен код PVS-Studio. Намирането на поне нещо вече е постижение. Нека да опитаме.
Пиша за Протоколни буфери (protobuf) като част от дългосрочна поредица от статии за проверка на проекти с отворен код. Библиотеката прилага протокол за сериализация на структурирани данни. Това е ефективна двоична алтернатива на 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] Възможно препълване. Помислете за кастинг на операнди, а не на резултата. 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] Използва се потенциално нулев интелигентен указател „file_generator“. java_kotlin_generator.cc 100
Ако променливата generate_immutable_code е равна на false, тогава интелигентният указател на 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))Анализаторът не харесва този код по две причини:
- 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 в процеса на разработка и вижте какво може да намира всеки ден. Чудите се как можете да направите това, ако вашият е голям проект? Натисни тук".
