Just recently I've checked the VirtualDub project with PVS-Studio. This was a random choice. You see, I believe that it is very important to regularly check and re-check various projects to show users that the PVS-Studio analyzer is evolving, and which project you run it on doesn't matter that much - bugs can be found everywhere. We already checked the VirtualDub project in 2011, but we found almost nothing of interest then. So, I decided to take a look at it now, 2 years later.
I downloaded the archive VirtualDub-1.10.3-src.7z from the VirtualDub website. Analysis was performed by PVS-Studio 5.10. It took me just about one hour, so don't be strict with me. I surely must have missed something or, on the contrary, taken correct code fragments for incorrect ones. If you develop and maintain the VirtualDub project, please don't rely on my report - check it yourselves. We always help the open-source community and will grant you a registration key.
I'm also asking Avery Lee to get me right. Last time his reaction to my mentioning VirtualDub in one of the articles was pretty negative. I never mean to say about any program that it's buggy. Software errors can be found in every program. My goal is to show how useful the static code analysis technology can be. At the same time, it will help to make open-source projects a bit more reliable. And that's wonderful.
Single-time checks are of little use, of course. But that I cannot help, I'm afraid. Whether or not to use static analysis tools on a regular basis is up to developers. I can only try to explain why regular use is better. Here's one interesting post on the subject: Leo Tolstoy and static code analysis.
However, this article is about bugs, not the static analysis methodology. Let's find out if there is anything interesting PVS-Studio has found in VirtualDub.
In C++, the destructor of a polymorphic base class must be declared virtual - this is the only way to ensure correct destruction of a derived object through a pointer to the corresponding base class.
I know you know that. However, it still doesn't guarantee that you will never forget to declare the destructor virtual.
There is the class VDDialogBaseW32 in VirtualDub:
class VDDialogBaseW32 {
....
~VDDialogBaseW32();
....
virtual INT_PTR DlgProc(....) = 0;
virtual bool PreNCDestroy();
....
}As you can see, it contains virtual functions. The destructor, however, is not declared virtual. And, naturally, there are some classes inherited from it, for example VDDialogAudioFilterFormatConvConfig:
class VDDialogAudioFilterFormatConvConfig :
public VDDialogBaseW32
{ .... };Here's the object destruction error:
INT_PTR CALLBACK VDDialogBaseW32::StaticDlgProc(....) {
VDDialogBaseW32 *pThis =
(VDDialogBaseW32 *)GetWindowLongPtr(hwnd, DWLP_USER);
....
delete pThis;
....
}PVS-Studio's diagnostic message: V599 The destructor was not declared as a virtual one, although the 'VDDialogBaseW32' class contains virtual functions. VirtualDub gui.cpp 997
As you can see, a pointer to the base class is used to destroy the object. Doing it this way will cause an undefined behavior.
The same trouble is with the class VDMPEGAudioPolyphaseFilter.
It's all clear with bugs related to virtual destructors. Shift operations, however, is a more subtle subject. Take a look at the following example:
void AVIVideoGIFOutputStream::write(....) {
{
....
for(int i=0; i<palsize; ++i)
dict[i].mPrevAndLastChar = (-1 << 16) + i;
....
}However hard one may try to convince me that this is an absolutely safe code which has been working for a dozen years, I'll keep saying that we are still having an undefined behavior here. Let's see what the standard has to say about such constructs:
The shift operators << and >> group left-to-right.
shift-expression << additive-expression
shift-expression >> additive-expression
The operands shall be of integral or unscoped enumeration type and integral promotions are performed.
1. The type of the result is that of the promoted left operand. The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand.
2. The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If E1 has an unsigned type, the value of the result is E1 * 2^E2, reduced modulo one more than the maximum value representable in the result type. Otherwise, if E1 has a signed type and non-negative value, and E1*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.
3. The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.
That the code works correctly is sheer luck, and it may suddenly change its behavior once you have switched over to a new compiler or started using compiler switches for optimization. See the article "Wade not in unknown waters. Part three" for more information about shift operations and code fixing in such cases.
Here's the list of fragments of VirtualDub where PVS-Studio has detected Undefined behavior or Unspecified behavior.
static ModuleInfo *CrashGetModules(void *&ptr) {
....
while(*pszHeap++);
if (pszHeap[-1]=='.')
period = pszHeap-1;
....
}PVS-Studio's diagnostic message: V529 Odd semicolon ';' after 'while' operator. VirtualDub crash.cpp 462
Note the semicolon after 'while'. It's either a mistake or incorrect code formatting. It seems more like the first thing. The loop "while(*pszHeap++);" will reach the end of the line and result in the 'pszHeap' variable pointing to the memory area after the terminal null. The check "if (pszHeap[-1]=='.')" is meaningless: it's the terminal null which is always found at "pszHeap[-1]".
Here's another misprint when handling strings.
void VDBackfaceService::Execute(...., char *s) {
....
if (*s == '"') {
while(*s && *s != '"')
++s;
} else {
....
}PVS-Studio's diagnostic message: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 183, 184. VirtualDub backface.cpp 183
This code must skip everything enclosed in the quotes. At least, it seems to do so. However, the condition (*s && *s != '"') is false right away. Perhaps the code should look like this:
if (*s == '"') {
++s;
while(*s && *s != '"')
++s;
}In old code you can often see checks of values returned by the new operator:
int *p = new int[10];
if (!p)
return false;Contemporary C++ compilers conforming to the C++ standard must throw an exception when memory cannot be allocated. You can set the 'new' operator not to do this, but this is outside the scope of our article now.
Therefore, the check if (!p) is not necessary. This code is safe in general - just an odd check, that's all.
But old code fragments may do you much harm, too. Take a look at the fragment from VirtualDub below.
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;
}
....
}PVS-Studio's diagnostic message: V668 There is no sense in testing the 'next' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. VirtualDub hexviewer.cpp 2012
If an exception is thrown in the line "char *revstring = new char[nFindLength];", a memory leak will occur. The delete[] operators won't be called. This is not a critical error, but it is worth mentioning.
See the list of all the fragments of VirtualDub where a pointer is checked after calling the 'new' operator.
vdlist_iterator& operator--(int) {
vdlist_iterator tmp(*this);
mp = mp->mListNodePrev;
return tmp;
}PVS-Studio's diagnostic message: V558 Function returns the reference to temporary local object: tmp. VirtualDub vdstl.h 460
The function is implemented incorrectly: it returns a reference to the local object 'tmp'. After leaving the function, this object will have been destroyed already; handling that reference will cause an undefined behavior.
By the way, the ++ operator, standing nearby, is implemented correctly.
In various programs you can often see a bug when a pointer is first dereferenced and only then is checked for being NULL. These errors may stay hidden for a very long time because a pointer being null is a rare accident. VirtualDub also has a few of these. For example:
void VDTContextD3D9::Shutdown() {
....
mpData->mFenceManager.Shutdown();
....
if (mpData) {
if (mpData->mhmodD3D9)
FreeLibrary(mpData->mhmodD3D9);
....
}PVS-Studio's diagnostic message: V595 The 'mpData' pointer was utilized before it was verified against nullptr. Check lines: 1422, 1429. Tessa context_d3d9.cpp 1422
The pointer "mpData" gets first dereferenced and then checked: "if (mpData)". These errors usually occur during code refactoring: the new code is inserted before the necessary checks.
The other fragments which triggered the V595 diagnostic are listed here.
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));
}PVS-Studio's diagnostic message: V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'AVIStreamInfoA(pas, & asi, sizeof asi)'. The SUCCEEDED or FAILED macro should be used instead. VirtualDub avireadhandlertunnelw32.cpp 230
The function AVIStreamInfo() returns a HRESULT value. This type cannot be interpreted as 'bool'. Information stored in a variable of the HRESULT type has a pretty complex structure, and to check a HRESULT value one needs to use either the SUCCEEDED or FAILED macros declared in "WinError.h". This is how they are implemented:
#define FAILED(hr) (((HRESULT)(hr)) < 0)
#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)The fixed code should look like this:
if (FAILED(AVIStreamInfo(pas, &asi, sizeof asi)))The same warning is generated on the following lines:
It's not a good idea to declare a string length as a number. You may easily make a mistaken when counting the characters. For example:
bool VDOpenGLBinding::Attach(....) {
....
if (!memcmp(start, "GL_EXT_blend_subtract", 20))
....
}PVS-Studio's diagnostic message: V512 A call of the 'memcmp' function will lead to underflow of the buffer '"GL_EXT_blend_subtract"'. Riza opengl.cpp 393
The length of the string "GL_EXT_blend_subtract" is 21 characters, not 20. This error is not critical; no troubles usually occur in practice. But you'd still better avoid such magic numbers and rather use a special macro to count the string length. For example:
#define LiteralStrLen(S) (sizeof(S) / sizeof(S[0]) - 1)C++ allows you to create a template function which is safer:
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 method is that it doesn't allow you to accidentally pass a plain pointer as an argument. This method is described in detail in the article "PVS-Studio vs Chromium".
VDDbgHelpDynamicLoaderW32::VDDbgHelpDynamicLoaderW32()
{
hmodDbgHelp = LoadLibrary(
"c:\\program files\\debugging tools for windows\\dbghelp");
if (!hmodDbgHelp) {
hmodDbgHelp = LoadLibrary("c:\\program files (x86)\\......
....
}PVS-Studio's diagnostic message: V631 Consider inspecting the 'LoadLibraryA' function call. Defining an absolute path to the file or directory is considered a poor style. VirtualDub leaks.cpp 67, 69
I guess you understand what is bad about this code. It has to do with debugging of course and doesn't seem to affect the end users in any way, but it's still better to get a correct path to Program Files.
sint64 rva;
void tool_lookup(....) {
....
printf("%08I64x %s + %x [%s:%d]\n",
addr, sym->name, addr-sym->rva, fn, line);
....
}PVS-Studio's diagnostic message: V576 Incorrect format. Consider checking the fourth actual 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, which means that it will write 8 bytes into the stack. The function printf() is a variadic function. The data type it must process is specified by the format string. In our case, the 'rva' variable will be processed as a 32-bit variable ("%x").
Whether or not this error will cause any faults depends on how in particular the compiler will be passing the arguments and on the platform bitness. For example, all the integer types in Win64 are first cast to a 64-bit type and only then are written into the stack, so there'll be no trouble with a variable occupying more stack memory than necessary.
However, if the variable 'rva' stores values bigger than INT_MAX, its value will be incorrectly printed anyway.
The same warning is generated for the following fragments:
void VDVideoCompressorVCM::GetState(vdfastvector<uint8>& data) {
DWORD res;
....
res = ICGetState(hic, data.data(), size);
....
if (res < 0)
throw MyICError("Video compression", res);
}PVS-Studio's diagnostic message: V547 Expression 'res < 0' is always false. Unsigned type value is never < 0. Riza w32videocodecpack.cpp 828
The variable 'res' is unsigned DWORD. It means that the "res < 0" expression will always give 'false'.
A similar check can be found in w32videocodec.cpp 284.
Here's one more bug of that kind.
#define ICERR_CUSTOM -400L
static const char *GetVCMErrorString(uint32 icErr) {
....
if (icErr <= ICERR_CUSTOM) err = "A codec-specific error occurred.";
....
}PVS-Studio's diagnostic message: 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 'unsigned', therefore the number '-400' will be implicitly cast to 'unsigned' before executing the comparison. As a result, the number '-400' will turn into 4294966896. Thus, the comparison (icErr <= -400) is equivalent to (icErr <= 4294966896). I guess this is far not what the programmer intended.
void AVIOutputFile::finalize() {
....
if (stream.mChunkCount && hdr.dwScale && stream.mChunkCount)
....
}PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'stream.mChunkCount' to the left and to the right of the '&&' operator. VirtualDub avioutputfile.cpp 761
The variable 'stream.mChunkCount' is checked twice. Either one of the checks is not necessary or something else should have been checked.
void VDVideoCompressorVCM::Start(const void *inputFormat,
uint32 inputFormatSize,
const void *outputFormat,
uint32 outputFormatSize,
const VDFraction& frameRate,
VDPosition frameCount)
{
this->hic = hic;
....
}PVS-Studio's diagnostic message: 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;
....
}PVS-Studio's diagnostic message: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. VirtualDub optdlg.cpp 120
Seems like incorrect code formatting. Or perhaps the keyword 'else' is missing.
bool VDCaptureDriverScreen::Init(VDGUIHandle hParent) {
....
mbAudioHardwarePresent = false;
mbAudioHardwarePresent = true;
....
}PVS-Studio's diagnostic message: V519 The 'mbAudioHardwarePresent' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 274, 275. VDCapture cap_screen.cpp 275
As you can see, even running static analysis for once may be very useful. But it is much more efficient to run it regularly. Programmers keep compiler warnings turned on all the time, not only once before the release, don't they? It's the same with static analysis tools. Using them regularly allows you to eliminate any bugs as soon as they occur. Think of PVS-Studio as kind of an additional story over the compiler which generates some more worthy warnings. It's best of all to use the incremental analysis: it allows you to detect bugs in freshly modified files right after compilation.