Checking the source code of 7-Zip using PVS-Studio / PVS-Studio Blog

One of the programs that allows us to solve the problem of data compression is the popular file archiver 7-Zip, I often use it myself. Readers have long asked us to check the code for this application. Well, it's time to look into its sources and see what PVS-Studio can find interesting.

Introduction

A few words about the project. 7-Zip - a free file archiver with a high degree of data compression, written in C and C ++. It has a small size of 235 thousand lines of code. It supports several compression algorithms and many data formats, including its own 7z format with a highly efficient LZMA compression algorithm. The program has been developed since 1999, it is free and has open source code. 7-Zip is the winner of the 2007 SourceForge.net Community Choice Awards in the categories of "Best Project" and "Best Technical Design". For verification, version 16.00 was selected, the source code of which was downloaded at http://www.7-zip.org/download.html

Validation Results

To check the 7-Zip code, we used the PVS-Studio v6.04 static code analyzer . For the article, the most interesting analyzer messages were selected and analyzed. Let's look at them.

Typos in conditional statements

Typos in conditional statements are common in programs. In the case of a large number of checks, their detection can be a lot of trouble. In such cases, a static code analyzer comes to the rescue.

I will give some examples of this error.

V501 There are identical sub-expressions 'Id == k_PPC' to the left and to the right of the '||' operator. 7zupdate.cpp 41

void SetDelta()
{
  if (Id == k_IA64)
    Delta = 16;
  else if (Id == k_ARM || Id == k_PPC || Id == k_PPC)    //<==
    Delta = 4;
  else if (Id == k_ARMT)
    Delta = 2;
  else
    Delta = 0;
}

The analyzer detected the same conditional expressions. In the best case, one of the conditions Id == k_PPC is redundant and does not affect the logic of the program. To correct a typo, you just need to remove this condition, then the correct expression will look like this:

if (Id == k_IA64)
  Delta = 16;
else if (Id == k_ARM || Id == k_PPC)
  Delta = 4;

But more serious consequences of such a typo are possible, if instead of the constant k_PPC , in one of the repeating conditions, there should be another constant. In this case, the logic of the program may be violated.
Here's another example of a typo in a conditional statement:
V501 There are identical sub-expressions to the left and to the right of the '||' operator: offs> = nodeSize || offs> = nodeSize hfshandler.cpp 915

HRESULT CDatabase::LoadCatalog(....)
{
  ....
  UInt32 nodeSize = (1 << hr.NodeSizeLog);
  UInt32 offs = Get16(p + nodeOffset + nodeSize - (i + 1) * 2);
  UInt32 offsNext = Get16(p + nodeOffset + nodeSize - (i + 2) * 2);
  UInt32 recSize = offsNext - offs;
  if (offs >= nodeSize
           || offs >= nodeSize    //<==
           || offsNext < offs
           || recSize < 6)
    return S_FALSE;
  ....
}

Here the problem is in the repeating condition offs> = nodeSize .

Most likely, the typos resulted in using Copy-Paste to duplicate the code. It makes no sense to urge to refuse to copy sections of code. It is too convenient and useful to deprive yourself of such functionality in the editor. You just need to more carefully check the result.

Identical comparisons

The analyzer detected a potential error in a construct consisting of conditional statements. Here is her example:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 388, 390. archivecommandline.cpp 388

static void AddRenamePair(...., NRecursedType::EEnum type, ....)
{
  ....
  if (type == NRecursedType::kRecursed)
    val.AddAscii("-r");
  else if (type == NRecursedType::kRecursed)    //<==
    val.AddAscii("-r0");
  ....
}

In the code, NRecursedType is defined as follows:

namespace NRecursedType { 
  enum EEnum {
    kRecursed,
    kWildcardOnlyRecursed,
    kNonRecursed
  };
}

It turns out that the second condition will never be fulfilled. Let's try to understand this problem in more detail. Based on the description of the command line options, the -r option talks about using recursion for subdirectories. In the case of the -r0 parameter , recursion is used only for wildcard names. Comparing this with the definition of NRecursedType, we can conclude that in the second case, the type NRecursedType :: kWildcardOnlyRecursed should be used . Then the correct code will look like this:

static void AddRenamePair(...., NRecursedType::EEnum type, ....)
{
  ....
  if (type == NRecursedType::kRecursed)
    val.AddAscii("-r");
  else if (type == NRecursedType::kWildcardOnlyRecursed)    //<==
    val.AddAscii("-r0");
  ....
}

Conditions that are always true or false

You must carefully monitor whether you are working with a signed or unsigned type. Ignoring these features can lead to unpleasant consequences.

V547 Expression 'newSize <0' is always false. Unsigned type value is never <0. update.cpp 254

This is an example of code from a program in which this language feature was ignored:

STDMETHODIMP COutMultiVolStream::SetSize(UInt64 newSize)
{
  if (newSize < 0)    //<==
    return E_INVALIDARG;
  ....
}

The problem is that newSize is unsigned and the condition will never be met. If a negative value is passed to the SetSize function , this error will be ignored and the function will start using the wrong size. In 7-Zip, there were 2 more conditions that, due to confusion with signed / unsigned, are always true or always false.

• V547 Expression 'rec.SiAttr.SecurityId> = 0' is always true. Unsigned type value is always> = 0. ntfshandler.cpp 2142
• V547 Expression 's.Len ()> = 0' is always true. Unsigned type value is always> = 0. xarhandler.cpp 258

The same condition is checked twice

The analyzer detected a potential error due to the fact that the same condition is checked twice.

V571 Recurring check. The 'if (Result! = ((HRESULT) 0L))' condition was already verified in line 56. extractengine.cpp 58

This is how the code fragment looks like:

void Process2()
{
  ....
  if (Result != S_OK)
  {
    if (Result != S_OK)    //<==
      ErrorMessage = kCantOpenArchive;
    return;
  }
  ....
} 

Most likely, in this situation, the second check is simply redundant, but a situation is possible in which the programmer after copying did not change the second condition, and it turned out to be erroneous.

More similar places in the 7-Zip code:

• V571 Recurring check. The '! QuoteMode' condition was already verified in line 18. stringutils.cpp 20
• V571 Recurring check. The 'IsVarStr (params [1], 22)' condition was already verified in line 3377. nsisin.cpp 3381

Suspicious work with pointers

There is also an error in the 7-Zip code when the pointer is dereferenced at the beginning, and only then it is checked for equality to zero.

V595 The 'outStreamSpec' pointer was utilized before it was verified against nullptr. Check lines: 753, 755. lzmaalone.cpp 753.

This is a very common mistake in all programs. It usually arises due to inattention in the process of code refactoring. Accessing the null pointer will lead to undefined program behavior. Consider an application code fragment containing an error of this type:

static int main2(int numArgs, const char *args[])
{
  ....
  if (!stdOutMode)
    Print_Size("Output size: ", outStreamSpec->ProcessedSize);   //<==
  if (outStreamSpec)    //<==
  {
    if (outStreamSpec->Close() != S_OK)
      throw "File closing error";
  }
  .... 
}

The outStreamSpec pointer is dereferenced in the outStreamSpec-> ProcessedSize expression . Then it is checked for equality to zero. You need to either check the pointer even higher or the check that occurs below is pointless. Here is a list of such potentially problematic places in the program code:

• V595 The '_file' pointer was utilized before it was verified against nullptr. Check lines: 2099, 2112. bench.cpp 2099
• V595 The 'ai' pointer was utilized before it was verified against nullptr. Check lines: 204, 214. updatepair.cpp 204
• V595 The 'options' pointer was utilized before it was verified against nullptr. Check lines: 631, 636. zipupdate.cpp 631
• V595 The 'volStreamSpec' pointer was utilized before it was verified against nullptr. Check lines: 856, 863. update.cpp 856

Exception inside the destructor

If an exception occurs in the program, the stack minimizes, during which the objects are destroyed by calling destructors. If the destructor of the object being destroyed when the stack collapses throws another exception and the destructor leaves this exception, the C ++ library immediately terminates the program by calling the terminate () function . It follows that destructors should never throw exceptions. An exception thrown inside the destructor must be handled inside the same destructor.

The analyzer issued the following message:

V509 The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal. consoleclose.cpp 62

And here is what the destructor throwing the exception looks like:

CCtrlHandlerSetter::~CCtrlHandlerSetter()
{
  #if !defined(UNDER_CE) && defined(_WIN32)
  if (!SetConsoleCtrlHandler(HandlerRoutine, FALSE))
    throw "SetConsoleCtrlHandler fails";    //<==
  #endif
}

Message V509 warns that if the CCtrlHandlerSetter object is destroyed during exception handling, a new exception will lead to an immediate program crash. This code should be rewritten in such a way as to report an error that occurred in the destructor without using the exception mechanism. If the error is not critical, then it can be ignored.

CCtrlHandlerSetter::~CCtrlHandlerSetter()
{
  #if !defined(UNDER_CE) && defined(_WIN32)
  try
  {
    if (!SetConsoleCtrlHandler(HandlerRoutine, FALSE))
      throw "SetConsoleCtrlHandler fails";    //<==
  }
  catch(...)
  {
    assert(false);
  }
  #endif
}

Bool variable increment

Historically, for variables of type bool the increment operation is valid, it sets the value of the variable to true . This feature is related to the fact that earlier integer values ​​were used to represent Boolean variables. Subsequently, this opportunity remained to support backward compatibility of programs. Starting with the C ++ 98 standard, it is marked as deprecated and is not recommended for use. In the upcoming C ++ 17 standard, the possibility of using an increment for a boolean variable is marked for deletion.

In the 7-Zip code, a couple of places were found where this obsolete feature is used.

• V552 A bool type variable is being incremented: numMethods ++. Perhaps another variable should be incremented instead. wimhandler.cpp 308
• V552 A bool type variable is being incremented: numMethods ++. Perhaps another variable should be incremented instead. wimhandler.cpp 318

STDMETHODIMP CHandler::GetArchiveProperty(....)
{
  ....
  bool numMethods = 0;
  for (unsigned i = 0; i < ARRAY_SIZE(k_Methods); i++)
  {
    if (methodMask & ((UInt32)1 << i))
    {
      res.Add_Space_if_NotEmpty();
      res += k_Methods[i];
      numMethods++;    //<==
    }
  }
  if (methodUnknown != 0)
  {
    char temp[32];
    ConvertUInt32ToString(methodUnknown, temp);
    res.Add_Space_if_NotEmpty();
    res += temp;
    numMethods++;    //<==
  }
  if (numMethods == 1 && chunkSizeBits != 0)
  {
    ....
  }
  ....
}

In this situation, two options are possible. Or, the variable numMethods is a flag, in which case it is better to use initialization with the boolean value numMethods = true . Or, judging by the name of the variable, this is a counter that must be an integer.

Check for failed memory allocation

The analyzer detected a situation where the value of the pointer returned by the new operator is compared to zero. As a rule, this means that the program, if it is impossible to allocate memory, will behave differently than the programmer expects.

V668 There is no sense in testing the 'plugin' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. far.cpp 399

Here's what it looks like in program code:

static HANDLE MyOpenFilePluginW(const wchar_t *name)
{
  ....
  CPlugin *plugin = new CPlugin(
    fullName,
    // defaultName,
    agent,
    (const wchar_t *)archiveType
    );
    if (!plugin)
      return INVALID_HANDLE_VALUE;
    ....
  }

If the new operator could not allocate memory, then according to the C ++ language standard, an exception std :: bad_alloc () is thrown . Thus, it does not make sense to check for equality to zero. The plugin pointer will never be zero. The function will never return the constant value INVALID_HANDLE_VALUE . If it is impossible to allocate memory, an exception occurs that is best handled at a higher level, and the check for equality to zero can be simply deleted. Well, or if exceptions in the application are undesirable, then you can use the new operator that does not throw exceptions, in this case you can check the return value to zero. In the application code, there were 3 more similar checks:

• V668 There is no sense in testing the 'm_Formats' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. enumformatetc.cpp 46
• V668 There is no sense in testing the 'm_States' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. bzip2decoder.cpp 445
• V668 There is no sense in testing the 'ThreadsInfo' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. bzip2encoder.cpp 170

Structures requiring optimization

Now a little about places that can potentially be optimized. An object is passed to the function. This object is passed by value, but is not modified, since there is a const keyword . Perhaps it would be rational to pass it using a constant reference in C ++ or using a pointer in C.

Here is an example for a vector:

V801 Decreased performance. It is better to redefine the first function argument as a reference. Consider replacing 'const ... pathParts' with 'const ... & pathParts'. wildcard.cpp 487

static unsigned GetNumPrefixParts(const UStringVector pathParts)
{
  ....
}

When this function is called, the copy constructor for the UStringVector class will be called . If such copying of objects occurs frequently, then this can significantly reduce the performance of the application. This code can be easily optimized by adding a link:

static unsigned GetNumPrefixParts(const UStringVector& pathParts)
{
  ....
}

Here are some more similar places:

• V801 Decreased Performance. It is better to redefine the first function argument as a reference. Consider replacing 'const ... props' with 'const ... & props'. benchmarkdialog.cpp 766
• V801 Instantiate CRecordVector <CAttribIconPair>: Decreased performance. It is better to redefine the first function argument as a reference. Consider replacing 'const ... item' with 'const ... & item'. yvector.h 199

Conclusion

7-Zip is a small project that has been developing for a long time and of course we could not find a large number of serious errors. But there are still places in the code that you need to pay attention to and the PVS-Studio static code analyzer will greatly facilitate this work. If you are developing a project in C, C ++ or C #, I suggest that you immediately download PVS-Studio and check your project: http://www.viva64.com/en/pvs-studio-download/

If you want to share this article with an English-speaking audience, then please use the translation link: Kirill Yudintsev. Checking 7-Zip with PVS-Studio analyzer .