Check of CommonLib using PVS-Studio

# Check of CommonLib using PVS-Studio

18:26
Mon
13
Nov 2017

Courtesy developers of PVS-Studio, I could use this static code analysis tool to check my home projects. I blogged about the tool recently. It's powerful and especially good at finding issues with 32/64-bit portability. Now I analyzed CommonLib - a library I develop and use for many years, which contains various facilities for C++ programming. That allowed me to find and fix many bugs. Here are the most interesting ones I've met:

(Disclaimer: This code is very old. Some parts of it can have as much as 10 or more years. Thus it's not representative to my current coding skills or style.)

V512 The format string in the 'swprintf_s' function is longer than the 'sz' buffer, so it will be truncated. Probably it is a mistake.

The tool was right. Following function converts pointer to string. It assumed pointer representation is 8 chars long, while in 64-bit applications it obviously has 16 chars.

void PtrToStr(tstring *Out, const void* p)
{
-    tchar sz[9];
+    tchar sz[17];
#ifdef _UNICODE
    _stprintf_s(sz, _countof(sz), L"%p", p); // <=
#else
    #ifdef _MSC_VER
        sprintf_s(sz, _countof(sz), "%p", p);
    #else
        sprintf(sz, "%p", p);
    #endif
#endif
    *Out = sz;
}
V1001 The 'out' variable is assigned but is not used until the end of the function.

That's the most horrible bug I found. Instead of assigning output parameter pointed by out, I was moving the pointer itself. This function didn't work at all. Apparently I never used or tested it.

template <typename T>
inline void EvalQuadraticBezierCurve(T *out, const T &a, const T &b, const T &c, float t)
{
    float t_inv = 1.f - t;
-    out = t_inv*t_inv*a + 2.f*t*t_inv*b + t*t*c; // <=
+    *out = t_inv*t_inv*a + 2.f*t*t_inv*b + t*t*c;
}
V690 The 'GameTime' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class.

GameTime is a structure that wraps int64_t number, fetched from QueryPerformanceCounter. It is intended to be used like built-in POD types, so just passing and copying it by value is OK. Custom copy constructor, destructor or assignment operator is not needed. So the tool was right, but the solution was to remove unnecessary assignment operator instead of adding copy constructor.

-    GameTime & operator = (GameTime v) { m_PerfCount = v.m_PerfCount; return *this; }
V595 The 'beforeThis' pointer was utilized before it was verified against nullptr.

Node class represents a node of a tree structure. This method clearly assumes that beforeThis parameter cannot be null, which is checked in first assert, so the pointed condition is unnecessary.

void Node::LinkChildBefore( Node *beforeThis, Node *addThis )
{
    assert(beforeThis && beforeThis->m_Parent == this);
    assert(addThis && addThis->m_Parent == NULL && addThis->m_PrevSibling == NULL && addThis->m_NextSibling == NULL);
    
    Node *afterThis = beforeThis->m_PrevSibling; // <=
    
    addThis->m_Parent = this;
    addThis->m_PrevSibling = afterThis;
    addThis->m_NextSibling = beforeThis;
    
-    if (beforeThis) // <=
        beforeThis->m_PrevSibling = addThis;
    if (afterThis)
        afterThis->m_NextSibling = addThis;
    else
        m_FirstChild = addThis;
}
V522 There might be dereferencing of a potential null pointer 'Ptr'.

FreeList<T> class contains some C++ and pointer trickery because it's a custom allocator based on fixed memory pool and list of free items. PrvTryNew tries to allocate (find a free place) for new item and returns pointer to it. If no free cells are available, it returns null, so following function would call constructor of type T on null pointer in such case.

-    T * TryNew() { T *Ptr = PrvTryNew(); return new (Ptr) T; }
+    T * TryNew() { T *Ptr = PrvTryNew(); return Ptr ? new (Ptr) T : nullptr; }
V820 The 'sr' variable is not used after copying. Copying can be replaced with move/swap for optimization.

It was a good suggestion for optimization. I know this function could be written in much better way, but for now I just followed this message and replaced usage of first string (sr) with conversion of Red component straight to Out.

void ColorToStr(tstring *Out, const COLORF &Color, char Format)
{
    if (Format == 'f')
    {
        tstring sr, sg, sb;
        FloatToStr(&sr, Color.R);
        FloatToStr(&sg, Color.G);
        FloatToStr(&sb, Color.B);
        *Out = sr; *Out += _T(','); *Out += sg; *Out += _T(','); *Out += sb; // <=
    }
-->
void ColorToStr(tstring *Out, const COLORF &Color, char Format)
{
    if (Format == 'f')
    {
        tstring sg, sb;
        FloatToStr(Out, Color.R);
        FloatToStr(&sg, Color.G);
        FloatToStr(&sb, Color.B);
        *Out += _T(','); *Out += sg; *Out += _T(','); *Out += sb;
    }
V818 It is more efficient to use an initialization list 'm_strName(Name)' rather than an assignment operator.

It is a good idea to use initialization list instead of normal assignments in a constructor, especially if the type is std::wstring, like in my case:

ProfilerItem::ProfilerItem(const tstring &Name) // <=
{
    m_Time = GameTime::ZERO;
    m_Count = 0;
    m_strName = Name;
}
-->
ProfilerItem::ProfilerItem(const tstring &Name) :
    m_Time(GameTime::ZERO),
    m_Count(0),
    m_strName(Name)
{
}
V816 It is more efficient to catch exception by reference rather than by value.

An optimization suggestion for my testign code. PVS-Studio is absolutely right. The class I use for error handling contains a vector of strings, so it's definitely better not to copy it.

    try
    {
        Test();
    }
-    catch (Error Err) // <=
+    catch (const Error& Err)
    {
        tcout << _T("!!!!!!!!!!! EXCEPTION !!!!!!!!!!!") << endl;
        tstring s;
        Err.GetMessage_(&s);
        WriteLine(s);
    }
V105 Second operand of '?:' operation: implicit type conversion to memsize type.

A nasty bug related to 64-bit compatibility. This is a function for case-insensitive search of a substring. It returns maximum value if not found. Originally this value was UINT32_MAX, but after I made the library portable to 64 bits and this function returns size_t now, it definitely should be SIZE_MAX.

size_t StrStrI(const tstring &Str, const tstring &SubStr, size_t Count)
{
    const tchar *StrSz    = Str.c_str();
    const tchar *SubStrSz = SubStr.c_str();
    const tchar *R = StrStrI(StrSz, SubStrSz, Count);
-    return (R == NULL) ? MAXUINT32 : (R - StrSz); // <=
+    return (R == NULL) ? SIZE_MAX : (R - StrSz);
}
V104 Implicit conversion of 'i' to memsize type in an arithmetic expression: i < KeywordCount

The tools found many bugs like this one. I tried to change all the sizes and counts from uint32_t to size_t when porting to 64 bits and I definitely did it everywhere on library interface, but apparently I omitted some places in the internal implementation. Here variable i is compared with KeywordCount of type size_t and used to index an array, so it definitely should have type size_t.

void Tokenizer::RegisterKeywords(const tchar **Keywords, size_t KeywordCount)
{
-    for (uint i = 0; i < KeywordCount; i++) // <=
+    for (size_t i = 0; i < KeywordCount; i++)
        RegisterKeyword(i, Keywords[i]);
}

There are also many messages repoted by PVS-Studio that I deliberately decided to ignore. Although they may indicate a bug in certain cases, I'm pretty sure they are not applicable to my specific case.

V547 Expression 'size > 0xffffffffffffffffui64' is always false.

size variable is always 64-bit. I then use it to set capacity of a vector, which takes size_t. On 64-bit platforms these types are equivalent, but on 32-bit ones, size_t is 32-bit, so before casting to smaller type I wanted to check if the size doesn't exceed 4 GB. So the reported message is true for 64-bit configuration, but still makes sense on 32-bit configuration.

void LoadUnicodeFromStream(SeekableStream *Src, wstring *Out, unsigned Encoding, FILE_ENCODING *OutEncoding)
{
    ERR_TRY;

    // Wczytaj cały plik do pamięci
    VectorStream VS;
    {
        uint64 size = Src->GetSize();
        if(size > SIZE_MAX) // <=
            throw Error(_T("Stream too long."), __TFILE__, __LINE__);
        VS.SetCapacity((size_t)size);
        CopyToEnd(&VS, Src);
        VS.Rewind();
    }
V730 Not all members of a class are initialized inside the constructor. Consider inspecting: x, y, z.

This is generally a good advice... unless I write a structure that should behave like basic POD types (including uninitialized default value) and I want it to be as efficient as possible - like a geometric vector.

struct VEC3
{
    float x, y, z;
    VEC3() { } // <=
V550 An odd precise comparison: x == v.x. It's probably better to use a comparison with defined precision: fabs(A - B) < Epsilon.

Again, this is generally a good advice... unless I write a structure that should behave like basic types (e.g. float, double), including comparisons.

bool operator == (const VEC3 &v) const { return x == v.x && y == v.y && z == v.z; } // <=
V779 Unreachable code detected. It is possible that an error is present.

This is a more complex one. Read function of Stream class tries to read data from a stream and returns number of bytes read. MustRead function uses the former, but if unable to read, it throws exception. PVS-Studio detected the place I marked as unreachable code because Read function unconditionally throws exception.

The problem is that Read is virtual and intended to be overriden by classes inherited from Stream, which provide valid reading logic and not necessarily throw an exception! I think PVS-Studio shouldn't report this error. If called function unconditionally throws exception, but it's virtual, it is not really an unconditional throw.

size_t Stream::Read(void *Data, size_t Size)
{
    throw Error(_T("Stream class doesn't support read."), __TFILE__, __LINE__);
}

void Stream::MustRead(void *Data, size_t Size)
{
    if (Size == 0) return;
    size_t i = Read(Data, Size);
    if (i != Size) // <=
        throw Error(Format(_T("Stream read error: #/# bytes read.")) % i % Size, __TFILE__, __LINE__);
}
V112 Dangerous magic number 32 used: c = ((c >> 32) + c) & 0x0...
V112 Dangerous magic number 0x00000000FFFFFFFFll used: ...32) + c) & 0x00000000FFFFFFFFll;.

I got a lot of this one. First I thought it's related to bit shift, but later I saw that the tool could report it for pretty much any literal number in the code. I just needed to ignore this message.

inline uint CountBitsSet(uint64 v)
{
    uint64 c = v - ((v >> 1) & 0x5555555555555555ll);
    c = ((c >>  2) & 0x3333333333333333ll) + (c & 0x3333333333333333ll);
    c = ((c >>  4) + c) & 0x0F0F0F0F0F0F0F0Fll;
    c = ((c >>  8) + c) & 0x00FF00FF00FF00FFll;
    c = ((c >> 16) + c) & 0x0000FFFF0000FFFFll;
    c = ((c >> 32) + c) & 0x00000000FFFFFFFFll; // <=
    return (uint)c;
}

Comments | #commonlib #c++ #pvs-studio Share

Comments

STAT NO AD
[Stat] [STAT NO AD] [Download] [Dropbox] [pub] [Mirror]
Copyright © 2004-2017