Ошибки программистов C++ — это отдельный вид искусства, вроде бы простой язык, но стоит отвлечься на чашечку кофе, как компилятор начинает вываливать простыню ворнингов пополам с ошибками, и иногда это больше похоже на древнеегипетские письмена, чем на нормальный выхлоп. Вы наверное и сами не раз сталкивались с разыменованием nullptr или перепутали (= и ==) по недосмотру. Часто причиной ошибкой является лень или невнимательность, или усталость - не зря появились суеверия "не комитить в пятницу вечером", "не кодить в состоянии изменного сознания" или "избегать кода под кофейным угаром", ну это когда три-четыре кружечки кофе навернул и пошел нести добрый код направо и налево.

Вообще статья планировалась про то как можно поиздеваться над switch оператором - была у меня запись разговора с одного питерского митапа, проходившего несколько лет назад и где присутствующим предложили написать "невозможный", но работающий код со switch. И вот в поисках этой записи я наткнулся на файл с описанием "пятничных" багов и комитов, которые бы в трезвом уме посреди недели врядли бы проскочили в мастер. А еще была статья тоже про ошибки "Федя, дичь", где уважаемый хабрачитатель угадывал причину разных ошибок, явных или не очень. Поэтому я решил продолжить в том же стиле, вопрос - ответ, а вам предлагаю также поучаствовать в отгадывании возможных причин. Все примеры из реальных проектов, игр, движков и пятничных комитов.

C++ не прощает ошибок, но именно в этом его "шарм". В большинстве приведенных примеров сохранен стиль и названия параметров, только немного подчищены коментарии, дабы не палить контору.


0х10

Игнорирование правил разработки и изобретение велосипедов в большинстве случаев заканчиваются багом. Так и тут, движок иногда крашился в при отрисовке UI виджетов, но чаще просто портил другие виджеты, которые были полностью сделаны на POD типах.

std::vector<int> Widget::DrawElements(Element* p, int n) {
    std::vector<int> result;
    result.reserve(n);
    for (int i = 0; i < n; ++i) {
    	bool r = DrawElement(p[i]);
        result.push_back(r);
    }
    ...
    return result;
}

void Widget::Draw(int m) {
    constexpr int n = 10;
    Element a[n] = {}; // локальный буффер, связано с отложенным рендером виджетов
    // copy elements
    // ...
    DrawElements(a, m);   
    // ...
}
А где ошибка?

Здесь мы допустили небольшую ошибку в функции Widget::Draw, которая приводила к повреждению данных и реже сбою программы. Интерфейс в стиле «указатель, количество» не оставляет функции DrawElements() практически никакой возможности защититься от ошибок выхода за пределы массива, там лежал другой массив, который и принимал на себя удар. Если бы мы могли проверять индексы на выход за границы массива, ошибка была бы обнаружена гораздо раньше. А так ошибка прожила в коде примерно полгода, просто потому что у виджета обычно было меньше 10 элементов.

0x11

При портировании движка Dagor на платформу nintendo switch пришлось сделать прослойку для работы с файловой системой. После очередной пятницы игра стала крашиться через несколько загрузок в рандомных местах с ООМ (out of memory), а причина оказалось всего в одной строчке. Тут должно быть просто.

void OsFile::ropen(const NxString &name) {
    NxFile f = NxFileOpen(name, "r");
    // ...
    if (something) return;   
    // ...
    NxFileClose(f);
}
A где ошибка?

Строка 5 была добавлена в какой-то момент, после фикса бага неподдерживаемыми типами файлов. Но даже медленный рост потребления ресурсов со временем может исчерпать их доступность.

Утечка памяти — это любое выделение ресурса, которое не было освобождено. Например, если объект был выделен в куче, а указатель на этот объект был утерян, такие ресурсы уже не могут быть очищены. Так на консоли Nintendo switch одновременно можно открыть не больше 256 файлов на процесс, потом кончаются дескрипторы ядра и при открытии очередного файла игра падает по ООМ гдето в недрах musl, но бывало и так, что открытые файлы забирали всю доступную память и уже игра падала с честным ООМ, но уже в рандомном месте.

Это правило не следует понимать как обязательное требование освобождать все выделенные ресурсы при завершении работы программы. В некоторых случаях можно полагаться на автоматическую очистку ресурсов операционной системой, такую как закрытие файлов или освобождение памяти при завершении процесса

0x12

В отдел пришел новичок, ему дали мелкие баги для ознакомления с кодовой базой. А чтобы было не так скучно разрешили немного рефакторить код в дев ветке, дабы была история комитов и понимание как продвигается онбординг. Во вторник от QA прилетает претензия, что редактор стал на порядок медленее грузить уровни - мелкие еще нормально грузит, а большие минут по двадцать стал прям. Причем только у QA отдела, у всех остальных все норм. Тут надо немного пояснить, инструмент для тестирования которым пользовались, заодно и форматировал конфиги уровней перед апрувом. QA пострадали денек, и решили все таки написать программерам, которые по горячим следам нашли вот такое:

void lower(xstring &s) {
    for (int i = 0; i < strlen(s); ++i) {
    	s[i] = tolower(s[i]);
    }
}
А где ошибка?

А до фикса было вот так, тоже не супер, но никто не жаловался на скорость.:

for (int i = 0, size = (int)strlen(s); i < size; ++i) {
	s[i] = tolower(s[i]);
}

В условии используется выражение i < strlen(s). Это выражение будет вычисляться на каждой итерации цикла, что означает, что функция strlen должна проходить по строке каждый раз, чтобы определить её длину. Хотя содержимое строки меняется, предполагается, что tolower не повлияет на длину строки, поэтому лучше сохранить длину строки вне цикла, чтобы избежать лишних затрат на каждую итерацию. А теперь представьте что происходит, когда форматер пробует обработать 40мегабайтный конфиг уровня.

0х13

Отправили очередной патч для бокса, а он не проходит тесты. Майкрософт сначала просто морозились и реджектили билды, потом прислали комплайнс на забытые сенситив данные. Какие блин такие забытые?

Subject: Urgent Security Alert: Unused Encryption Keys Found in Memory
Hey Team,
Hope you’re all doing great! During our recent testing sessions, we came across a serious issue regarding some leftover encryption keys in memory tied to our Secure Layered Login (SLL) system. It looks like these keys were overlooked, and if we don’t deal with them quickly, they could potentially expose sensitive data to unauthorized access, approval pipeline will be moved to pre-stage policy after one week.

Было: CL125332
void XBL::UserLogin::requestKey() {
    char secretKeyPwa[MAX];
    // ...
    // something work
    // ...
    memset(secretKeyPwa, 0, sizeof buffer);
    ENSURE(secretKeyPwa[0] == 0);
}
<->
Cтало: СL127499
void XBL::UserLogin::requestKey() {
    char secretKeyPwa[MAX];
    // ...
    // something work
    // ...
    memset(secretKeyPwa, 0, sizeof buffer);
    assert(secretKeyPwa[0] == 0);
}
А где ошибка?

Тем, кто читал предыдущую статью, эта ошибка покажется знакомой. И, да, вам не кажется, компилятору пофиг на ваши сенситив данные. Макрос ENSURE() сравнивал значение из переменной с нулем, чем заставлял работать функцию memset. Макрос assert() ничего не делает в релизе, и компилятор также удалял вызов memset() и убирал логику очистки сенситив данных.

0x14

Дивное утро понедельника, от дизайнеров прилетает баг, что сущность на уровне перестала определять у себя наличие правой и левой руки в скелете, оттуда доставались кости и локаторы, чтобы цеплять руки к соотвествующим места на оружии. Или определяла, что руки есть но путала их местами из-за чего возникали различные графические глюки, вроде левой руки на прикладе и т.д.

using namespace std;

struct UnitComponent {
    string name;
    int number;

    UnitComponent(string nm, int num) : name(nm), number(num) {}
    virtual bool operator==(const UnitComponent& a) const {
        return name == a.name && number == a.number;
    }
    // ...
};

struct UnitHand : public UnitComponent {
    bool left_hand;

    UnitHand(string nm, int num, char ch) : UnitComponent(nm, num), left_hand(ch) {}
    virtual bool operator==(const UnitHand& a) const {
        return UnitComponent::operator==(a) && left_hand == a.left_hand;
    }
    // ...
};
А где ошибка?

Причина тоже банальная, коллеги порефакторили свой код и решили вместо явных сравнений признаков компонента написать операторы сравнения и перегрузить их в нужных классах. Да вот только забыли, чтобы с такой перегрузкой надо быть очень осторожным, иначе все рискуют сравнивать параметры базового класса вместо своих. Конечно, есть способы заставить
оператор сравнения работать по всей иерархии наследования, но такой наивный подходы будет работать с ошибками. Попробуйте поиграться с этим кодом в onlinegdb.

int main() {
    UnitComponent comp{ "test", 1 };
    UnitHand hand{ "test", 1, true };
    printf("comp == hand (%d)\n", comp == hand);    // compares name and number, ignores unit's left_hand
    UnitHand leg{ "test", 1, false };
    printf("hand == leg (%d)\n", hand == leg);   // compares name, number, and left_hand
    UnitComponent &hand_ref = hand;
    UnitComponent &leg_ref = leg;
    printf("hand_ref == leg_ref (%d)\n", hand_ref == leg_ref);  // ???

    return 0;
}

0x15

Бывает что ошибаются даже рокстары-рендерщики, которые в погоне за перфом, в пятницу вечером пишут всякое. А потом утром в понедельник в аврале ревертят стопицот изменений. В теории, пулл текстур должен был содержать пустые текстуры, которые бы оттуда забирались по мере создания объектов. Но что-то пошло не так и движок крашился уже на первом обращении к текстуре.

struct TextureBase {
    virtual void update() = 0;
    std::shared_ptr<int> sp;
};

struct TextureDX12 : public TextureBase {
    void update() override {}
};

void init(std::vector<TextureDX12> &a) {
    memset(a.data(), 0, sizeof(TextureDX12) * a.size());
}
А где ошибка?

Попытки обмануть механизм создания экземпляра типа обычно заканчиваются плачевно. Конструктор класа должен создавать полностью инициализированный объект и нет необходимости дополнительной инициализации. А использование memcpy/memset для изменения данных класса, который не является тривиально копируемым, ведет к неопределенному поведению. И это одна из наиболее частых ошибок при работе нетривиальными типами данных, которая приводит к memory corruption. Ошибка получилась, потому что один из коллег сделал новый класс текстур для DX12 и он изначально был без vtbl, а другая коллега выделила базовый класс и добавила виртуальности. Оба коммита формально прошли ревью без ошибок, но когда наложились друг на друга - получилась фигня.

0х16

Еще один пример невнимательности или случайное стечение обстоятельств, игре не хватало памяти на iPhone6, где пришлось урезать пул эффектов. В итоге на загруженных сценах получили графические артефакты в случайных местах. Но что забавно игра не крашилась, а порою эти баги и вообще не были замечены.

std::array<Particle, 2048> particles;
std::array<ParticleShader, 2048> shaders;

void updateEffects() {
    for (int i = 0, size = particles.size(); i < size; ++i) {
        //
        if (/* something */) ++i; 
        //
        ... some logic
    }
}
А где ошибка?

Управление циклом должно позволять правильно понимать что происходит внутри. Изменение счетчика цикла как в выражении итерации, так и внутри тела цикла приводило к выходу i за границы массива и порче памяти в shaders (в редких случаях), что соответственно приводило к графическим артефактам. На пк такой проблемы не было, потому что буфер был больше размером и никогда не заполнялся полностью. на мобилке же портились данные шейдера, который выглядел плохо, если отрисовывался. Но чаще просто пропускался апдейт некоторых эффектов.

0х17

Напортачить можно даже в трех строчках, особенно если код стал многопоточным, в некоторых случаях отдельный юнит на сцене не двигался. Исследование бага показало, что Mover (это компонент, который отвечал за навигацию юнита по сцене) не получал апдейты, потому его не было в пуле муверов. Найдете ошибку?

Mover MoverPool::acquire() {
    static int init = 0;
    if (!init) {
        init = 1;
        create_pool();
    }

    return create_mover();
}
Скрытый текст

Хотя переменная init работает идеально в однопоточной среде, в многопоточной две статических переменных приводят к гонкам данных, что вызывает неопределённое поведение. В многопоточном приложении, если несколько потоков обращаются к этим статическим переменным одновременно, они могут модифицировать их в непредсказуемой последовательности. В итоге мы получали два пула муверов, в один подадало большинство, во второй - один неудачник.

0х18

Вам может казаться что вы пофиксили проблему, но она возвращается через какое-то время. Например в этом простом баге иногда оставались лишние юниты на уровне, и движок ассертил об этом в логе. Но так как это все происходило на выгрузке уровня, то никто особо внимания не обращал. Человек, который был отвественный за эту систему придумал гениальный фикс. Когда переставали проходить тесты, он убирал или добавлял [volatile] и тесты начинали работать. В истории набралось с десяток комитов с такими фиксами.

[volatile] int free_slots = max_slots;

PathFinder* use()
{
    if (int n = free_slots--) return &finders[n];
}
А где ошибка?

В C++ ключевое слово volatile не обеспечивает атомарность, не синхронизирует между потоками и не предотвращает перестановку инструкций (ни на уровне компилятора, ни на уровне оборудования). Оно просто не имеет отношения к конкуренции. Коллеге просто везло, по другому это не назвать, вероятно поворошив код в этом месте, он смещал точку ошибки и тесты её какоето время не ловили. Правильный фикс был в использовании atomic<int>.

0x19

Не совсем чтобы ошибка, но тоже попортила нервов порядком. Цена такого кода плюс несколько секунд на загрузке уровня. Немного информации по багу, у юнита есть компоненты, которые он на старте уровня подгружает в себя. Это может быть все, что угодно - рука, пушка, граната и какой-то другой аттач, система зрения и т.д. Таких компонетов у объектов могут быть десятки и сотни, и самих объектов на сцене тоже десятки и сотни - от декалей и луж, до машин и зданий.

std::map<ComponentId, Component> components;

bool ComponentVec::add(Component c) {
    ...

    if (components.find(c.id()) == components.end()) {
        components.insert({c.id(), c});
    }

    auto &compc = components[c.id()];
    ...
}
А где ошибка?

А ошибка здесь в том, что поиск в мапе компонентов производитя ТРИ раза в худшем случае, первый на find(c.id()), второй на insert() если компоненты нет. И третий в операторе доступа по индексу.

0х1а

А тут потихоньку текла память в менеджере частиц. Cможете найти виновника?

void Particle::CreateVerties(size_t p)
{
  std::shared_ptr<Vertex> ss(new Point[p * 3]); // each p is 3 vertex
  ....
}
А где ошибка?

Наверное будет в коментах :)

0х1b

Обидная копипаста и обидная ошибка, которая была только в релизной сборке, движок проигрывал только первый звук из заданных для ентити. Но так, как обычно звуков было 1, редко 2 или больше, то ошибка долгое время так и жила себе.

int Entity::update_sounds(...)
{
  sounds.lock();
  sounds.commit();
  ...
  int i, size = sounds.size();
  ...
  for(i = 0; i < size; ++i); {
    sounds[i].pre_update(dt);
  }

  for(i = 0; i < size; ++i); {
    sounds[i].update(dt);
  }
  ...
  sounds.unlock();
}
А где ошибка?

Обратите внимание, что цикл for не выполняет никакой работы, но изза вынесенной за пределы цикла переменной i у неё валидное значение 0. Компилятор оптимизациями выкидывал оба цикла и i оставался равным нулю. Выполняется апдейт только первого звука в массиве. А ниже код бы скопипащен и ошибка перебралась и туда.

0x1c

Еще одна частая, глупая, но плохо детектируемая ошибка. Моделькам назначается индекс шейдера, в большинстве случаев все работало, но в редких ситуациях модель рендерилась с графическими ошибками. Индекс шейдера не может быть больше 65к (размера кеша шейдеров). Ошибок не было на Ps5, но на Ps4 модели были с ошибками, игра при этом не крашилась, потому что рендер нормально отрабатывал отложенные шейдеры.

using AssignShaderCB = std::function<int()>;

void LoadModel() {
    BaseModel *model = ... 
    ...

    model.setShader([&] (Model* m) {
        const uint16_t shaderIndex = dummyShader();  
        return [&] () {
            shaderIndex = createShader(m);
            return shaderIndex;
        };
    });
}
А где ошибка?

setShader() устанавливал колбэк для получения индекса шейдера, на ps5 изза более быстрого cpu и предварительно собранных шейдеров этот коллбек вызывался сразу и возвращал правильный индекс, который еще был на стек. На ps4 колбек откладывался при большой загрузке и вызывался позже, но стека где он существовал уже не было и возвращался мусор. А так как любой индекс в пределах кеша был валидный, то модели получали не те шейдеры, которые должны были. Банальный dangling reference.

0x1d

А тут было какое-то дикое стечение обстоятельств, луна не в той фазе, солнечная буря и шабаш ведьм в соседнем кабинете. Но код тесты прошел и даже какое-то время работал.

void BaseBehavior::ChaseEnemy(Human *h) {
    ...
    HumanType enemy = h->type();
    if ( enemy == HumanType::Corpse) {
        return;
    }

    if ( (enemy =! HumanType::Friend) || (enemy == HumanType::Follower)) {
        ...
    }
}
А где ошибка?

Автор перепутал != и =! и получалось, что переменной enemy присваивается !HumanType::Friend, который был ненулевым, получалось 0. А участие в логическом выражении скрывало ошибку и проверялась только правая часть. Код проходил тесты и был выявлен только при ручной проверке.

0x1e

В одном достаточно известном движке, долгое время жила своя функция перевода числа в хекс, видимо из коробки не устраивала скоростью или кодстайлом. Пока кто-то из мейнтейнеров не решил в порыве необузданного рефакторинга, что нулей в этой функции слишком много и залил этот комит в обход тестов, ну а че - изменения то самые банальные.

до "фикса"
const char buffer[] = "abcdef\0000123456789";

после "фикса"
const char buffer[] = "abcdef\0123456789";
А где ошибка?

Только автор "фикса" подзабыл, что \0хх посреди строки интерпретируется как число, а неудачное расположение цифр после такого нуля привело к тому что они стали часть этого числа, т.е. было \000 - завершение строки, а стало \012 - перевод на новую строку. Но алгоритм это сломало, хорошо быстро отловили.

0x1f

Сложный, эта функция использовалась в известном физ движке для поиска произвольного объема, заданного несколькими плоскостями, потенциально любым количеством плоскостей. Работала отлично, если плоскостей было меньше 16, и могла упасть если было больше. Должна работать быстро, потому что часто вызвалась в рантайме для проверок коллизий между объектами.

// NOTE! convex volume created in local space, you need apply xform from owner for correct world positions
template<typename T, typename E, typename V>
inline bool convex_volume_from_planes(const T &clip_planes, E &out_edges, V *out_vertices) {
    const u32 num_planes        = clip_planes.size();
    ...

    for (u32 i = 0; i < num_planes; ++i) {
        pair<vec, vec> * plane_egdes = alloca (16 * sizeof(vec) * 2);
        u32 next_plane_start_index = out_vertices ? 0 : (i + 1); // if we want to collect tris from planes, it should check all intersections
                                                                 // if we need only edges we can skip some checks, because edges was found on previous step
        for (u32 j = next_plane_start_index; j < num_planes; ++j) {
            vec line_orig, line_dir;
            ...

            vec *plane_vectices = alloca(sizeof(vec) * 3); // maybe expanded to 12 later
            ...
        }
    }
}
А где ошибка?

Вообще аллокации памяти в цикле это плохо, втройне плохо если это алокации на стеке, т.к. там память только выделяется, но не возвращается. Работа этого алгоритма в сложных случаях выбирала всю память стека и крашила движок.

На этом всё! Всем безбажных тикетов.

Комментарии (19)


  1. slepmog
    07.10.2024 00:49

    Обратите внимание, что цикл for не выполняет никакой работы, но изза вынесенной за пределы цикла переменной i у неё валидное значение 0. Выполняется апдейт только первого звука в массиве.

    Разве? Цикл выполняет работу: неэффективно меняет i с 0 на size.
    Так что последующее внецикловое sounds[i] даёт sounds[size] (UB), а не sounds[0].


    1. dalerank Автор
      07.10.2024 00:49

      Да, спасибо что поправили, так выглядит. Помню дебажил этот блок, он работал только в релизе, компилятор выкидывал оба цикла и оставлял только вызов первого элемента в итоге. Поправил описание


      1. e-zig
        07.10.2024 00:49
        +1

          int i, size = sounds.size();
          ...
          for(i = 0; i < size; ++i); {
            sounds[i].pre_update(dt);
          }

        а почему этот цикл выкидывается?


        1. dalerank Автор
          07.10.2024 00:49

          Мы с коллегами так и не поняли, попытки поворошить код в этом месте или изменить флаги приводили к нормальной работе. Компилятор clang 7.0.19 для плойки. Но конкретно такой код работал с ошибками


        1. viordash
          07.10.2024 00:49

          хм, а разве он выкидывается? sounds[i].pre_update(dt); исполнится же один раз.


        1. KanuTaH
          07.10.2024 00:49

          Потому что цикл здесь - это вот эта часть: for(i = 0; i < size; ++i);, а компилятор видит, что никаких сайд эффектов у неё нет. Может быть заменен на простое присваивание в i.


          1. viordash
            07.10.2024 00:49

            я так понял что выкидывается и "мнимое" тело цикла, которое после ";". Оно то выполнится один раз


            1. KanuTaH
              07.10.2024 00:49
              +1

              Ну я так понял, что нет - выкидывался именно цикл, а то что после ; - это уже не цикл, апдейт одного-то звука выполнялся и он проигрывался.


          1. e-zig
            07.10.2024 00:49

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


            1. KanuTaH
              07.10.2024 00:49

              Как ни странно, но лучше всего с ним бы справился банальный автоформаттер типа clang-format, который на более-менее крупном проекте ИМХО обязательно должен быть.


              1. e-zig
                07.10.2024 00:49

                Автоформатер и в этой ошибке бы помог:

                if ( (enemy =! HumanType::Friend)

                стало бы как то так:

                if ( (enemy = !HumanType::Friend)

                было бы проще увидеть.


          1. slepmog
            07.10.2024 00:49

            Может быть заменен на простое присваивание в i.

            Тогда опять же, было бы sounds[size], потому что i == size, а автор утверждает, что выходило sounds[0]. Как оно может выходить, я так и не понял. Для этого надо, чтобы for(i = 0 осталось, а ++i) было выкинуто. Потому что без цикла i вообще не инициализировано.


            1. KanuTaH
              07.10.2024 00:49
              +1

              Ну когда i == size, то там уже идет выход за границы этого sounds, а это UB, и кто его знает, во что превратит компилятор этот UB в конкретном случае.


  1. sergio_nsk
    07.10.2024 00:49

    Майкрософт сначала просто морозились и реджектили билды, потом прислали комплайнс на забытые сенситив данные. Какие блин такие забытые?

    Было же нормально, потом русский пошёл вразнос.

    Большинство вышеперечисленных багов - это код, написанный на C в файлах .cpp. Почти все остальные - из-за выключенных предупреждений компиляторов.


    1. dalerank Автор
      07.10.2024 00:49

      Ну что поделать, люди так пишут. Если код решает поставленную задачу, заказчику обычно до лампочки на чем и как он написан, хоть на бейсике, se la vi. А вот когда он попадает к вам уже встаёт вопрос, поправить в рамках того что сделано или рисковать переписывать полждвижка с непонятными перспективах.


  1. JordanCpp
    07.10.2024 00:49

    В геймдеве вообще принято тесты писать? Или девочки из отдела тестирования, протестирую?

    Или геймеры на релизе?


    1. dalerank Автор
      07.10.2024 00:49
      +1

      Ну а как вы думаете? на последнем проекте было за 8к тестов, но они покрфвали от силы треть кода движка и игры.


  1. voldemar_d
    07.10.2024 00:49

    void init(std::vector<TextureDX12> &a) { memset(a.data(), 0, sizeof(TextureDX12) * a.size());}

    В этом куске кода даже C++11 нельзя использовать? Почему бы здесь не использовать for (auto& v... или std::fill?


  1. voldemar_d
    07.10.2024 00:49

    void Particle::CreateVerties(size_t p){ std::shared_ptr<Vertex> ss(new Point[p * 3]); // each p is 3 vertex ....}

    Так тут each Point состоит из 3 Vertex или наоборот - each Vertex состоит из трёх Points?

    Если каждый Vertex содержит 3 точки, разве недостаточно написать так?

    auto ss = std::make_shared<Vertex[]>(p);