VirtualDub Check

Just now, I sat down and checked the VirtualDub project with PVS-Studio. The choice was random. I think the most important thing is to regularly check / recheck various projects to show how the PVS-Studio code analyzer is developing. And what project will be tested is not so important. Errors are everywhere. We already checked the VirtualDub project in 2011, but then almost nothing interesting was found. So I decided to see how things are, after 2 years.



I downloaded the VirtualDub-1.10.3-src.7z archive from the VirtualDub website. For analysis, I used PVS-Studio version 5.10. I spent about an hour on the analysis, so do not judge strictly. Surely, I missed something. Conversely, he could consider the correct code as suspicious. I ask those who support the VirtualDub project not to rely on my report, but to make an independent verification. We always go to the open-source community and are ready to allocate the registration key.
')
I also want to ask Avery Lee to treat this article with understanding. Last time, he very painfully perceived the mention of VirtualDub in one of my articles. I didn’t want to, and I don’t want to say that any program is buggy. Software bugs are everywhere . My goal is to show the benefits that static code analysis can provide. Along the way, this will make open-source projects a bit more reliable. It is wonderful.

Of course, one-time checks are ineffective. But with this, unfortunately, I can not do anything. Using or not using regular static analysis tools depends on the developers. I can only try to explain the benefits of regular use. Here is one of the interesting notes on this topic: Leo Tolstoy and static code analysis .

However, this article is about errors, but not the methodology of using static analysis. Let's see what new interesting things the PVS-Studio analyzer found in VirtualDub.

Virtual Destructors

In the C ++ programming language, the destructor of a polymorphic base class must be declared virtual. This is the only way to ensure the correct destruction of the derived class object through a pointer to the corresponding base class.

I know everyone knows that. However, this does not prevent to forget to declare the destructor virtual.

VirtualDub has a class VDDialogBaseW32:

class VDDialogBaseW32 { .... ~VDDialogBaseW32(); .... virtual INT_PTR DlgProc(....) = 0; virtual bool PreNCDestroy(); .... } 

As you can see, it contains virtual functions. However, the destructor is not declared virtual. Naturally, he inherited his classes. For example, VDDialogAudioFilterFormatConvConfig:

 class VDDialogAudioFilterFormatConvConfig : public VDDialogBaseW32 { .... }; 

Error deleting an object looks like this:

 INT_PTR CALLBACK VDDialogBaseW32::StaticDlgProc(....) { VDDialogBaseW32 *pThis = (VDDialogBaseW32 *)GetWindowLongPtr(hwnd, DWLP_USER); .... delete pThis; .... } 

A warning issued by PVS-Studio: VDDialogBaseW32 class contains virtual functions. VirtualDub gui.cpp 997

As you can see, the pointer to the base class is used to destroy the object. Such an object deletion will lead to undefined program behavior.

A similar problem exists with the VDMPEGAudioPolyphaseFilter class.

More about undefined behavior

There are no questions about errors related to virtual destructor. A much more slippery topic is shift operations. Here is one such example:

 void AVIVideoGIFOutputStream::write(....) { { .... for(int i=0; i<palsize; ++i) dict[i].mPrevAndLastChar = (-1 << 16) + i; .... } 

One may argue that this is a completely reliable code that has been used successfully for more than 10 years. However, all the same, here we are dealing with an undefined program behavior. Here is what the standard says about such constructions:

The shift operators << and >> group left-to-right.

shift-expression << additive-expression

shift-expression >> additive-expression

This is an integral part of this article.

1. The promoted left operand. If you are the right operand.

2. The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If it is an unsigned pattern, it can be reduced to a minimum. Otherwise, if it is a non-negative value, and if it is an E1 * 2 ^ E2, it is a representation of the result; otherwise, the behavior is undefined.

3. The value of E1 >> E2 is E1 right-shifted E2 bit positions. It is an insignia of the E1 / 2 ^ E2 . If E1 has a signed value

The fact that the code works is luck. When mastering a new compiler or other keys for optimization, the program may unexpectedly change its behavior. You can read more about shifts and whether or not to edit the code, in the article " Without knowing the ford, do not climb into the water. Part Three ".

Here is the complete list of places where the PVS-Studio analyzer detected Undefined behavior or Unspecified behavior in the VirtualDub project.

Typos

 static ModuleInfo *CrashGetModules(void *&ptr) { .... while(*pszHeap++); if (pszHeap[-1]=='.') period = pszHeap-1; .... } 

Diagnostic message issued by PVS-Studio: V529 Odd semicolon ';' after 'while' operator. VirtualDub crash.cpp 462

Notice the semicolon after the 'while'. Here either the error or the code is incorrectly formatted. I think this is a mistake. The cycle “while (* pszHeap ++);” will pass to the end of the line and as a result, the variable 'pszHeap' will point to memory after the terminal zero . The “if (pszHeap [-1] == '.')” Check does not make sense. At the address “pszHeap [-1]” there is always a terminal zero.

Consider another typo when working with strings.

 void VDBackfaceService::Execute(...., char *s) { .... if (*s == '"') { while(*s && *s != '"') ++s; } else { .... } 

Diagnostic message issued by PVS-Studio: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 183, 184. VirtualDub backface.cpp 183

This code should skip everything in quotes. At least, I think so. However, the condition (* s && * s! = '"') Is immediately false. Perhaps the code should look like this:

 if (*s == '"') { ++s; while(*s && *s != '"') ++s; } 

The new operator generates exceptions for memory allocation errors.

In the old code, it is often possible to meet the check that returned the new operator. It looks like this:

 int *p = new int[10]; if (!p) return false; 

Modern compilers that support the C ++ standard of the language should throw an exception if memory could not be allocated. It is possible to make the 'new' operator not generate exceptions, but this has nothing to do with the cases under consideration.

Thus, if (! P) is an extra check. In general, such a code is harmless. Excess check. Nothing wrong.
However, the old code can lead to unpleasant consequences. Consider a fragment from the project VirtualDub.

 void HexEditor::Find(HWND hwndParent) { .... int *next = new int[nFindLength+1]; char *searchbuffer = new char[65536]; char *revstring = new char[nFindLength]; .... if (!next || !searchbuffer || !revstring) { delete[] next; delete[] searchbuffer; delete[] revstring; return; } .... } 

Diagnostic message issued by PV-Studio: V668: it was not using the 'new' operator. The exception will be generated in the case of memory allocation error. VirtualDub hexviewer.cpp 2012

If an exception is raised in the string “char * revstring = new char [nFindLength];”, a memory leak will occur. Operators delete [] will not be called. Not a critical error, but still worth mentioning about it.

Here , all the places where the pointer is checked in VirtualDub after calling the 'new' operator are listed.

Link to the destroyed object

 vdlist_iterator& operator--(int) { vdlist_iterator tmp(*this); mp = mp->mListNodePrev; return tmp; } 

Diagnostic message issued by PVS-Studio: V558 Function returns the reference to the temporary local object: tmp. VirtualDub vdstl.h 460

The function is implemented incorrectly. It returns a reference to the local 'tmp' object. After exiting the function, it will be destroyed. Working with this link will result in undefined behavior.

By the way, the operator ++, located nearby, is implemented correctly.

At the beginning we use, then we check

In different programs, it is often possible to encounter an error, when the pointer is first dereferenced, and only then, compared with NULL. Such errors do not manifest themselves for a very long time, since the equality of the NULL pointer is an abnormal rare situation. These shortcomings also exist in the VirtualDub code. Example:

 LRESULT YUVCodec::DecompressGetFormat(BITMAPINFO *lpbiInput, BITMAPINFO *lpbiOutput) { BITMAPINFOHEADER *bmihInput = &lpbiInput->bmiHeader; BITMAPINFOHEADER *bmihOutput = &lpbiOutput->bmiHeader; LRESULT res; if (!lpbiOutput) return sizeof(BITMAPINFOHEADER); .... } 

Diagnostic message issued by PVS-Studio: V595 The "lpbiOutput" pointer was used against it. Check lines: 82, 85. VirtualDub yuvcodec.cpp 82

At the beginning, the lpbiOutput pointer is dereferenced. Then it is checked: "if (! LpbiOutput)". Such an error usually occurs during the refactoring process. New code is inserted before the necessary checks. To correct the above code, you need to change the sequence of actions:

 LRESULT YUVCodec::DecompressGetFormat(BITMAPINFO *lpbiInput, BITMAPINFO *lpbiOutput) { if (!lpbiOutput) return sizeof(BITMAPINFOHEADER); BITMAPINFOHEADER *bmihInput = &lpbiInput->bmiHeader; BITMAPINFOHEADER *bmihOutput = &lpbiOutput->bmiHeader; LRESULT res; .... } 

Other places where the analyzer issues a V595 warning are listed here .

Work with type HRESULT

 VDPosition AVIReadTunnelStream::TimeToPosition(VDTime timeInUs) { AVISTREAMINFO asi; if (AVIStreamInfo(pas, &asi, sizeof asi)) return 0; return VDRoundToInt64(timeInUs * (double)asi.dwRate / (double)asi.dwScale * (1.0 / 1000000.0)); } 

Diagnostic message issued by PVS-Studio: V545 Such conditional expression for AVRTreamInfoA (pas, & asi, sizeof asi) '. The SUCCEEDED or FAILED macro should be used instead. VirtualDub avireadhandlertunnelw32.cpp 230

The AVIStreamInfo () function returns a value of type HRESULT. This type cannot be interpreted as 'bool'. The information stored in a variable of type HRESULT has a rather complex structure . To check the HRESULT value, you must use the SUCCEEDED or FAILED macro declared in WinError.h. This is how these macros work:

 #define FAILED(hr) (((HRESULT)(hr)) < 0) #define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0) 

The correct code is:

 if (FAILED(AVIStreamInfo(pas, &asi, sizeof asi))) 

PVS-Studio issues similar warnings on the following lines:

• avireadhandlertunnelw32.cpp 238
• avireadhandlertunnelw32.cpp 335
• inputfileavi.cpp 440
• context_d3d11.cpp 959

Magic numbers

Setting the length of a string with a number is not a good idea. It is very easy to make a mistake by counting characters. Example:

 bool VDOpenGLBinding::Attach(....) { .... if (!memcmp(start, "GL_EXT_blend_subtract", 20)) .... } 

Diagnostic message issued by PVS-Studio: V512 A call of the 'memcmp' function will be "GL_EXT_blend_subtract". Riza opengl.cpp 393

The length of the string "GL_EXT_blend_subtract" is not 20, but 21 characters. The error is noncritical. In practice, there will be no collisions. However, such magic numbers should be avoided. It is better to use a special macro to calculate the length of the string. Example:

 #define LiteralStrLen(S) (sizeof(S) / sizeof(S[0]) - 1) 

In C ++, you can make a more secure template function:

 template <typename T, size_t N> char (&ArraySizeHelper(T (&array)[N]))[N]; template <typename T, size_t N> size_t LiteralStrLen(T (&array)[N]) { return sizeof(ArraySizeHelper(array)) - 1; } 

The advantage of the second option is that you cannot accidentally pass a simple pointer as an argument. This technique is described in more detail in the article " PVS-Studio vs Chromium ".

Absolute ways

 VDDbgHelpDynamicLoaderW32::VDDbgHelpDynamicLoaderW32() { hmodDbgHelp = LoadLibrary( "c:\\program files\\debugging tools for windows\\dbghelp"); if (!hmodDbgHelp) { hmodDbgHelp = LoadLibrary("c:\\program files (x86)\\...... .... } 

Diagnostic message issued by PVS-Studio: V631 Consider inspecting the 'LoadLibraryA' function call. It is considered a poor style. VirtualDub leaks.cpp 67, 69

I think it's clear why this code is bad. Of course, the code is associated with debugging, and hardly any negative impact on end users. But still, it would be better to get the right path to Program Files.

Invalid argument

 sint64 rva; void tool_lookup(....) { .... printf("%08I64x %s + %x [%s:%d]\n", addr, sym->name, addr-sym->rva, fn, line); .... } 

Diagnostic message issued by PVS-Studio: V576 Incorrect format. Consider checking the fourth argument of the printf function. The argument is expected to be not greater than 32-bit. Asuka lookup.cpp 56

The variable 'rva' is a 64-bit type. This means that 8 bytes will be pushed onto the stack. The printf () function is a function with a variable number of arguments . The type of data that it must process is specified using the format string. In this case, the 'rva' variable will be processed as a 32-bit variable ("% x").

This error will lead to failure or not depends on how the compiler organizes the transfer of arguments and on the bitness of the platform. For example, in Win64, all integer types are first converted to the 64-bit type, and then pushed onto the stack. Problems that the variable will take in a stack more places, than it is necessary, will not be.

However, if the variable 'rva' stores values ​​greater than INT_MAX, its value in any case will be printed incorrectly.

The analyzer issues similar warnings here:

• dubstatus.cpp 360
• lookup.cpp 58

Wrong comparisons

 void VDVideoCompressorVCM::GetState(vdfastvector<uint8>& data) { DWORD res; .... res = ICGetState(hic, data.data(), size); .... if (res < 0) throw MyICError("Video compression", res); } 

Diagnostic message issued by PVS-Studio: V547 Expression 'res <0' is always false. Unsigned type value is never <0. Riza w32videocodecpack.cpp 828

Variable 'res' we have unsigned type DWORD. This means that the expression "res <0" is always equal to the value 'false'.

A similar test is contained here: w32videocodec.cpp 284.

Consider another similar error.

 #define ICERR_CUSTOM -400L static const char *GetVCMErrorString(uint32 icErr) { .... if (icErr <= ICERR_CUSTOM) err = "A codec-specific error occurred."; .... } 

Diagnostic message issued by PVS-Studio: V605 Consider verifying the expression: icErr <= - 400L. An unsigned value is compared to the number -400. system error_win32.cpp 54

The variable 'icErr' is of type 'unsigned'. Therefore, before comparing, the number '-400' will be implicitly converted to 'unsigned'. The value of '-400' will turn into 4294966896. Thus, the comparison (icErr <= -400) is equivalent to (icErr <= 4294966896). Apparently, this is not what the programmer wanted.

Miscellaneous strange

 void AVIOutputFile::finalize() { .... if (stream.mChunkCount && hdr.dwScale && stream.mChunkCount) .... } 

Diagnostic message issued by PVS-Studio: V501 There are identical sub-expressions of the operator. VirtualDub avioutputfile.cpp 761

The variable 'stream.mChunkCount' is checked twice. One check is superfluous or you forgot to check something else.

 void VDVideoCompressorVCM::Start(const void *inputFormat, uint32 inputFormatSize, const void *outputFormat, uint32 outputFormatSize, const VDFraction& frameRate, VDPosition frameCount) { this->hic = hic; .... } 

Diagnostic message issued by PVS-Studio: V570 The 'this-> hic' variable is assigned to itself. Riza w32videocodecpack.cpp 253

 void VDDialogAudioConversionW32::RecomputeBandwidth() { .... if (IsDlgButtonChecked(mhdlg, IDC_PRECISION_NOCHANGE)) { if (mbSourcePrecisionKnown && mbSource16Bit) bps *= 2; else bps = 0; } if (IsDlgButtonChecked(mhdlg, IDC_PRECISION_16BIT)) bps *= 2; .... } 

Diagnostic message issued by PVS-Studio: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. VirtualDub optdlg.cpp 120

The code may be incorrectly formatted. Or perhaps the 'else' keyword is forgotten.

 bool VDCaptureDriverScreen::Init(VDGUIHandle hParent) { .... mbAudioHardwarePresent = false; mbAudioHardwarePresent = true; .... } 

Diagnostic message issued by PVS-Studio: V519 The 'mbAudioHardwarePresent' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 274, 275. VDCapture cap_screen.cpp 275

Conclusion

As you can see, even with a one-time start, static analysis can be useful. But it is much more useful to run it regularly. After all, warnings (warnings) in the compiler, programmers include not once before the release, but use them constantly. The same situation with the static analysis tools. Their constant use allows you to quickly eliminate errors. You can consider PVS-Studio as a kind of add-on for the compiler, which gives out more interesting warnings. The best option is to use incremental code analysis . You will find new errors immediately after compiling the modified files.