Analyzing Godot Engine

Developing and playing games can be incredibly exciting and addictive activities that bring great pleasure. But nothing spoils the impression of the gameplay more than a cunningly hidden bug. So today we will take a close look at the Open Source Godot Engine. Let's check how good it is and whether it is ready to give us unforgettable emotions from creating and playing games.

Godot

Godot is a general-purpose 2D and 3D game engine designed to support all kinds of projects. It can be used to create games or applications that can then be released on desktop, mobile, or web platforms.

The engine is quite young, but is already gaining momentum and is popular among those who value open source, free and good extensibility. Godot is very friendly to beginners, and therefore popular among novice developers.

The engine was used to write games such as 1000 days to escape, City Game Studio: Your Game Dev Adventure Begins, Precipice.

Godot Engine version tested – 4.2.2.

By the way, we already tested Godot Engine in 2018. You can read the previous article Here.

Results of checking with PVS-Studio

And the first thing I would like to start with after viewing the report is the problems with macros in the project. The problem with them is that the parameters are not wrapped in parentheses. Let's look at a few examples where this can shoot you in the foot.

Fragments N1-N2

#define HAS_WARNING(flag) (warning_flags & flag)

This macro is needed to check whether a certain warning flag is set or not.

Variable warning_flags is a bitmask and has the type uint_32t. This means that its value consists of 32 bits, where each bit corresponds to 1 if the flag is set, and 0 if not. The macro is used in conditional statements, where it is implicitly converted to the type bool. To understand the work, let's consider a simplified version, where instead of 32 bits we will use 8.

Suppose we have some flag X that corresponds to bit 4 in the mask and it is currently set to high. Then the value of the variable warning_flags in binary will have the following form:

00001000

Now let's assume that we decide to check with our macro whether the flag is set X.

We pass a variable to the macro flag with meaning 00001000 and as a result of the bitwise “AND” we get a non-zero value, which is converted to bool with the meaning of k true.

Now suppose we wanted to check the flag Ywhich corresponds to the third bit, with the same value of the variable *warning_flags. *We pass a variable with the value to the macro 00000100 and as a result of the bitwise “AND” we get a zero value, which is converted to bool with meaning false.

It would seem that everything is fine, and what could go wrong? But what if someone wants to check whether one of several flags is set? Then he can call the macro like this:

if (HAS_WARNING(flags::X | flags::Y)) ....

And then the result of such an operation will always be trueeven if none of the flags are set. Why does this happen? Let's work with the preprocessor and simply substitute the expression passed to the macro:

if (warning_flags & flags::X | flags::Y) ....

Now let's turn to table operator priorities:

The operators are listed from top to bottom in descending order of precedence. This means that the expression will be processed as follows:

if (( warning_flags & flags::X ) | flags::Y) ....

Let's say, in warning_flags the flags we are interested in are not set X And Y. Then the first bitwise AND operation will return the value 0, and then the flag will be set to it Y. We always receive a true test.

In fact, the analyzer issues the following warning on this macro:

V1003 The macro 'HAS_WARNING' is a dangerous expression. The parameter 'flag' must be surrounded by parentheses. shader_language.cpp 40

And as the message states, the fix is ​​as simple as wrapping the macro parameter in parentheses:

#define HAS_WARNING(flag) (warning_flags & (flag))

Another example of a dangerous macro:

#define IS_SAME_ROW(i, row) (i / current_columns == row)

Analyzer warning:

V1003 The macro 'IS_SAME_ROW' is a dangerous expression. The parameters 'i', 'row' must be surrounded by parentheses. item_list.cpp 643

If we pass some expression to the macro instead of one variable, for example, like this:

IS_SAME_ROW(current + 1, row)

Then as a result of preprocessor substitution we get:

(current + 1 / current_columns == row)

Where the order of calculation is not at all what was expected.

To protect yourself from such situations, simply wrap the macro parameters in brackets:

#define IS_SAME_ROW(i, row) ((i) / current_columns == (row))

Fragment N3

Now consider the following condition:

const auto hint_r = ShaderLanguage::ShaderNode::Uniform::HINT_ROUGHNESS_R;
const auto hint_gray = ShaderLanguage::ShaderNode::Uniform::HINT_ROUGHNESS_GRAY;

if (tex->detect_roughness_callback
    && (   p_texture_uniforms[i].hint >= hint_r
        || p_texture_uniforms[i].hint <= hint_gray))
{
  ....
}

This condition will always be true (except for the case when the pointer tex->detect_roughness_callback will be zero).

To understand why this is so, you need to look at enum Hint in the structure Uniform:

struct Uniform
{
  ....
  enum Hint 
  {
    HINT_NONE,
    HINT_RANGE,
    HINT_SOURCE_COLOR,
    HINT_NORMAL,
    HINT_ROUGHNESS_NORMAL,
    HINT_ROUGHNESS_R,
    HINT_ROUGHNESS_G,
    HINT_ROUGHNESS_B,
    HINT_ROUGHNESS_A,
    HINT_ROUGHNESS_GRAY,
    HINT_DEFAULT_BLACK,
    HINT_DEFAULT_WHITE,
    HINT_DEFAULT_TRANSPARENT,
    HINT_ANISOTROPY,
    HINT_SCREEN_TEXTURE,
    HINT_NORMAL_ROUGHNESS_TEXTURE,
    HINT_DEPTH_TEXTURE,
    HINT_MAX
  };
  ....
};

Under the box of such an enum is an integer type, and the values HINT_ROUGHNESS_R And HINT_ROUGHNESS_GRAY correspond to the numbers 5 and 9.

Based on this, the condition checks that p_texture_uniforms[i].hint >= 5 or p_texture_uniforms[i].hint <= 9. This means that any value p_texture_uniforms[i].hint will pass these checks, which is what PVS-Studio warns about:

V547 Expression is always true. material_storage.cpp 929

In fact, the programmer wanted to check that p_texture_uniforms[i].hint is in the range from 5 to 9. To do this, you need to use the logical “AND”:

if (tex->detect_roughness_callback
    && (   p_texture_uniforms[i].hint >= hint_r
        && p_texture_uniforms[i].hint <= hint_gray))
{
  ....
}

Similar operation:

Fragment N4

Try to find the error here yourself:

Error FontFile::load_bitmap_font(const String &p_path)
{
  if (kpk.x >= 0x80 && kpk.x <= 0xFF)
  {
    kpk.x = _oem_to_unicode[encoding][kpk.x - 0x80];
  } else if (kpk.x > 0xFF){
    WARN_PRINT(vformat("Invalid BMFont OEM character %x
                        (should be 0x00-0xFF).", kpk.x));
    kpk.x = 0x00;
  }
   
  if (kpk.y >= 0x80 && kpk.y <= 0xFF) 
  {
    kpk.y = _oem_to_unicode[encoding][kpk.y - 0x80];
  } else if (kpk.y > 0xFF){
    WARN_PRINT(vformat("Invalid BMFont OEM character %x
                        (should be 0x00-0xFF).", kpk.x));
    kpk.y = 0x00;
  }
  ....
}

Analyzer warning:

V778 Two similar code fragments were found. Perhaps, this is a typo and 'y' variable should be used instead of 'x'. font.cpp 1970

So, PVS-Studio found an error that occurred when copying a piece of code. Let's take a closer look at the conditional blocks. In essence, they are identical, except that in the first case, all operations are performed on kpk.xand in the second – above kpk.y.

But in the second condition, as a result of copy-paste, an error got into it. Pay attention to the call WARN_PRINT: If kpk.y > 0xFFthen when generating a warning, the symbol will be printed kpk.xbut not kpk.y. It will be more difficult to find an error based on logs 🙂

PS: Actually, you shouldn't have duplicated the code this way. It's clear that the two code blocks only differ in the field they use. A better option would have been to move the code into a function and call it twice for different fields:

Error FontFile::load_bitmap_font(const String &p_path)
{
  constexpr auto check = [](auto &ch)
  {
    if (ch >= 0x80 && ch <= 0xFF)
    {
      auto res = _oem_to_unicode[encoding][ch - 0x80];
      ch = res;
    }
    else if (ch > 0xFF)
    {
      WARN_PRINT(vformat("Invalid BMFont OEM character %x
                              (should be 0x00-0xFF).",ch));
      ch = 0x00;
    }
  };

  check(kpk.x);
  check(kpk.y);
  ....
}

Fragment N5

More conditions, but already nested:

void GridMapEditor::_mesh_library_palette_input(const Ref<InputEvent> &p_ie) 
{
  const Ref<InputEventMouseButton> mb = p_ie;
  // Zoom in/out using Ctrl + mouse wheel
  if (mb.is_valid() && mb->is_pressed() && mb->is_command_or_control_pressed()) 
  {
    if (mb->is_pressed() && mb->get_button_index() == MouseButton::WHEEL_UP) 
    {
      size_slider->set_value(size_slider->get_value() + 0.2);
    }
    ....
  }
}

Analyzer warning:

V571 Recurring check. The 'mb->is_pressed()' condition was already verified in line 837. grid_map_editor_plugin.cpp 838

This fragment contains an extra check in the nested statement. if. Expression mb->is_pressed() was already checked at a higher level. It may be a double check (often seen in GUIs), but then it would have been worth adding a comment about it. Or it may be that something else should have been checked.

Similar triggers:

• V571 Recurring check. The '!r_state.floor' condition was already verified in line 1711. physics_body_3d.cpp 1713

• V571 Recurring check. The '!wd_window.is_popup' condition was already verified in line 2012. display_server_x11.cpp 2013

• V571 Recurring check. The 'member.variable->initializer' condition was already verified in line 946. gdscript_analyzer.cpp 949

Fragment N6

And where would we be without the classics – dereferencing a pointer before checking it:

void GridMapEditor::_update_cursor_transform()
{
  cursor_transform = Transform3D();
  cursor_transform.origin = cursor_origin;
  cursor_transform.basis = node->get_basis_with_orthogonal_index(cursor_rot);
  cursor_transform.basis *= node->get_cell_scale();
  cursor_transform = node->get_global_transform() * cursor_transform;

  if (selected_palette >= 0)
  {
    if (node && !node->get_mesh_library().is_null())
    {
      cursor_transform *= node->get_mesh_library()
                              ->get_item_mesh_transform(selected_palette);
    }
  }
  ....
}

Analyzer warning:

V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 246, 251. grid_map_editor_plugin.cpp 246

It's very strange to dereference a pointer and then check it a few lines down. Perhaps the dereference was introduced later in the code than the check, and the developer didn't notice the check below.

Similar triggers:

• V595 The 'p_ternary_op->true_expr' pointer was utilized before it was verified against nullptr. Check lines: 4518, 4525. gdscript_analyzer.cpp 4518

• V595 The 'p_parent' pointer was utilized before it was verified against nullptr. Check lines: 4100, 4104. node_3d_editor_plugin.cpp 4100

• V595 The 'item' pointer was utilized before it was verified against nullptr. Check lines: 950, 951. project_export.cpp 950

• V595 The 'title_bar' pointer was utilized before it was verified against nullptr. Check lines: 1153, 1163. editor_node.cpp 1153

• V595 The 'render_target' pointer was utilized before it was verified against nullptr. Check lines: 2121, 2132. rasterizer_canvas_gles3.cpp 2121

• V595 The '_p' pointer was utilized before it was verified against nullptr. Check lines: 228, 231. dictionary.cpp 228

• V595 The 'class_doc' pointer was utilized before it was verified against nullptr. Check lines: 1215, 1231. extension_api_dump.cpp 1215

Fragment N7

template <class T, class U = uint32_t,
          bool force_trivial = false, bool tight = false>
class LocalVector
{
  ....
public:
  operator Vector<T>() const
  {
    Vector<T> ret;
    ret.resize(size());
    T *w = ret.ptrw();
    memcpy(w, data, sizeof(T) * count);
    return ret;
  }
  ....
};

Analyzer warning:

V780 Instantiation of LocalVector < AnimationCompressionDataState >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

Interesting fragment. The class template LocalVector made a conversion operator into a class Vector. With this transformation, you need to copy the contents of the current vector to the new one. To do this, use the function memcpy.

Everything is quite good, as long as it's a template type T trivially copyableHowever, the analyzer found different specializations LocalVectorin which this property is violated. As an example, consider the specialization LocalVector:

struct AnimationCompressionDataState
{
  uint32_t components = 3;
  LocalVector<uint8_t> data; // Committed packets.
  struct PacketData
  {
    int32_t data[3] = { 0, 0, 0 };
    uint32_t frame = 0;
  };

  float split_tolerance = 1.5;

  LocalVector<PacketData> temp_packets;

  // used for rollback if the new frame does not fit
  int32_t validated_packet_count = -1;
  ....
};

Class AnimationCompressionDataState contains LocalVectorwho himself non-trivially copyable.

For this case, the documentation on memcpy there is a clarification: “If the objects are potentially-overlapping or not TriviallyCopyable, the behavior of memcpy is not specified and may be undefined.”

It won't be difficult to fix the code, just replace the call memcpy on std::uninitialized_copy:

operator Vector<T>() const
{
  Vector<T> ret;
  ret.resize(size());
  T *w = ret.ptrw();
  std::uninitialized_copy(data, data + count, w);
  return ret;
}

PVS-Studio found 38 more dangerous specializations, but I will not provide a full list of them, of course:

• V780 Instantiation of LocalVector < AnimationCompressionDataState >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

• V780 Instantiation of LocalVector < LocalVector >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

• V780 Instantiation of LocalVector < Mapping, uint32_t, bool, bool >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

• V780 Instantiation of LocalVector < OAHashMap < uint64_t, Specialization > >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

• V780 Instantiation of LocalVector < Pair < ​​StringName, StringName >, uint32_t, bool, bool >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

• …

Fragment N8

Possible violation of program logic:

Dictionary GDScriptSyntaxHighlighter::_get_line_syntax_highlighting_impl
                                                             (int p_line)
{
  const String &str = text_edit->get_line(p_line);
  ....
  if (   is_digit(str[non_op])
      || (   str[non_op] == '.' 
          && non_op < line_length 
          && is_digit(str[non_op + 1]) ) )
  {
    in_number = true;
  }
  ....
}

Analyzer warning:

V781 The value of the 'non_op' index is checked after it was used. Perhaps there is a mistake in program logic. gdscript_highlighter.cpp 370

Meaning non_op is first used as an index when accessing characters in the string, and only then is it checked to see if it is less than the length.

Please note the access to the line after checking. If non_op < line_lengththis does not mean that (non_op + 1) < line_length. Therefore, in str[non_op + 1] may occur beyond the line boundary. Especially considering that under the box String there are no null-terminated strings.

A correct check should look like this:

if (   is_digit(str[non_op])
    || (   str[non_op] == '.' 
        && non_op + 1 < line_length 
        && is_digit(str[non_op + 1]) ) )
{
  in_number = true;
}

Fragment N9

struct Particles
{
  ....
  int amount = 0;
  ....
};

void ParticlesStorage::_particles_update_instance_buffer(
  Particles *particles,
  const Vector3 &p_axis,
  const Vector3 &p_up_axis)
{
  ....
  uint32_t lifetime_split = ....;
  // Offset VBO so you render starting at the newest particle.
  if (particles->amount - lifetime_split > 0)
  {
    ....
  }
  ....
}

Analyzer warning:

V555 The expression 'particles->amount – lifetime_split > 0' will work as 'particles->amount != lifetime_split'. particles_storage.cpp 959

This example is interesting because, despite the analyzer not working quite correctly, it showed us a place where developers should pay attention.

If the difference between two unsigned variables is greater than zero, then in fact this expression is semantically equal to particles->amount != lifetime_split. The condition will be calculated as false only if these variables are equal. If the left operand is less than the right one, then wraparound overflow will occur, and the resulting expression will be greater than zero. If the left operand is greater than the right one, then the difference will be greater than zero.

However, something else is noteworthy here: both variables have the same rank, but different sign. According to the standard, the compiler is obliged to carry out implicit conversionsbefore performing the subtraction. And in this situation, the general type will be unsigned 32-bit int. And this can also add surprises if the left operand is a negative number.

The most correct check in the case of expressions of signed and unsigned types of the same rank will look like this:

if (particles->amount >= 0 && particles->amount > lifetime_split)

In fact, you and I have reinvented std::cmp_greaterintroduced in C++20, and starting with this version of the standard, one can write concise code:

if (std::cmp_greater(particles->amount, lifetime_split))

Fragment N10

void AnimationNodeStateMachineEditor::_delete_tree_draw()
{
  TreeItem *item = delete_tree->get_next_selected(nullptr);
  while (item) 
  {
    delete_window->get_cancel_button()->set_disabled(false);
    return;
  }
  delete_window->get_cancel_button()->set_disabled(true);
}

Analyzer warning:

V1044 Loop break conditions do not depend on the number of iterations. animation_state_machine_editor.cpp 693

Cycle while lasts exactly one iteration. Very similar to the pattern when you need to take only the first element from the container, and this is done using a loop for:

for (auto &&item : items)
{
  DoSomething(item);
  break;
}

Thus, there is no need to check whether the container contains the first element. IMHO, such code is rather confusing, because you expect something obviously unknown from the cycles final number of iterations.

In the fragment above there is a cycle while makes absolutely no sense. A simple design would suffice. if:

void AnimationNodeStateMachineEditor::_delete_tree_draw()
{
  TreeItem *item = delete_tree->get_next_selected(nullptr);
  if (item)
  { 
    delete_window->get_cancel_button()->set_disabled(false);
    return;
  }
  
  delete_window->get_cancel_button()->set_disabled(true);
}

Fragment N11

static const char *script_list[][2] = {
  ....
  { "Myanmar / Burmese", "Mymr" },
  { "​Nag Mundari", "Nagm" },
  { "Nandinagari", "Nand" },
  ....
}

The reader may ask: “What's wrong here?” We ourselves would not have understood if the diagnostic rule had not been triggered V1076. What's interesting is that this is the first warning we've written. The diagnostic rule checks the program text for invisible symbols. Such symbols are a kind of bookmarks that the programmer may not see due to the text display settings in the development environment, but the compiler sees and processes them perfectly well.

Analyzer warning:

V1076 Code contains invisible characters that may alter its logic. Consider enabling the display of invisible characters in the code editor. locales.h 1114

Let's take a close look at the next line:

{ "​Nag Mundari", "Nagm" },

It is there that the bookmark with the invisible symbol is contained. If you use a hex editor, you will notice the following:

Between double quote and symbol N 3 bytes got lost: E2, 80 And 8B. They correspond to the Unicode character ZERO WIDTH SPACE (U+200B).

Most likely a bookmark contributed accidentally as a result of copying the name from some site. It is enough to simply remove this symbol from the string literal.

Fragment N12

void MeshStorage::update_mesh_instances() 
{
  ....
  uint64_t mask = RS::ARRAY_FORMAT_VERTEX | RS::ARRAY_FORMAT_NORMAL 
                | RS::ARRAY_FORMAT_VERTEX;
  ....
}

Analyzer warnings:

• V501 There are identical sub-expressions 'RenderingServer::ARRAY_FORMAT_VERTEX' to the left and to the right of the '|' operator. mesh_storage.cpp 1414

• V578 An odd bitwise operation detected. Consider verifying it. mesh_storage.cpp 1414

Strange initialization of the bit mask. It is written twice RS::ARRAY_FORMAT_VERTEXalthough they may have wanted to record some other flag.

Same trigger:

• V501 There are identical sub-expressions 'RenderingServer::ARRAY_FORMAT_VERTEX' to the left and to the right of the '|' operator. mesh_storage.cpp 1300

• V578 An odd bitwise operation detected. Consider verifying it. mesh_storage.cpp 1300

Fragment N13

void Image::initialize_data(int p_width, int p_height, bool p_use_mipmaps,
                            Format p_format, const Vector<uint8_t> &p_data)
{
  ....
  ERR_FAIL_COND_MSG(p_width > MAX_WIDTH, "The Image width specified (" + 
                                         itos(p_width) +
                                         " pixels) cannot be greater than " +
                                         itos(MAX_WIDTH) +
                                         " pixels.");

  ERR_FAIL_COND_MSG(p_height > MAX_HEIGHT, "The Image height specified (" +
                                           itos(p_height) +
                                           " pixels) cannot be greater than " +
                                           itos(MAX_HEIGHT) +
                                           " pixels.");

  ERR_FAIL_COND_MSG(p_width * p_height > MAX_PIXELS,
                   "Too many pixels for image, maximum is " + itos(MAX_PIXELS));
  ....
}

Analyzer warning:

V1083 Signed integer overflow is possible in 'p_width * p_height' arithmetic expression. This leads to undefined behavior. Left operand is in range '[0x1..0x1000000]', right operand is in range '[0x1..0x1000000]'. image.cpp 2200

So we have two variables p_width And p_height type int. The maximum value that a 4-byte array can store intequals 2'147'483'647.

First, the code checks that p_width <= MAX_WIDTHWhere MAX_WIDTH == 16'777'216. Then it is checked that p_height <= MAX_HEIGHTWhere MAX_HEIGHT == 16,777,216In the third check we compare that the product p_width * p_height <= MAX_PIXELS.

Let's look at a situation where p_width == p_height && p_width == 16'777'216. The result of multiplying these two numbers is 281'474'976'710'656. In order to display such a result, an 8-byte number is required, i.e. there is a signed overflow. And, as is known, in the C and C++ languages ​​this leads to undefined behavior.

If there are no helper functions that check for overflow, then the simplest fix might look like this:

ERR_FAIL_COND_MSG((int64_t) p_width * (int64_t) p_height > (int64_t) MAX_PIXELS,
                  "Too many pixels for image, maximum is " + itos(MAX_PIXELS));

Fragment N14

void RemoteDebugger::debug(....)
{
  ....
  mutex.lock();
  while (is_peer_connected())
  {
    mutex.unlock();
    ....
  }

  send_message("debug_exit", Array());
  if (Thread::get_caller_id() == Thread::get_main_id())
  {
    if (mouse_mode != Input::MOUSE_MODE_VISIBLE)
    {
      Input::get_singleton()->set_mouse_mode(mouse_mode);
    }
  } 
  else 
  {
    MutexLock mutex_lock(mutex);
    messages.erase(Thread::get_caller_id());
  }
}

V1020 The function exited without calling the 'mutex.unlock' function. Check lines: 556, 438. remote_debugger.cpp 556

A very interesting fragment with multi-threaded execution. The PVS-Studio analyzer found that on some execution paths the mutex may not be unlocked. Let's figure it out.

We need to start with what type of mutex is used:

class RemoteDebugger : public EngineDebugger
{
  ....
private:
  // Make handlers and send_message thread safe.
  Mutex mutex;
  ....
};

Let's dig a little deeper to see what this is. Mutex:

template <class StdMutexT>
class MutexImpl
{
  friend class MutexLock<MutexImpl<StdMutexT>>;
  using StdMutexType = StdMutexT; 
  mutable StdMutexT mutex;
public:
  _ALWAYS_INLINE_ void lock() const { mutex.lock(); }

  _ALWAYS_INLINE_ void unlock() const { mutex.unlock(); }

  _ALWAYS_INLINE_ bool try_lock() const { return mutex.try_lock(); }
};

// Recursive, for general use
using Mutex = MutexImpl<THREADING_NAMESPACE::recursive_mutex>;

So, in fact, we have not a regular mutex, but a recursive one. It is used together with a custom RAII wrapper:

template <class MutexT>
class MutexLock
{
  friend class ConditionVariable;

  std::unique_lock<typename MutexT::StdMutexType> lock;

public:
  _ALWAYS_INLINE_ explicit MutexLock(const MutexT &p_mutex)
    : lock(p_mutex.mutex) {}
};

Mutex almost everywhere RemoteDebugger::mutex used together with RAII wrappers, I'll show just a couple of places: [1], [2], [3]….

However, something went wrong in one place. The analyzer pointed to a place where the mutex is handled manually. Because of this, we have several different code execution options:

1. The mutex is locked, the loop is never executed (N == 0). As a result, the control flow will leave the function. RemoteDebugger::debug with the capture counter increased by 1.

2. Mutex is locked, loop is executed N == 1 times. In this case, everything will be fine – the recursive mutex acquisition counter increases and decreases by the same number.

3. Mutex is locked, loop is executed N > 1 times. As a result, the recursive mutex's acquisition counter will decrease by N – 1 relative to the moment before it was manually locked, which may lead to undefined behavior.

If you examine the function calls is_peer_connected by code base ([1], [2], [3]….), then in all cases they occur under blocking RemoteDebugger::mutex. Apparently, the programmer needed a lock in this case as well, but he implemented it manually.

Based on these assumptions, the code can be corrected in the following way:

void RemoteDebugger::debug(....)
{
  ....
  const auto is_peer_connected_sync = [this]
  {
    MutexLock _ { mutex };
    return is_peer_connected();
  };

  while (is_peer_connected_sync())
  {
    ....
  }
  ....
}

I can't guarantee that the fix is ​​absolutely correct, since only the Godot developers know what's supposed to happen here. But at least now we've gotten rid of the potential undefined behavior associated with unlocking the mutex on each loop iteration.

Conclusion

There are a wide variety of errors in code: from simple to complex, from noticeable to imperceptible. In order not to spoil the pleasure and impression of the product, it must be constantly cleared of bugs and errors. Static and dynamic analyzers cope well with this task.

It is easier to start using such solutions than it may seem. For example, you can get a trial version of the PVS-Studio analyzer Here. There are also various free scripts its use.

Thank you all for reading and have a nice day!

If you would like to share this article with an English-speaking audience, please use the translation link: Alexey Smolskas. How to find a job for Rescue Rangers: analyzing Godot Engine.