Analyzing the Dolphin-emu project

Feb 24 2012

Author: Andrey Karpov

Introduction
Misprints
Low-level memory handling errors
Handling float
String violence
Conclusion

We are regularly asked to check various open-source projects with the PVS-Studio analyzer. If you want to offer some project for us to analyze too, please follow this link. Another project we have checked is Dolphin-emu.

Introduction

Dolphin-emu is a Gamecube and Wii emulator. Since this is an open-source project, anyone can introduce modifications into it. The code can be found on GitHub.

We have found quite few errors in the project. First of all, this is because of its small size: it is about 260 000 code lines. The rest of the project (1340 000 code lines) is comprised by third-party libraries which are not so much interesting to test.

Though there are few errors, certain code fragments are worth being told about in the article. What the other unsafe code fragments is concerned, the developers can examine them themselves using the trial version of PVS-Studio.

Misprints

Misprints are insidious and they can be found in any application. Programmers are sure that they make only complicated mistakes and that analyzers should look for memory leaks and synchronization errors first of all. Unfortunately, reality is much more trivial: the most widespread errors are misprints and copy-paste mistakes. For example, there is this function in Dolphin-emu:

bool IRBuilder::maskedValueIsZero(
  InstLoc Op1, InstLoc Op2) const
{
  return (~ComputeKnownZeroBits(Op1) &
          ~ComputeKnownZeroBits(Op1)) == 0;
}

PVS-Studio's diagnostic message:

V501 There are identical sub-expressions '~ComputeKnownZeroBits(Op1)' to the left and to the right of the '&' operator. Core ir.cpp 1215

The misprint in this code causes the 'Op1' variable to be used twice, while the 'Op2' variable is not used at all. Here is another sample where a closing parenthesis ')' is in a wrong place.

static const char iplverPAL[0x100] = "(C) 1999-2001 ....";
static const char iplverNTSC[0x100]= "(C) 1999-2001 ....";
CEXIIPL::CEXIIPL() : ....
{
  ...
  memcpy(m_pIPL, m_bNTSC ? iplverNTSC : iplverPAL,
         sizeof(m_bNTSC ? iplverNTSC : iplverPAL));
  ...
}

PVS-Studio's diagnostic message:

V568 It's odd that the argument of sizeof() operator is the 'm_bNTSC ? iplverNTSC : iplverPAL' expression. Core exi_deviceipl.cpp 112

The expression inside the sizeof() operator is not calculated. This code works only because the types of the 'iplverNTSC' and 'iplverPAL' arrays coincide. It appears that sizeof() always returns 0x100. This is an interesting thing: if the sizes of the 'iplverNTSC' and 'iplverPAL ' arrays were different, the code would work quite differently. Let's examine the test sample to make it clear:

const char A[10] = "123";
const char B[10] = "123";
const char C[20] = "123";
cout << sizeof(true ? A : B) << ", "
     << sizeof(true ? A : C) << endl;

This is the result of program execution: 10, 4.

In the first case the array's size is printed, and in the second the size of pointer.

Low-level memory handling errors

Besides misprints there are many errors of handling such functions as memset() and memcpy().

u32 Flatten(..., BlockStats *st, ...)
{
  ...
  memset(st, 0, sizeof(st));
  ...
}

PVS-Studio's diagnostic message:

V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. Core ppcanalyst.cpp 302

It is the size of the pointer to an object which is accidentally calculated instead of the size of the BlockStats object itself. The correct code is: sizeof(*st).

Here is another similar situation:

void drawShadedTexSubQuad(...,
  const MathUtil::Rectangle<float>* rDest, ...)
{
  ...
  if (stsq_observer ||
      memcmp(rDest,
        &tex_sub_quad_data.rdest, sizeof(rDest)) != 0 ||
      tex_sub_quad_data.u1 != u1 ||
      tex_sub_quad_data.v1 != v1 ||
      tex_sub_quad_data.u2 != u2 ||
      tex_sub_quad_data.v2 != v2 ||
      tex_sub_quad_data.G != G)
  ...
}

Only a part of the structure is participating in comparison. The correct code is this: sizeof(*rDest).

Handling float

In some fragments of the Dolphin-emu project, handling variables of the float types looks too optimistic. Operators == and != are used to compare float-variables. Such comparisons are admissible only in certain cases. To know more about comparison of float-variables see the documentation on the V550 diagnostic rule. Consider the following sample:

float Slope::GetValue(float dx, float dy)
{
  return f0 + (dfdx * dx) + (dfdy * dy);
}

void BuildBlock(s32 blockX, s32 blockY)
{
  ...
  float invW = 1.0f / WSlope.GetValue(dx, dy);
  ...
  float q = TexSlopes[i][2].GetValue(dx, dy) * invW;
  if (q != 0.0f)
    projection = invW / q;
  ...
}

PVS-Studio's diagnostic message:

V550 An odd precise comparison: q != 0.0f. It's probably better to use a comparison with defined precision: fabs(A - B) > Epsilon. VideoSoftware rasterizer.cpp 264

Note the "if (q != 0.0f)" comparison. As you can see, the 'q' variable is calculated in a rather complicated way. As a consequence, it is almost improbable that it is CERTAINLY equal to zero. The variable will most likely get some value like 0.00000002, for instance. It is not 0, but division by such a small number might cause an overflow. A special check for such cases is needed.

String violence

void CMemoryWindow::onSearch(wxCommandEvent& event)
{
  ...
  //sprintf(tmpstr, "%s%s", tmpstr, rawData.c_str());
  //strcpy(&tmpstr[1], rawData.ToAscii());
  //memcpy(&tmpstr[1], &rawData.c_str()[0], rawData.size());
  sprintf(tmpstr, "%s%s", tmpstr, (const char *)rawData.mb_str());
  ...
}

You can see from the commented code that this is a weak point. This is already a fourth attempt to form a string. Unfortunately, it is far from being ideal, too. The PVS-Studio analyzer warns us:

V541 It is dangerous to print the string 'tmpstr' into itself. Dolphin memorywindow.cpp 344

What is dangerous about it is that the "tmpstr" string is printed into itself. This code can work correctly but you'd better not do it that way. Depending on how the sprintf() function is implemented, you may unexpectedly get an incorrect result. Consider using the strcat() function instead.

There are other code fragments that work well but are potentially dangerous:

V541 It is dangerous to print the string 'pathData_bin' into itself. Dolphin wiisavecrypted.cpp 513

V541 It is dangerous to print the string 'regs' into itself. Core interpreter.cpp 84

V541 It is dangerous to print the string 'fregs' into itself. Core interpreter.cpp 89