Breaking down the C++ quiz by Sergei Kushnirenko

• Task N1. You spin me right round
• Task N2. Hash sums
• Task N3. Threads
• Task N4. Mutex
• Task N5. A dangerous function with a variable number of arguments
• Task N6. unique_lock
• Task N7. memcmp
• Task N8. operator =
• Task N9. A line in DLL
• Task N10. serialize
• Task N11. sayHello
• Task N12. Number modulus
• Task N13. Where's the money, Lebowski?
• Task N14. cin
• Task N15. display_string
• Task N16. Bar
• Task N17. Bard
• Task N18. Iterators
• Thanks everyone
• Additional links

The PVS-Studio team along with Sergei Kushnirenko prepared a quiz based on his publications. Take the quiz, challenge your focus and coding skills while looking for errors in the C++ code. This article provides a detailed analysis of all the questions.

If you haven't taken the quiz, but have opened this article, we urge you not to read it just yet. Don't deny yourself the pleasure of "bug-spotting", that's the funniest part.

Take the quiz and come back to us.

Now that you have completed the quiz, you may have questions or not understand some of the tasks and answers. Make yourself some tea or coffee and we'll take a closer look at the tasks. But first, let us say a few words of gratitude.

We created the quiz with the help of Sergei Kushnirenko's articles on Habr. The PVS-Studio team came up with the idea of organizing the code fragments from his articles into an entertaining exercise book.

Many thanks to Sergei for taking part in this project.

Task N1. You spin me right round

This code fragment was in the Unity 2014 engine to control an object rotation using gizmo (a control element that usually has the shape of three circles — it enables you to rotate the object in the scene). The setDeltaPitch function here changes the object angle relative to its vertical axis (pitch). At angles close to 0 (depending on the editor settings), the function simply turned the object upside down, which really annoyed level designers.

void  UObject::setDeltaPitch(const UMatrix &gizmo) {
  ....
  if (_fpzero(amount, eps))
    return
  rotateAccum.setAnglesXYZ(axis);
  ....
}

Here are the answer options:

• Actually, everything's correct. This is just a warm-up example.
• The actual arguments are mixed up in the _fpzero function.
• The UMatrix class uses the float types, accuracy suffers as the result. Developers should have used UMatrixD based on the double types.
• The code design doesn't match the logic of its operation. [CORRECT]

The answer

There is no semicolon after the return. So, under certain conditions, it can cause the SetAnglesXYZ function to be called in the controlled object. As a result, the object would rotate to an arbitrary angle.

A note from the PVS-Studio team

It seems that errors caused by forgotten semicolons are quite common. Various articles mention them frequently. In reality, however, they are very rare. In more than 10 years of checking open-source projects, we've found only 3 such errors. The fact that everyone is aware of such errors and pays attention to them helps. Analyzers and compilers also seem to detect them well.

Task N2. Hash sums

The compiler had its fun here. The developers used this function to calculate the hash sum when checking file integrity. During content creation, a hash of the files is calculated and stored with the files. Later, when loading the same file, the Unity player would again calculate the hash of the file and compare it to the saved one to ensure that the file hasn't been modified or corrupted. Encryption was used at the final packaging stage. The code author believed that the key will not leak outside the function, but something went wrong. With some of the leaked Unity source code and the bug in the engine, SKIDROW cracked Syberia 3 and several other big Unity games in 2017. The games used native content encryption tools. Denuvo was there too, but SKIDROW learned how to bypass it even earlier.

void UContentPackage::createPackage(dsaFile *m, const void *v, int size) {
  unsigned char        secretDsaKey[3096], L;
  const unsigned char *p = v;
  int                  i, j, t;
  ....
  UContent::generateDsaKey(secretDsaKey, sizeof(salt));
  ....
  // there was some code to encrypt and sign the file
  ....
  memset (secretDsaKey, 0, sizeof(secretDsaKey));
}

Here are the answer options:

• The memset function is fake. The above is an intentional vulnerability that overrides the memset function.
• The compiler removes the call to memset because it thinks it's not needed here. [CORRECT]
• The compiler removes the call to memset due to a rare bug. The next compiler release fixed it. However, the broken version was used when the bug was discovered.
• An error is in the generateDsaKey function. It overflows the buffer, and some data gets lost somewhere.

The answer

The compiler applied aggressive optimizations and removed the call to the memset function. No operations with secretDsaKey were executed after memset, so the compiler would just optimize the function call out. All key contents remained on the stack.

A note from the PVS-Studio team

This case is the exact opposite of the previous error pattern. The disappearing memset function call seems exotic and rare. However, it's a very common potential vulnerability. At the time of writing the article, our collection already counts 330 such errors in various open-source projects, and the number keeps growing. Although, it seems that a lot has already been written on this topic:

• CWE-14
• Zero and forget -- caveats of zeroing memory in C
• Safe Clearing of Private Data

Private data can also be sent externally, for example, over a network. How? We discussed this topic in the article: "Overwriting memory - why?".

Task N3. Threads

Working in two or more threads with any of the a/b variables can be problematic. The error was in the CryTek engine when synchronizing car states over the network. It caused jerky movements and teleportation when players were driving a vehicle in FarCry 1 multiplayer. The more players were on the map, the more likely the last player would teleport. With 16 players on the map, the last player would always teleport when using a car.

struct X {
  int a : 2
  int b : 2
} x;

Thread 1:
void foo() { x.a = 1 }

Thread 2:
void boo() { x.b = 1 }

Here are the answer options:

• The entries in the a and b fields are non-atomic. They can be partially executed, and other operations can interrupt them. [CORRECT]
• The size of the first field was ignored due to a bug in the compiler. So, if a was written to a variable, b would also be overwritten.
• The developers forgot that a character takes one bit, making the value range of two-bit variables insufficient: [-2..1]. They needed to make three-bit fields.

Here's the answer

The atomicity of the write operation is violated. The a and b fields are non-atomic, so they can be partially executed and interrupted by other operations. This code fragment has shared access to the complex AB variable, which has two two-bit parts. However, the compiler can't do such an assignment atomically by grabbing the ab byte and using bitwise operations to set the desired value. This can lead to data races and undefined behavior in a multithreaded environment.

foo():                                # @foo()
        mov     AL, BYTE PTR [RIP + x] ; broken assignment 1
        and     AL, -4                 ; broken assignment 2
        or      AL, 1                  ; broken assignment 3
        mov     BYTE PTR [RIP + x], AL ; finished
        ret

boo():                                # @boo()
        mov     AL, BYTE PTR [RIP + x]
        and     AL, -13
        or      AL, 4
        mov     BYTE PTR [RIP + x], AL
        ret

Task N4. Mutex

The following code fragment contains a data race even with a seemingly functional mutex. The error has been found in the Nintendo Switch 4.5.1 firmware and above. Developers accidentally stumbled across it while trying to speed up the creation of UI texture atlases at the game start. When they tried to load more than 100 textures, the atlas would break. However, when there were fewer textures, the atlas would be built without any issues. The developers still haven't fixed the zombie mutexes on Nintendo Switch. Moreover, there you couldn't create more than 256 mutexes per application. So, the system is "fun".

const size_t maxThreads = 10;
 
void fill_texture_mt(int thread_id, std::mutex *pm) {
  std::lock_guard<std::mutex> lk(*pm);
  
  // Access data protected by the lock.
}
 
void prepare_texture() {
  std::thread threads[maxThreads];
  std::mutex m;
 
  for (size_t i = 0; i < maxThreads; ++i) {
    threads[i] = std::thread(fill_texture_mt, i, &m);
  }
}

Here are the answer options:

• The wrong default class was selected for implementing locks.
• There is no waiting for threads to be completed. Therefore, the mutex is removed once the prepare_texture function finishes, and no thread locking occurs. [CORRECT]
• The above code is correct. However, this type of parallelization is inefficient, but that's a different story.
• We need to declare the m variable with the volatile specifier.

The answer

The mutex is deleted after exiting the prepare_texture function. The OS doesn't react to an inactive mutex because, as a core object, it stays for a while. The address is valid and contains correct data but doesn't provide actual thread locking.

Note. According to the documentation, destructors for active std::threads are called when exiting the function. This should cause a crash (a call to terminate). However, according to the code author, this didn't happen in this project. This is strange, but it doesn't really matter because the code is incorrect anyway.

Task N5. A dangerous function with a variable number of arguments

We can define functions so that they take more arguments when called than when declared. Such functions are called variadic. C++ provides two mechanisms for defining a variadic function: a pattern with a variable number of parameters and a C-style ellipsis as the last parameter declaration. A rather unpleasant behavior occurred in the popular sound library — FMOD Engine. The code is given as it was in the source: it seems that the devs wanted to save on templates. You can play with the code on the OnlineGDB website.

int var_add(int first, int second, ...) {
  int r = first + second; 
  va_list va;
  va_start(va, second);
  while (int v = va_arg(va, int)) {
    r += v;
  }
  va_end(va);
  return r;
}

Here are the answer options:

• The declaration of the va variable should come right at the beginning of the function, before the r variable.
• The last function parameter should be 0. Otherwise, undefined behavior occurs.
• Passing any type other than int results in undefined behavior.
• Options 2 and 3 are both correct. [CORRECT]
• The code fragment is correct. An error is external (for example, if somebody has tampered with macros).

The answer

This is a C-style variadic function for adding a series of integers. The arguments are read until 0 is found. Calling the function without passing 0 as the last argument (after the first two arguments) causes undefined behavior. Passing any type other than int also results in undefined behavior.

Task N6. unique_lock

Here we didn't lock anything again, even though it looked like we did. This code fragment was found in the Metro: Exodus engine. When working with resources, the game strangely crashed. The developers found it thanks to bug reports and one clever French guy.

static std::mutex m;
static int shared_resource = 0;
 
void increment_by_42() {
  std::unique_lock<std::mutex>(m);
  shared_resource += 42;
}

Here are the answer options:

• Despite the locking, developers need to declare the shared_resource variable as volatile.
• An anonymous object is created and destroyed right away. No locking occurs because it's immediately destroyed.
• A new variable named m is created here, it's initialized by default. So, the mutex isn't captured. [CORRECT]

The answer

The code author expects the std::unique_lock anonymous local variable to lock and unlock the m mutex via RAII. In fact, the new m variable is created here, it's initialized by default. So, the mutex isn't captured. See also: "Most vexing parse".

A note from the PVS-Studio team

The error is quite rare. PVS-Studio has the V1025 diagnostic rule for detecting it. However, we have not yet found such an error when checking open-source projects. It was interesting to see it, as they say, in its natural habitat :).

Task N7. memcmp

The following code fragment was used to calculate region checksums in PathEngine. In the release build, it had issues on different compilers. The PlayStation build used a specific flag that masked the issue, while the Xbox build didn't. Developers found the error when trying to build the library on a PC using Clang.

struct AreaCrc32 {
  unsigned char buffType = 0;
  int crc = 0;
};

AreaCrc32 s1 {};
AreaCrc32 s2 {};

void similarArea(const AreaCrc32 &s1, const AreaCrc32 &s2) {
  if (!std::memcmp(&s1, &s2, sizeof(AreaCrc32))) {
    // ....
  }
}

Here are the answer options:

• The answer is simple: it's a typo. The negation operator (!) is redundant.
• The issue relates to the field alignment in the structure. [CORRECT]
• The pointer size is calculated instead of the structure size.

The answer

The fields in the structure are aligned on a 4-byte boundary. Padding bytes, which may be filled with zeros or with garbage, are formed between buffType and sgs.

struct S {
  unsigned char buffType = 0;
  char[3] _padding = { compiler don't care about content in release };
  int crc = 0;
};

The memcmp function compares memory on a byte-by-byte basis, including garbage bytes. So, we get an unknown result when we compare s1 to s2. The PlayStation compiler settings explicitly tell the compiler to fill padding bytes with zeros. The C++ rules for structures state that we need to use the operator == here. The memcmp function isn't designed for this task.

By the way, here's another case of comparing redundancies. We noticed it once during a code review, and just in time.

class C {
  int i;
  
public:
  virtual void f();
   
  // ...
};
  
void similar(C &c1, C &c2) {
  if (!std::memcmp(&c1, &c2, sizeof(C))) {
    // ...
  }
}

In C++, comparing structures and classes using memcmp is a bad idea, especially if they contain a pointer to a virtual function table.

Let's say there are two different classes inherited from C. Then, if c1 and c2 are references to instances of these two different classes, they won't be equal, even if all the data is the same (except for the pointer to the virtual function table).

But wait, there's more. The presence of an implicit pointer in the class can cause alignment issues (the presence of padding bytes).

Task N8. operator =

It should be simple, but things like this often confuse applicants during interviews.

struct S { S(const S *) noexcept; /* ... */ };
  
class T {
  int n;
  S *s1;
  
public:
  T(const T &rhs) : n(rhs.n),
                    s1(rhs.s1 ? new S(rhs.s1) : nullptr) {}
  ~T() { delete s1; }
  
  // ...
  
  T& operator=(const T &rhs) {
    n = rhs.n;
    delete s1;
    s1 = new S(rhs.s1);
    return *this;
  }
};

Here are the answer options:

• Writing return rhs would be more correct.
• There is no protection against assigning the object to itself in operator =.
• You need to zeroize the s1 pointer after delete s1; in operator =. Otherwise, the double free may occur.
• Options 2 and 3 are both correct. [CORRECT]
• If it's a 64-bit application, you can lower the size of the structure by swapping the n and s1 fields. The size will decrease from 16 bytes to 12 bytes.

The answer

There's no check for assigning the object to itself in operator =. As a result, we delete the data and load some garbage.

There's another weak point in operator =. If exceptions are not disabled in the project, new may potentially throw std::bad_alloc. In this case, this->s1 keeps the old and possibly non-zero value which is then passed to delete in the destructor. As a result, we get double free.

A note from the PVS-Studio team

Indeed, when rearranging the fields in a structure or class, sometimes you can reduce their size. However, it doesn't apply to the class in question. In a 32-bit program, the class size is always 8 bytes; in a 64-bit program, it's always 16 bytes because of the alignment required. At least, this is true for the most common data models and architectures. Learn more about reducing the size of classes and structures: V802.

Task N9. A line in DLL

You can catch an unobvious bug in as little as four lines of code. Especially in Unity Engine, which likes to include common headers in different DLLs and then compare them like this: super_secret() == super_secret(). What could possibly go wrong, right? Well, secret != secret, and the game doesn't run on Windows Phone.

// header a.h
constexpr inline const char* super_secret(void) {
  constexpr const char *STRING = "string";
  return STRING;
}

// somewhere in a.dll
if (super_secret() == saved_string_ptr)

// somewhere in b.dll
if (super_secret() != saved_string_ptr)

Here are the answer options:

• It's in the h-file, so using this function in different DLLs can be problematic. Each DLL has its own string instance. [CORRECT]
• The function returns a pointer to a temporary line, and the pointer can't be used. Or rather, the use of it leads to undefined behavior.
• We need to replace constexpr with static.

The answer

The error is caused by using a common a.h header in different libraries. Everything works fine as long as the engine is built as a single DLL. By building two different DLLs, we get two different addresses for STRING. The error here occurs because strings are compared as pointers instead of using the strcmp function.

A note from the PVS-Studio team

Most of the bugs we've described in this article are related to the game development industry in one way or another. So, there are probably a lot of game devs reading this article. The PVS-Studio team wants to ask you for something. We have started to implement special diagnostic rules in the analyzer. They are designed to detect bugs specific to Unity and Unreal Engine projects. We appreciate your ideas for new diagnostic rules, so don't hesitate to share them.

Task N10. serialize

The function may have UB with a buffer overflow. This often happens with the -O3 optimization for Clang, and doesn't happen with -O0. The issue has already been fixed for all optimization modes in Clang 12.10 and above. I didn't write the code. It came up during one of the interviews when we were just having a heart-to-heart.

char destBuffer[16];
void serialize(bool x) {
  const char* whichString = x ? "true" : "false";
  const size_t len = strlen(whichString);
  memcpy(destBuffer, whichString, len);
}

int main() {
  bool x;
  serialize(x);
  printf("%s", destBuffer);
  return 0;
}

Here are the answer options:

• Since the x variable is not initialized, the program outputs "true" or "false" depending on the optimization keys and moon phase.
• Not only "true" or "false", but also an empty string can show up due to undefined behavior.
• The code doesn't compile.
• Using the uninitialized x variable results in undefined behavior. There's no point in discussing what the outcome can be. The program can crash or display garbage — anything is possible. [CORRECT]

The answer

The compiler fooled itself here. So, what's going on? First, remember that bool isn't just 0 or 1. It's compiler-dependent. Clang determines that 0 is false and everything else is true. A strict compiler optimization converts the len calculation to 'len = 5 - x'. However, the x variable is not initialized, so we may get 'len = 5-7'. In this case, calling memcpy causes the program to crash due to a buffer overflow.

A note from the PVS-Studio team

Programmers are often wrong when they try to predict how undefined behavior will manifest itself. It can't and shouldn't be done. Here's a good article on the subject: "Falsehoods programmers believe about undefined behavior".

Task N11. sayHello

What will be displayed as a result of running the program?

void sayHello() {
    std::cout << "Hello, World!\n";
}
void sayHello() {
    std::cout << "Goodbye, World!\n";
}
int main() {
    sayHello();
    return 0;
}

Here are the answer options:

• Don't mess with my mind. The code doesn't compile.
• The program will display "Hello World".
• The program will display "Hello World" first, followed by "Goodbye, World!". It's a special kind of magic.
• There is a trick I already know about. Or I don't know it yet but can assume. [CORRECT]

The answer

The console will display "Goodbye, World!" when you run the code. This is because the name of the second sayHello function is written in Unicode, which unfortunately can't always be displayed. So, this function is called (Clang, latest).

sayHello():                           # @sayHello()
        push    rbp
        mov     rbp, rsp
        mov     rdi, qword ptr [rip + std::cout@GOTPCREL]
        lea     rsi, [rip + .L.str]
        call    std::basic_ostream<char, std::char_traits<char> >&
  std::operator<< <std::char_traits<char> >(std::basic_ostream<char,
  std::char_traits<char> >&, char const*)@PLT
        pop     rbp
        ret
"_Z9sayHellov":                         # @"_Z9say\D0\9Dellov"
        push    rbp
        mov     rbp, rsp
        mov     rdi, qword ptr [rip + std::cout@GOTPCREL]
        lea     rsi, [rip + .L.str.1]
        call    std::basic_ostream<char, std::char_traits<char> >&
  std::operator<< <std::char_traits<char> >(std::basic_ostream<char,
  std::char_traits<char> >&, char const*)@PLT
        pop     rbp
        ret

A note from the PVS-Studio team

Anyone interested in learning more about the Unicode nuances is welcome to read: "Trojan Source attack for introducing invisible vulnerabilities".

Task N12. Number modulus

The function has the amazing capability to alter reality depending on how many legs its caller has.

int abs_legs(int my_legs) {
  if (my_legs < 0) {
    return -my_legs;
  }
}

Here are the answer options:

• What's there to think about? At the end of the function, there is no return which would be executed for non-negative numbers. This results in undefined behavior. [CORRECT]
• Actually, there is nothing wrong at all, as the code will not compile because of the missing return.
• Everything is correct. Since C++20, a function with a single argument is assumed to return that argument unless there is an explicit return. That is, the function will simply return my_legs by default. The funny thing is that the code used to be wrong, but was fixed when the developers switched to C++20.

The answer

A value-returning function should return a value (hm!) from all possible branches, because there will always be a condition that leads to undefined behavior.

Here's the correct code:

int abs_legs(int my_legs) {
  if (my_legs < 0) {
    return -my_legs;
  }
  return my_legs;
}

A note from the PVS-Studio team

Many people think that undefined behavior here boils down to the fact that the function can return a random value. Let us say once again that this is not the case. All we can say for sure is that this code contains an error. It's impossible to predict exactly how it will reveal itself. For example, the compiler may not generate a function exit statement at all. Don't you believe? Then you may want to read "An example of undefined behavior caused by absence of return".

Task N13. Where's the money, Lebowski?

The following function doesn't fully expand, leaving its magic behind the compiler veil.

int get_money(int index, const int *pockets) {
  int a = index + pockets[++index];
  // ....
  return a;
}

Here are the answer options:

• We're looking at unspecified behavior.
• It's undefined behavior. [CORRECT]
• Since C++17, this code is correct, and we know what it will compute.

The answer

The compiler may rearrange the evaluation order for index + pockets[++index], leading to undefined behavior with different optimization settings. An unordered or undefined sequence of operations will cause a side effect when working with the index variable.

The same code given in "Order of evaluation" is considered to result in undefined behavior:

n = ++i + i;      // undefined behavior

So, we can rewrite the code like this:

int a = index + pockets[index + 1];
++index;

Or like this, depending on what we want to get:

++index;
int a = index + pockets[index];

A note from the PVS-Studio team

Do you know how the following code will behave?

printf("%d, %d\n", i++, i++);

Are you sure? We invite you to read the following article: "How deep the rabbit hole goes, or C++ job interviews at PVS-Studio".

Task N14. cin

The code looks correct, but we put it here for a reason, right? :)

void umcp_read_buffer_from_pipe() {
  char bufKernel[12];
  char bufMatrix[12];
  std::cin.width(12);
  std::cin >> bufKernel;
  std::cin >> bufMatrix;
}

Here are the answer options:

• Another call to std::cin.width(12) is needed.
• The code works correctly starting with C++20.
• Options 1 and 2 are both correct. [CORRECT]
• The code is correct. It's just an example to take a little breather :)
• The correct way would be to write std::cin.width(12-1); to take into account the terminal null.
• The code results in undefined behavior regardless of the input data.

The answer

In this example, the first read doesn't lead to overflow and fills bufKernel with a truncated string. But the second read can overflow bufMatrix. To prevent this from happening, let's call std::cin.width(12); before getting the bufMatrix. Or we can safely work via strings.

void umcp_read_buffer_from_pipe() {
  std::string bufKernel, bufMatrix;
  std::cin >> bufKernel >> bufMatrix;
}

If you're using C++20, the next function overload is selected and everything goes safely. Even if you didn't do it on purpose...

template< class Traits, std::size_t N >
basic_istream<char, Traits>&
  operator>>( basic_istream<char, Traits>& st, signed char (&s)[N] );

Task N15. display_string

It seems that in this code fragment, the "we-don't-do-that-here" programmer simply mixes randomness with absurdity and calls it "display customization".

std::string str_func();
void display_string(const char *);

void set_display_options() {
  const char *str = str_func().c_str();
  display_string(str);
}

Here are the answer options:

• The temporary string object is destroyed, and the pointer to the buffer can no longer be used. [CORRECT]
• This code doesn't compile since C++11.
• If the string is empty, the str pointer is nullptr.

The answer

Here std::string::c_str is called for the temporary std::string object. The resulting pointer points to freed but possibly still valid memory after destroying the std::string object at the end of the assignment expression. However, don't count on it. You will get undefined behavior when dereferencing this pointer.

We can fix the code like this:

std::string str = str_func();
const char *cstr = str.c_str();
display_string(cstr);

Or like this:

display_string(str_func().c_str())

Task N16. Bar

Why is it dangerous to go to a bar in the morning?

void morning(const std::string &owner) {
  std::fstream bar(owner);
  if (!bar.is_open()) {
    // Handle error
    return;
  }

  bar << "customer";

  std::string str;
  bar >> str;
}

Here are the answer options:

• It's fine. Let's write "customer" and read the same thing.
• We'll read the "void" from the stream. [CORRECT]
• The check whether the stream has been opened is implemented incorrectly.
• The code is dangerous if you enable the -O2 optimization mode.

The answer

Because there is a chance you won't exit the bar. The data is added to the end of the file and then read from the same file. However, since the position indicator in the file remains in the same place (at the end), an attempt to read the data (from the end) of the file leads nowhere. You have to go to the bar in the evening, then you can exit it without any issues.

void evening(const std::string &owner) {
  std::fstream bar(owner);
  if (!bar.is_open()) {
    // Handle error
    return;
  }

  bar << "customer";

  std::string str;
  bar.seekg(0, std::ios::beg);
  bar >> str;
}

Task N17. Bard

Will the bard become a drunkard?

class Bard {
  int _beer;
  int _meal;

public:
  Bard(int meal) : _meal(meal), _beer(_meal - 1) {}
};

Here are the answer options:

• You can't start a class field with an underscore character. This leads to unexpected consequences when using some compilers.
• The _meal variable, which is not yet initialized, is used to initialize _beer. [CORRECT]
• Everything is correct.
• Everything is correct if there can be no inheritance from this class.

The answer

Class members are initialized in the order of their declaration, not in the order in which they are listed in the constructor initialization list.

The class member initialization order is broken. So, the _beer variable is initialized first, and then _meal. Trying to read the _meal value before it's initialized leads to undefined behavior. Most likely, the bard will get drunk without a snack.

We can fix it like this:

Bard(int meal) : _beer(meal - 1), _meal(meal) {}

Another way to fix this is to use the meal variable for initialization everywhere:

Bard(int meal) : _meal(meal), _beer(meal - 1) {}

We can also swap the class fields:

class Bard {
  int _meal;
  int _beer;

public:
  Bard(int meal) : _meal(meal), _beer(_meal - 1) {}
};

Task N18. Iterators

When would a shot in the leg be particularly effective?

std::vector<....> formats;
....
for (auto format = begin(formats), __end = end(formats);
     format != __end; ++format)
{
  if (snd::CodecNamesEq(....)) {
    format->is_stereo = true;
    formats.push_back(stereo_format);
  }
}

Here are the answer options:

• Variable names should not contain a double underscore.
• After push_back is called, the iterators become invalid.
• We need to write format++, not ++format. It's important here.
• Options 1 and 2 are both correct. [CORRECT]
• Options 1, 2, and 3 are correct.

The answer

If the push_back process reallocates inside the formats container, the iterator format points to deleted memory, which causes a lot of issues.

Moreover, a variable named __end is not a good thing. Variables containing a double underscore are reserved for a standard library and internal use.

Note. It's implied that the used container can invalidate iterators when inserting elements at the end, although this isn't specified in the code. That's important because push_back in std::list doesn't do that.

Thanks everyone

We'd like to thank Sergei Kushnirenko for providing materials for the quiz and this article. A kind word to Andrey Karpov, Mikhail Gelvih, Phillip Khandeliants, and other PVS-Studio team members. Why not actually praise ourselves? :) Thank you, readers, we hope you had fun. If you want more stuff like this, follow the link.

Drop a comment below to share your funny stories or the weirdest code you've ever come across.

Additional links

• A discussion on some tasks listed here on Reddit: Hello, World! Where are your bugs?
• You can use the PVS-Studio analyzer to find most of the errors described here. We haven't gone into details of what the analyzer can and can't find and why, because we didn't want to overload the text. However, if you think that your code might have such unexpected issues, we suggest you download the analyzer and try it.