Boss, it's all gone

Errors by C++ programmers are a separate form of art, a seemingly simple language, but as soon as you get distracted by a cup of coffee, the compiler starts throwing out a sheet of warnings in half with errors, and sometimes it looks more like ancient Egyptian writing than a normal output. You yourself have probably encountered nullptr dereferencing more than once or confused (= and ==) due to an oversight. Often the cause of an error is laziness or inattention, or fatigue – it’s not in vain that the superstitions “don’t commit on a Friday night”, “don’t code in a state of altered consciousness” or “avoid coding while on a coffee stupor” appeared, well, this is when you’ve had three or four mugs of coffee and went to carry the good code left and right.

In general, the article was planned about how you can make fun of the switch operator – I had a recording of a conversation from a St. Petersburg meetup that took place several years ago and where those present were asked to write “impossible” but working code with a switch. And while searching for this entry, I came across a file with a description of “Friday” bugs and commits, which in a sober mind would hardly have slipped into the master in the middle of the week. And there was also an article about errors, “Fedya, game,” where a respected habra reader guessed the cause of various errors, obvious or not. Therefore, I decided to continue in the same style, question and answer, and I invite you to also participate in guessing possible reasons. All examples are from real projects, games, engines and Friday commits.

C++ is unforgiving, but that's where its charm lies. In most of the examples given, the style and names of the parameters have been preserved, only the comments have been cleaned up a little so as not to scorch the office.

0x10

Ignoring development rules and reinventing wheels in most cases ends in a bug. So here, the engine sometimes crashed when rendering UI widgets, but more often it simply spoiled other widgets that were entirely made on POD types.

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);   
    // ...
}

0x11

When porting the Dagor engine to the Nintendo Switch platform, we had to create a layer to work with the file system. After another Friday, the game began to crash after several loads in random places with OOM (out of memory), and the reason turned out to be just one line. This should be simple.

void OsFile::ropen(const NxString &name) {
    NxFile f = NxFileOpen(name, "r");
    // ...
    if (something) return;   
    // ...
    NxFileClose(f);
}

0x12

A newbie came to the department and was given small bugs to familiarize himself with the code base. And to make it not so boring, we were allowed to refactor the code a little in the dev branch, so that there was a history of commits and an understanding of how onboarding is progressing. On Tuesday, a complaint arrived from QA that the editor has become an order of magnitude slower to load levels – small ones still load normally, but large ones have become straight for about twenty minutes. And only in the QA department, everything else is fine. Here we need to explain a little, the testing tool we used also formatted the level configs before approval. QA suffered for a day, and decided to write to the programmers, who, hot on their heels, found this:

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]);
}

0x13

We sent another patch for boxing, but it doesn’t pass the tests. Microsoft at first simply froze and rejected the builds, then they sent compliance for forgotten sensitive data. What the hell are so forgotten?

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, the 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);
}

0x14

Wonderful Monday morning, a bug arrives from the designers that the entity at the level has ceased to determine the presence of a right and left hand in the skeleton, bones and locators were obtained from there to attach the hands to the corresponding places on the weapon. Or she determined that there were hands, but confused them in places, which caused various graphic glitches, such as the left hand on the butt, etc.

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;
    }
    // ...
};

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

It happens that even Rockstar renderers make mistakes, who, in pursuit of perfection, write all sorts of things on Friday evenings. And then on Monday morning, a hundred changes will be reversed. In theory, the texture pool should contain empty textures that would be taken from there as objects are created. But something went wrong, and the engine crashed on the first call to the texture.

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());
}

0x16

Another example of inattention or an accidental coincidence, the game did not have enough memory on the iPhone6, where the effect pool had to be cut. As a result, on busy scenes we got graphic artifacts in random places. But what’s funny is that the game did not crash, and sometimes these bugs were not noticed at all.

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
    }
}

0x17

You can mess up even in three lines, especially if the code has become multi-threaded; in some cases, a separate unit on the stage did not move. Investigation of the bug showed that Mover (this is the component that was responsible for navigating the unit around the stage) did not receive updates, so it was not in the pool of movers. Will you find a mistake?

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

    return create_mover();
}

0x18

You may think that you have fixed the problem, but it returns after some time. For example, in this simple bug, sometimes there were extra units left on the level, and the engine asserted this in the log. But since this all happened while the level was unloading, no one paid much attention. The person who was responsible for this system came up with a brilliant fix. When they stopped passing tests, he removed or added [volatile] and the tests started working. There are a dozen commits in history with such fixes.

[volatile] int free_slots = max_slots;

PathFinder* use()
{
    if (int n = free_slots--) return &finders[n];
}

0x19

Not exactly a mistake, but it also spoiled my nerves. The price of such a code plus a few seconds to load the level. A little information on the bug; the unit has components that it loads into itself at the start of the level. It can be anything – a hand, a gun, a grenade and some other attachment, a vision system, etc. Objects can have dozens and hundreds of such components, and there can also be tens and hundreds of objects themselves on the stage – from decals and puddles, to cars and buildings.

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()];
    ...
}

0x1a

And then the memory in the particle manager was slowly leaking. Can you find the culprit?

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

0х1b

An offensive copy-paste and an offensive mistake, the engine played only the first sound specified for the entity. But since there were usually 1 sound, rarely 2 or more, the error lived on for a long time.

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();
}

0x1c

Another common, stupid, but difficult to detect error. Models are assigned a shader index, in most cases everything worked, but in rare situations the model was rendered with graphical errors. The shader index cannot be greater than 65k (shader cache size). There were no errors on Ps5, but on Ps4 the models had errors, and the game did not crash because the renderer worked normally with deferred shaders.

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

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

    model.setShader([&] (Model* m) {
        const uint16_t shaderIndex = dummyShader();  
        return [&] () {
            shaderIndex = createShader(m);
            return shaderIndex;
        };
    });
}

0x1d

And then there was some wild coincidence of circumstances, the moon was in the wrong phase, a solar storm and a witches’ Sabbath in the next office. But the code passed the tests and even worked for some time.

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

    if ( (enemy =! HumanType::Friend) || (enemy == HumanType::Follower)) {
        ...
    }
}

0x1e

In one fairly well-known engine, for a long time there was its own function for converting a number into a hex, apparently out of the box it was not satisfactory with the speed or code style. Until one of the maintainers, in a fit of unbridled refactoring, decided that there were too many zeros in this function and uploaded this commit, bypassing the tests, so what – the changes are the most banal.

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

после "фикса"
const char buffer[] = "abcdef\0123456789";

0x1f

Complex, this function was used in a well-known physics engine to search for an arbitrary volume defined by several planes, potentially any number of planes. It worked perfectly if there were less than 16 planes, and could fall if there were more. It should work quickly, because it was often called in runtime to check collisions between objects.

// 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
            ...
        }
    }
}

That's all! Bad tickets to everyone.