• bearophile (161/162) Feb 07 2011 This article shows some common bugs found by a static analysis tool in a...

bearophile <bearophileHUGS lycos.com> writes:

This article shows some common bugs found by a static analysis tool in about
1.6 millions of code lines of the Intel IPP Samples for Windows, a high-quality
C++ project.

"Intel IPP Samples for Windows - error correction", by Andrey Karpov, January
2011:
http://software.intel.com/en-us/articles/intel-ipp-samples-for-windows-error-correction/

Here I show a reduced version of some of the bugs discussed.

-------------------

"Identical code branches" and "Copy-Paste and missing +1", caused by copy &
paste with missed modify of one of the two branches:

void main(string[] args) {
    int x;
    if (args.length > 1) {
        x++;
    } else {
        x++;
    }
}

-------------------

"Loss of error flag", "Incorrect processing of buffer overflow", "Overrun in
the wake of incorrect check":

import core.stdc.stdio: printf;
int foo(bool cant_be_true) {
    int error = cant_be_true ? -1 : 1;
    return error;
}
void main() {
    uint x = foo(true);
    if (x < 0)
        printf("error!");
}


This is a serious, common and easy to catch bug, so I have added this:
http://d.puremagic.com/issues/show_bug.cgi?id=5539

-------------------

"Remove that does not remove anything":

Time ago I have added an enhancement request about that:
http://d.puremagic.com/issues/show_bug.cgi?id=5464

-------------------

"Comparing structures' fields to themselves":

bool H264_AU_Stream::IsPictureSame(H264SliceHeaderParse & p_newHeader)
{
  if ((p_newHeader.frame_num != m_lastSlice.frame_num) ||
      (p_newHeader.pic_parameter_set_id !=
       p_newHeader.pic_parameter_set_id) ||
      (p_newHeader.field_pic_flag != p_newHeader.field_pic_flag) ||
      (p_newHeader.bottom_field_flag != m_lastSlice.bottom_field_flag)
      ){
      return false;
  }
  ...
}


The bug is:

if (x != x) {...

Generally similar redundancies in the code are a sign of a probable bug, so I
have added this:
http://d.puremagic.com/issues/show_bug.cgi?id=5540

-------------------

"Two loops for one variable":

void main() {
    int c;
    for (c = 0; c < 10; c++) {
        for (c = 0; c < 20; c++) {
            // ...
        }
    }
}

-------------------

"Double assignment for additional safety", "Code making you alert", "Strange
handling of array" and "Paranormal assignments":

void main() {
    int x;
    x = 10;
    x = 20;
}

-------------------

"Minimum value which is not quite minimum", "Maximum value which is not quite
maximum":

import std.algorithm: min, max;
void main() {
    int x = 10;
    int y = 20;
    int z = min(x, x);
    int w = max(y, y);    
}

-------------------

"Subtraction result always amounts to 0":

void main() {
    int x = 10;
    return x - x;
}

Another example of redundancy that often hides a bug. Covered by 5540.

-------------------

"Error of using '?:' ternary operator", it's a problem caused by ternary
operator precedence

enum { F0, F1, F2 }
void main() {
    int x = 10;
    int y = F1 | (x == 0) ? 0 : F2;
    assert(y == 0);
}

-------------------

From the Summary:

void main() {
    int x = 10;
    assert(x == x);
}


Covered by 5540.

===========================

The following cases are from this related article:
http://www.viva64.com/en/a/0068/

-------------------

inline_ bool Contains(const LSS& lss)
{
  // We check the LSS contains the two 
  // spheres at the start and end of the sweep
  return
    Contains(Sphere(lss.mP0, lss.mRadius)) && 
    Contains(Sphere(lss.mP0, lss.mRadius));
}


void COX3DTabViewContainer::OnNcPaint() 
{
  ...
  if(rectClient.top<rectClient.bottom &&
     rectClient.top<rectClient.bottom)
  {
    dc.ExcludeClipRect(rectClient);
  }
  ...
}


That are of this kind:
x && x

-------------------

The last case can't be reached:

string TimePeriod::toString() const
{
  ...
  if (_relativeTime <= 143)
    os << ((int)_relativeTime + 1) * 5 << _(" minutes");
  else if (_relativeTime <= 167)
    os << 12 * 60 + ((int)_relativeTime - 143) * 30 << _(" minutes");
  else if (_relativeTime <= 196)
    os << (int)_relativeTime - 166 << _(" days");
  else if (_relativeTime <= 143)
    os << (int)_relativeTime - 192 << _(" weeks");
  ...
}



While here one case is duplicated:

void DXUTUpdateD3D10DeviceStats(...)
{
  ...
  else if( DeviceType == D3D10_DRIVER_TYPE_SOFTWARE )
    wcscpy_s( pstrDeviceStats, 256, L"WARP" );
  else if( DeviceType == D3D10_DRIVER_TYPE_HARDWARE )
    wcscpy_s( pstrDeviceStats, 256, L"HARDWARE" );
  else if( DeviceType == D3D10_DRIVER_TYPE_SOFTWARE )
    wcscpy_s( pstrDeviceStats, 256, L"SOFTWARE" );
  ...
}

-------------------

One nice advice to avoid some bugs caused by code copy & paste:

The second method to make errors fewer in some cases is to discipline oneself
and edit the code being copied in a special way. [...] You should edit the code
so that fragments which must differ from each other are visually arranged in a
column. It is much more difficult to make an error if you use this method.<

int ztrend = sgn(
  buffer[samplesleft - WindowSizeInt-2]-buffer[samplesleft 
- WindowSizeInt-2]);

==>

int ztrend = sgn(
  buffer[samplesleft - WindowSizeInt-2] -
  buffer[samplesleft - WindowSizeInt-2]);
  
Bye,
bearophile