Мы используем куки, чтобы пользоваться сайтом
было удобно.
Весь год мы скакали по бескрайним полям открытого кода, расследуя преступления, отстреливая уязвимости и собирая трофеи. Но сегодня мы решили заглянуть в самый пыльный бар, где у стойки стоит опытный шериф и вспоминает десять самых лихих и опасных багов на диком западе.
В течение целого года мы сражались с разными ошибками из Open Source проектов на языках C и C++. Каждого мы ловили, допрашивали и записывали его проделки в наше досье. А теперь пришло время вспомнить самые громкие дела.
Сегодня я расскажу вам историю про 10 самых интересных ошибок, с которыми мы встретились в разных местах дикого запада. По каждому из этих мест у нас заведено отдельное досье — целая статья. А для наиболее увлечённых мы выбрали 5 самых популярных по C и C++ за этот год:
Полный список статей нашего блога можно на сайте по ссылке.
N10
Было однажды у нас дело о пропавшей лошади. Кобыла всегда была в стойле, но, когда мы хотели поехать в город, она как будто растворялась.
Предупреждение PVS-Studio: V674 The literal '0.5f' of 'float' type is being implicitly cast to 'unsigned char' type while calling the 'SetRenderColor' function. Inspect the second argument. grenade_bugbait.cpp 168
typedef unsigned char byte;
inline void CBaseEntity::SetRenderColor( byte r, byte g, byte b, byte a )
{
m_clrRender.Init( r, g, b, a );
}
void CGrenadeBugBait::BugBaitTouch( CBaseEntity *pOther )
{
....
if ( pSporeExplosion )
{
....
pSporeExplosion->SetRenderColor( 0.0f, 0.5f, 0.25f, 0.15f ); // <=
....
}
....
}Функция SetRenderColor устанавливает значение цвета по RGBA, где каждый параметр имеет тип unsigned char с возможным диапазоном значений [0 .. 255]. При попытке передать аргументы с типом float дробная часть будет отброшена. Это приведёт к тому, что параметры функции r, g, b, a будут иметь значения, равные 0.
К сожалению, в репозитории отсутствует информация по blame, поэтому у меня есть два предположения, как ошибка появилась в кодовой базе:
SetRenderColor обрабатывает вещественные числа, и задал их таким способом.Дополнительно несколько аналогичных предупреждений:
Этот баг был обнаружен в проекте Source SDK, а полную версию статьи можно найти по ссылке.
N9
Когда-то пришлось скакать в прерии по сотне одинаковых троп, и случилось, что свернул на ту, по которой уже проезжал. Вот только вела она уже в другое место.
Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2903, 3053. erl_bif_info.c 2903
BIF_RETTYPE system_info_1(BIF_ALIST_1)
{
....
if (is_tuple(BIF_ARG_1)) { // L2778
....
} else if (BIF_ARG_1 == am_scheduler_id) { // L2782
....
}
....
else if (BIF_ARG_1 == am_garbage_collection) { // L2903
....
} else if (BIF_ARG_1 == am_fullsweep_after) { // L2921
....
}
else if (BIF_ARG_1 == am_garbage_collection) { // L3053
....
} else if (BIF_ARG_1 == am_instruction_counts) { // L3056
....
}
....
else if (ERTS_IS_ATOM_STR("halt_flush_timeout", BIF_ARG_1)) { // L3552
....
}
}Анализатор обнаружил в функции, содержащей огромное количество if-else if (~800 строк кода), несколько веток с одинаковыми проверками. При этом логика в них отличается: первая проверка, вторая проверка. С учётом числа веток в этой функции и дистанции между одинаковыми проверками (150 строк) неудивительно, что такое могло произойти. Предотвратить такие случаи как раз и помогает статический анализ.
Этот баг был обнаружен в проекте Erlang, а полную версию статьи можно найти по ссылке.
N8
Знал я одного шерифа, у которого был единственный вердикт на все преступления — свободен. Странный он был.
Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the subsequent code fragment. cmComputeLinkInformation.cxx 1748
bool cmComputeLinkInformation::CheckImplicitDirItem(LinkEntry const& entry)
{
BT<std::string> const& item = entry.Item;
// We only switch to a pathless item if the link type may be
// enforced. Fortunately only platforms that support link types
// seem to have magic per-architecture implicit link directories.
if (!this->LinkTypeEnabled) {
return false;
}
// Check if this item is in an implicit link directory.
std::string dir = cmSystemTools::GetFilenamePath(item.Value);
if (!cm::contains(this->ImplicitLinkDirs, dir)) {
// Only libraries in implicit link directories are converted to
// pathless items.
return false;
}
// Only apply the policy below if the library file is one that can
// be found by the linker.
std::string file = cmSystemTools::GetFilenameName(item.Value);
if (!this->ExtractAnyLibraryName.find(file)) {
return false;
}
return false;
}Анализатор намекает, что с функцией CheckImplicitDirItem что-то определённо не так:
then-ветка последней конструкции if совпадает с нижележащим кодом (return false;);false;AddFullItem никогда не приведёт к раннему возврату;return false;, т.к. это не изменит наблюдаемого поведения программы.Отметим, что функция написана с помощью паттерна "ранний возврат". Если кто не знает, этот способ помогает сократить уровень вложенности кода. При его применении в конце функции располагают наиболее "положительный" результат. Остальной код в случае расхождения с целью функции должен завершить её как можно раньше.
В нашем примере можно предположить, что наиболее "положительный" результат функции — это прохождение объектом типа LinkEntry всех необходимых проверок с возвратом значения true.
Вариант исправленного кода:
....
std::string file = cmSystemTools::GetFilenameName(item.Value);
if (!this->ExtractAnyLibraryName.find(file)) {
return false;
}
return true;Этот баг был обнаружен в проекте CMake, а полную версию статьи можно найти по ссылке.
N7
Однажды видел в одной деревне игрока, что в отчаянии поставил всё своё золото на карту, которой даже не было в колоде. Судья просто развёл руками, а золото так и осталось лежать на столе.
Предупреждение PVS-Studio:
V783 Dereferencing of the invalid iterator 'shades.end()' might take place. ColorHelper.cpp 194
winrt::Windows::UI::Color ColorHelper::GetAccentColor(
const winrt::Windows::UI::Color& color
)
{
....
auto shades = std::map<float, HSL>();
....
// 3f is quite nice if the whole non-client area is painted
constexpr auto readability = 1.75f;
for (auto shade : shades)
{
if (shade.first >= readability)
{
return HslToRgb(shade.second);
}
}
return HslToRgb(shades.end()->second); // <=
}Возможно ли, что ни один оттенок не будет соответствовать критерию читабельности? Точно сказать нельзя, но такая ситуация вполне вероятна. Здесь отлично читается неопределённое поведение, и психобумагу предъявлять не надо для убедительности, потому что разыменование std::map::end() приводит именно к нему, так как этот итератор указывает на элемент следующий за последним в std::map.
Этот баг был обнаружен в проекте Windows Terminal, а полную версию статьи можно найти по ссылке.
N6
Заключил как-то раз договор с попутчиком разделить найденный клад. Тот кивнул, развернулся и растворился, как мираж. С тех пор его больше никто не видел.
Предупреждение PVS-Studio: V758 The 'graph' reference becomes invalid when smart pointer returned by a function is destroyed. utils.cpp 391
template<typename T>
struct Ptr : public std::shared_ptr<T>;
// ....
Ptr<FlannNeighborhoodGraph> FlannNeighborhoodGraph::create(
const Mat &points, int points_size,
int k_nearest_neighbors_, bool get_distances,
int flann_search_params_, int num_kd_trees)
{
return makePtr<FlannNeighborhoodGraphImpl>(points, points_size,
k_nearest_neighbors_, get_distances,
flann_search_params_, num_kd_trees);
}
void Utils::densitySort (const Mat &points, int knn,
Mat &sorted_points, std::vector<int> &sorted_mask)
{
// ....
// get neighbors
FlannNeighborhoodGraph &graph = // <=
*FlannNeighborhoodGraph::create(points, points_size, knn,
true /*get distances */, 6, 1);
std::vector<double> sum_knn_distances (points_size, 0);
for (int p = 0; p < points_size; p++) {
const std::vector<double> &dists = graph.getNeighborsDistances(p);
for (int k = 0; k < knn; k++)
sum_knn_distances[p] += dists[k];
}
// ....
}template<typename T>
struct Ptr : public std::shared_ptr<T>
{
inline Ptr(const std::shared_ptr<T>& o)
CV_NOEXCEPT : std::shared_ptr<T>(o) {}
inline Ptr(std::shared_ptr<T>&& o)
CV_NOEXCEPT : std::shared_ptr<T>(std::move(o)) {}
typename std::add_lvalue_reference<T>::type operator*() const
CV_NOEXCEPT { return *std::shared_ptr<T>::get(); }
// ....
}
template<typename _Tp, typename ... A1> static inline
Ptr<_Tp> makePtr(const A1&... a1)
{
static_assert( !has_custom_delete<_Tp>::value,
"Can't use this makePtr with custom DefaultDeleter");
return (Ptr<_Tp>)std::make_shared<_Tp>(a1...);
}Если вы думаете, что использование умных указателей раз и навсегда решает проблему "висячих" ссылок и доступов к памяти, то здесь всё пошло не так. Давайте разбираться. Сейчас код работает следующим образом:
create создаёт и возвращает умный указатель на тип FlannNeighborhoodGraphImpl, и его счётчик ссылок на объект равен единице;graph на значение этого умного указателя, при этом счётчик ссылок на объект не изменяется;for происходит обращение к невалидной ссылке.В итоге код, который казался правильным, приводит к неопределённому поведению. Кроме того, эту проблему находит не только PVS-Studio, но и санитайзер. Пруф.
Чтобы исправить проблему, необходимо сохранить умный указатель, тогда объект типа FlannNeighborhoodGraph будет жить до конца блока. Например, можно сделать так:
std::vector<double> sum_knn_distances (points_size, 0);
{
// get neighbors
auto graph = FlannNeighborhoodGraph::create(points, points_size, knn,
true /*get distances */, 6, 1);
for (int p = 0; p < points_size; p++) {
const std::vector<double> &dists = graph->getNeighborsDistances(p);
for (int k = 0; k < knn; k++)
sum_knn_distances[p] += dists[k];
}
}Мы дополнительно ограничили область видимости graph, чтобы ресурс освободился после выполнения циклов.
Этот баг был обнаружен в проекте OpenCV, а полную версию статьи можно найти по ссылке.
N5
Однажды местный знаток начертил карту переправы, но у него кончился уголь. Поэтому самый опасный участок, начерченный остатками угля, смыло первым же дождём. В том месте все и пропадали.
Предупреждения PVS-Studio:
V629 Consider inspecting the '1 << (brake->type + 1)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. phpdbg_bp.c 1209
V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. phpdbg_bp.c 1209
uint64_t flags
....
PHPDBG_API void phpdbg_delete_breakpoint(zend_ulong num)
{
....
if ((brake = phpdbg_find_breakbase_ex(num, &table, &numkey, &strkey))) {
int type = brake->type;
char *name = NULL;
size_t name_len = 0L;
switch (type) {
....
default: {
if (zend_hash_num_elements(table) == 1) {
PHPDBG_G(flags) &= ~(1<<(brake->type+1)); // <=
}
}
}
....
}
}Математики, не расслабляемся. Переменная flags имеет тип unsigned long int, в то время как brake->type имеет тип int. Код призван снять определённый бит из flags. А теперь следим за руками:
1 типа int сдвигают влево на какое-то число бит. Тип int чаще всего 32-битный. Очень надеемся, что сдвиг не производится на 32 и более битов, иначе поведение не определено.int.flags. Потеря старших значимых битов во flags будет происходить, когда правый операнд положительный. А им он будет только когда происходит сдвиг на 31 бит влево, т.е. когда нужно сбросить 31-й бит во flags. Пруф.Заметили, сколько всего нужно держать в голове для такого безобидного выражения? А проблема кроется в разных размерах операндов и различной знаковости некоторых подвыражений. А надо всего лишь поменять у константы 1 тип с int до unsigned long long, и всё будет работать, как и задумывалось:
PHPDBG_G(flags) &= ~( 1uLL <<(brake->type+1));Этот баг был обнаружен в проекте PHP, а полную версию статьи можно найти по ссылке.
N4
Как-то раз я увидел, как молодой ковбой гнался за бандитом. Он загнал его в тупик в одном из местных баров, но выстрелил не в преступника, а в своё отражение в пыльном зеркале, разбив его вдребезги.
Предупреждение PVS-Studio: V794 The assignment operator should be protected from the case of 'this == &other'. fs_path.cpp 36
FsPath& FsPath::operator=(FsPath&& other)
{
m_path = std::move(other.m_path);
other.m_path.clear();
return *this;
}В представленном фрагменте реализован оператор перемещения присваивания класса FsPath, в котором происходит перенос данных из объекта other в текущий экземпляр. Следует отметить, что отсутствует проверка на самоприсваивание (проверка вида (this == &other)), что может привести к нежелательным последствиям.
Если объект пытаются присвоить самому себе, то операция m_path = std::move(other.m_path); перемещает содержимое other.m_path в m_path, а последующий вызов other.m_path.clear(); очищает данные. В результате m_path оказывается в неожиданном состоянии, и остаётся лишь пожелать удачной отладки :)
Чтобы устранить этот риск, рекомендую добавить в начало оператора следующую проверку:
if (this == std::addressof(other))
{
return *this;
}Использование std::addressof вместо оператора & гарантирует корректное сравнение адресов даже при перегрузке оператора & в классе.
Этот баг был обнаружен в проекте Nau Engine, а полную версию статьи можно найти по ссылке.
N3
Однажды видал я, как шаман пытался вызвать духа, не дойдя до священного места. Дух пришёл, но совсем другой — койот из ближайшего оврага.
Предупреждение PVS-Studio: V1099 sing the 'window_id' function of uninitialized derived class while initializing the 'modal_dialog' base class will lead to undefined behavior. install_dependencies.hpp 29
class install_dependencies : public modal_dialog
{
public:
explicit install_dependencies(const addons_list& addons)
: modal_dialog(window_id()), addons_(addons) // <=
{}
....
private:
virtual const std::string& window_id() const override;
....
}Благодаря этому фрагменту кода я расскажу вам чуточку больше о неопределённом поведении.
Как мы видим выше, класс install_dependencies является производным от класса modal_dialog. В конструкторе install_dependencies происходит инициализация родительского класса значением, возвращаемым (внимание!) нестатической функцией window_id. То есть произойдёт следующее:
install_dependencies::window_id;modal_dialog;addons_;install_dependencies.А это значит, что произойдёт вызов функции у объекта класса, который ещё не был инициализирован! А это, в свою очередь, нарушает правило стандарта, согласно которому:
Member functions (including virtual member functions, [class.virtual]) can be called for an object under construction
Similarly, an object under construction can be the operand of the typeid operator ([expr.typeid]) or of a dynamic_cast ([expr.dynamic.cast]).
However, if these operations are performed in a ctor-initializer (or in a function called directly or indirectly from a ctor-initializer) before all the mem-initializers for base classes have completed, the program has undefined behavior.
И это ещё не всё. Как вы могли заметить, функция-член window_id является виртуальной и переопределена в классе install_dependencies. Проблемы могут появиться в дальнейшем, когда программист напишет производный от него класс, в котором будет переопределён window_id.
В момент создания объекта этого производного класса, когда будет отрабатывать конструктор installed_dependencies, информации о существовании нового переопределения ещё нет. Поэтому в списке инициализации всегда будет вызываться именно функция installed_dependencies::window_id. Это может отличаться от того, что задумывал изначально программист. Подробнее об этом можно почитать здесь.
Этот баг был обнаружен в проекте Wesnoth, а полную версию статьи можно найти по ссылке.
N2
Знал я одного ковбоя, который выстрелил в тень на стене, приняв её за затаившегося врага. Выстрел грохнул, штукатурка осыпалась, осталась только дыра в стене. Цели и не существовало, а восстанавливать пришлось много.
Предупреждение PVS-Studio: V575 The null pointer is passed into 'fseek' function. Inspect the first argument. vid_ati_eeprom.c 61
void
ati_eeprom_load_mach8(ati_eeprom_t *eeprom, char *fn, int mca)
{
FILE *fp;
....
fp = nvr_fopen(eeprom->fn, "rb");
size = 128;
if (!fp) {
if (mca) {
(void) fseek(fp, 2L, SEEK_SET); // <=
memset(eeprom->data + 2, 0xff, size - 2);
fp = nvr_fopen(eeprom->fn, "wb");
fwrite(eeprom->data, 1, size, fp);
....
}Нам надо загрузить данные, сохранённые в NVRAM видеоадаптера, и храним мы их в файле в двоичном виде. Если файла нет, то надо создать в нём "начальные" данные. Вот как раз сценарий, когда файла нет. Мы смещаем указатель на файл, а он нулевой, и получаем разыменование нулевого указателя fp как из палаты мер и весов.
Взглянем подробнее на fseek. Стандарт C11 не определяет требования к первому параметру функции и, соответственно, не гарантирует проверку на NULL. Это значит, что его обработка остаётся на совести разработчиков стандартной библиотеки. В студию приглашаются:
Последние две реализации стандартной библиотеки языка C здесь в качестве гостей: 86Box не рассчитан на использование с ними или их совместимость не проверялась. В инструкции по сборке альтернативные реализации тоже не упомянуты. Поэтому начнём с ожидаемых к использованию стандартных библиотек и попросим их повторить те же самые действия над нулевым указателем на файл.
Подаём напряжение
Достаём с воображаемого стеллажа IBM PS/2 model 55SX и "вставляем" в него 2D-ускоритель IBM 8514/A в исполнении ATI.
Первым испытуемым станет собранный с помощью MinGW экземпляр для Windows. Убеждаемся в отсутствии файла NVRAM перед стартом: для этого нужно проверить папку %userprofile%\86Box VMs\<имя виртуальной машины>\nvr на наличие файла ati8514_mca.nvr. Если есть, то удаляем.
Включаем наш агрегат, и...
Ничего не взорвалось! Всё хорошо, файл NVRAM записан, компьютер работает и smoke-тест на glibc окончен. Дефект не обнаружен.
Переходим к FreeBSD. Стандартную библиотеку языка C в этой операционной системе реализует libc. В принципе, это справедливо для всех операционных систем семейства BSD.
Конфигурацию используем ту же самую. Отсутствие файла NVRAM ati8514_mca.nvr проверяем по пути ~/.local/share/86Box/Virtual Machines/<имя виртуальной машины>/nvr. Три, два, один, включаем...
Ну, лучше эту ситуацию опишет только произошедшее давным-давно у Макса Крюкова :)
Открываем зажмуренные после взрыва глаза и смотрим в консоль: у нас подтверждён ненормальный выход!
void VMManagerSystem::launchMainProcess() Full Command:
"/root/86Box/build_freebsd/src/86Box"
("--vmpath", "/root/.local/share/86Box/Virtual Machines/somevm",
"--vmname",
"somevm")
Connection received on 86Box.socket.5876c5
Connection disconnected
Abnormal program termination while launching main process:
exit code 11, exit status QProcess::CrashExitРядом с исполняемым файлом эмулятора появился дамп ядра. Приглашаем в студию LLDB:
root@freebsd:~/86Box/build_freebsd/src # lldb 86Box -c 86Box.core
(lldb) target create "86Box" --core "86Box.core"
Core file '/root/86Box/build_freebsd/src/86Box.core' (x86_64) was loaded.
(lldb) bt
* thread #1, name = '86Box', stop reason = signal SIGSEGV
* frame #0: 0x0000000832f880bf
libc.so.7`_flockfile(fp=0x0000000000000000)
at _flock_stub.c:65:20
frame #1: 0x0000000832f8b675
libc.so.7`fseek(fp=0x0000000000000000, offset=2, whence=0)
at fseek.c:62:2
frame #2: 0x00000000018cd964
86Box`ati_eeprom_load_mach8(eeprom=...., fn=<unavailable>, mca=1)
at vid_ati_eeprom.c:61:20Нулевой указатель fp устраивает светошумовое представление: не получается заблокировать файл, потому что нет действительного файлового дескриптора. К сожалению, LLDB очень не хотел работать в реальном времени и падал то с тихим lost connection, то с грохотом и спецэффектами. Поэтому хождения по коду, как в Windows, я показать не смогу.
Этот баг был обнаружен в проекте 86Box, а полную версию статьи можно найти по ссылке.
N1
Знакомый шериф из соседней деревни однажды в протокол допроса записал, что свидетель подтвердил показания самого себя. Суд так и не понял, было ли это ошибкой или хитрым приёмом защиты.
Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: PeekArg.getValNo() == PeekArg.getValNo() PPCISelLowering.cpp 7865
SDValue PPCTargetLowering::LowerCall_AIX(....) const {
....
for (unsigned I = 0, E = ArgLocs.size(); I != E;) {
....
CCValAssign &GPR1 = VA;
....
if (I != E) {
// If only 1 GPR was available, there will only be one custom GPR and
// the argument will also pass in memory.
CCValAssign &PeekArg = ArgLocs[I];
if (PeekArg.isRegLoc() && PeekArg.getValNo() == PeekArg.getValNo()) // <=
{
assert(PeekArg.needsCustom() && "A second custom GPR is expected.");
CCValAssign &GPR2 = ArgLocs[I++];
RegsToPass.push_back(std::make_pair(GPR2.getLocReg(),
DAG.getZExtOrTrunc(ArgAsInt, dl, MVT::i32)));
}
}
....
}Предварительный диагноз — ещё одна жертва копипасты. Сразу же проверяем getValNo на наличие побочных эффектов:
class CCValAssign{
....
unsigned ValNo;
unsigned getValNo() const { return ValNo; }
}Что ж, ничего необычного. Посмотрим код до последнего затронувшего его коммита:
CCValAssign &GPR1 = VA;
....
assert(I != E && "A second custom GPR is expected!");
CCValAssign &GPR2 = ArgLocs[I++];
assert(GPR2.isRegLoc() && GPR2.getValNo() == GPR1.getValNo() &&
GPR2.needsCustom() && "A second custom GPR is expected!");
RegsToPass.push_back(
std::make_pair(GPR2.getLocReg(),
DAG.getZExtOrTrunc(ArgAsInt, dl, MVT::i32)));Суть понятна: исключительный случай, ранее прикрывавшийся assert, редизайнился в рядовое ветвление. Об этом говорит и текст коммита:
This patch implements the caller side of placing function call arguments
in stack memory. This removes the current limitation where LLVM on AIX
will report fatal error when arguments can't be contained in registers.
Можно заметить, что кроме найденной ошибки есть ещё странное присваивание:
CCValAssign &PeekArg = ArgLocs[I];
....
CCValAssign &GPR2 = ArgLocs[I++]; // здесь PeekArg == GPR2Скорее всего, автор хотел написать вот так:
if (I != E) {
CCValAssign &GPR2 = ArgLocs[I];
if (GPR2.isRegLoc() && PeekArg.getValNo() == GPR1.getValNo())
{
assert(PeekArg.needsCustom() && "A second custom GPR is expected.");
I++;
RegsToPass.push_back(std::make_pair(
GPR2.getLocReg(), DAG.getZExtOrTrunc(ArgAsInt, dl, MVT::i32)));
}
}Но для большей выразительности разделил GPR2 на PeekArg, дабы показать, что в отличие от безусловного прежнего кода нужно сперва "подсмотреть" аргумент. А в процессе копипасты потерял GPR1 в условии.
Думаю, корректная версия if должна выглядеть так:
if (PeekArg.isRegLoc() && PeekArg.getValNo() == GPR1.getValNo())Что интересно, до недавнего полного переезда на GitHub у LLVM был отдельный сайт для код-ревью, и в коммите есть ссылка на него. Там можно заметить, что ручное ревью не всегда спасает:
Этот баг был обнаружен в проекте LLVM, а полную версию статьи можно найти по ссылке.
В баре воцарилась тишина, нарушаемая лишь скрипом входной двери и потрескиванием поленьев в камине. Десять самых лихих багов 2025 года — теперь лишь истории, которые передают из уст в уста.
На этом диком западе без надёжного напарника делать нечего, поэтому моим верным помощником в каждом деле был анализатор PVS-Studio. Он готов помочь и вам найти опасные участки кода:
А если ваш путь проходит и по другим территориям, то предлагаю послушать истории про баги из проектов, написанных на Java и C#:
0
0
0
45
0
0