Intel IPP Samples for Windows - error correction

This is one of my posts on how PVS-Studio makes programs safer. That is where and what types of errors it detects. This time it is samples demonstrating handling of the IPP 7.0 library (Intel Performance Primitives Library) we are going to examine.

Intel Parallel Studio 2011 includes the Performance Primitives Library. This library in its turn includes a lot of primitives which allow you to create efficient video and audio codecs, signal processing software, image rendering mechanisms, archivers and so on. Sure, it is rather difficult to handle such a library. That is why Intel created a lot of demonstration programs based on it. You may see descriptions of samples and download them here:

All the samples are arranged into four groups:

Each set contains many projects, so, for a start, I took only the first set IPP Samples for Windows for the check. I used PVS-Studio 4.10 to perform the analysis.

I want to show you in this post that static analysis is useful regardless of programmers' skill and level of a solution being developed. The idea "you must employ experts and write code without errors right away" does not work. Even highly skilled developers cannot be secure from all the errors and misprints while writing code. Errors in samples for IPP show this very well.

I want you to note that IPP Samples for Windows is a high-quality project. But due to its size, 1.6 millions of code lines, it cannot but contain various errors. Let's examine some of them.

Bad replacement of array's indexes

I could well include this sample into my previous article "Consequences of using the Copy-Paste method in C++ programming and how to deal with it":

struct AVS_MB_INFO
{
  ...
  Ipp8u refIdx[AVS_DIRECTIONS][4];
  ...
};

void AVSCompressor::GetRefIndiciesBSlice(void){
  ...
  if (m_pMbInfo->predType[0] & predType)
  {
    m_refIdx[iRefNum] = m_pMbInfo->refIdx[dir][0];
    iRefNum += 1;
  }
  if (m_pMbInfo->predType[1] & predType)
  {
    m_refIdx[iRefNum] = m_pMbInfo->refIdx[dir][1];
    iRefNum += 1;
  }
  if (m_pMbInfo->predType[2] & predType)
  {
    m_refIdx[iRefNum] = m_pMbInfo->refIdx[dir][2];
    iRefNum += 1;
  }
  if (m_pMbInfo->predType[3] & predType)
  {
    m_refIdx[iRefNum] = m_pMbInfo->refIdx[dir][30];
    iRefNum += 1;
  }
  ...
}

The PVS-Studio's diagnostic message: V557 Array overrun is possible. The '30' index is pointing beyond array bound. avs_enc umc_avs_enc_compressor_enc_b.cpp 495

The programmer copied the code fragment several times and changed the arrays' indexes. But in the end his hand shook and he typed number 3 but forgot to delete 0. As a result, we have got index 30 and there is an overrun far outside the array's boundaries.

Identical code branches

Since we started with code copying, let's examine one more example related to it:

AACStatus aacencGetFrame(...)
{
  ...
  if (maxEn[0] > maxEn[1]) {
    ics[1].num_window_groups = ics[0].num_window_groups;
    for (g = 0; g < ics[0].num_window_groups; g++) {
      ics[1].len_window_group[g] = ics[0].len_window_group[g];
    }
  } else {
    ics[1].num_window_groups = ics[0].num_window_groups;
    for (g = 0; g < ics[0].num_window_groups; g++) {
      ics[1].len_window_group[g] = ics[0].len_window_group[g];
    }
  }
  ...
}  

The PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. aac_enc aac_enc_api_fp.c 1379

But this time it is just on the contrary - the programmer forgot to edit the copied code. The both branches of the conditional operator "if" perform the same actions.

Confusion with priority of "--" decrement operation and "*" pointer's dereferencing

static void
sbrencConflictResolution (..., Ipp32s *nLeftBord)
{
  ...
  *nLeftBord = nBordNext - 1;
  ...
  if (*lenBordNext > 1) {
    ...
    *nLeftBord--;
  }
  ...
}

The PVS-Studio's diagnostic message: V532 Consider inspecting the statement of '*pointer--' pattern. Probably meant: '(*pointer)--'. aac_enc sbr_enc_frame_gen.c 428

The "nLeftBord" pointer returns values from the "sbrencConflictResolution" function. At first, it is the value "nBordNext - 1" which is written by the specified address. On certain conditions, this value must be decremented by one. To decrement the value, the programmer used this code:

*nLeftBord--;

The error is that it is the pointer itself which is decremented instead of the value. The correct code looks this way:

(*nLeftBord)--;

More confusion with "++" increment operation and "*" pointer's dereferencing

I cannot understand the following code at all. I do not know how to fix it to make it meaningful. Perhaps something is missing here.

static IppStatus mp2_HuffmanTableInitAlloc(Ipp32s *tbl, ...)
{
  ...
  for (i = 0; i < num_tbl; i++) {
    *tbl++;
  }
  ...
}

The PVS-Studio's diagnostic message: V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. mpeg2_dec umc_mpeg2_dec.cpp 59

Here, the loop from the sample above is equivalent to the following code:

tbl += num_tbl;

The PVS-Studio analyzer supposed that brackets might be missing here and there must be this code: "(*tbl)++;". But this variant is meaningless too. In this case, the loop is equivalent to this code:

*tbl += num_tbl;

So, this loop is a rather strange one. The error does exist but only the code's author seems to know how to fix it.

Loss of error flag

The code has the function "GetTrackByPidOrCreateNew" that returns "-1" if an error occurs.

typedef signed int     Ipp32s;
typedef unsigned int   Ipp32u;

Ipp32s StreamParser::GetTrackByPidOrCreateNew(
  Ipp32s iPid, bool *pIsNew)
{
  ...
  else if (!pIsNew || m_uiTracks >= MAX_TRACK)
    return -1;
  ...
}

The "GetTrackByPidOrCreateNew" function itself is absolutely correct. But an error occurs while using it:

Status StreamParser::GetNextData(MediaData *pData, Ipp32u *pTrack)
{
  ...
  *pTrack = GetTrackByPidOrCreateNew(m_pPacket->iPid, NULL);

  if (*pTrack >= 0 && TRACK_LPCM == m_pInfo[*pTrack]->m_Type)
    ippsSwapBytes_16u_I((Ipp16u *)pData->GetDataPointer(),
                        m_pPacket->uiSize / 2);
  ...
}

The PVS-Studio's diagnostic message: V547 Expression '* pTrack >= 0' is always true. Unsigned type value is always >= 0. demuxer umc_stream_parser.cpp 179

The value returned by the "GetTrackByPidOrCreateNew" function is saved as the unsigned int type. It means that "-1" turns into "4294967295". The "*pTrack >= 0" condition is always true.

As a result, if the "GetTrackByPidOrCreateNew" function returns "-1", an Access Violation will occur while executing "m_pInfo[*pTrack]->m_Type".

Copy-Paste and missing +1

void H264SegmentDecoder::ResetDeblockingVariablesMBAFF()
{
  ...
  if (GetMBFieldDecodingFlag(m_gmbinfo->mbs[m_CurMBAddr 
                                            - mb_width * 2]))
    m_deblockingParams.nNeighbour[HORIZONTAL_DEBLOCKING] =
      m_CurMBAddr - mb_width * 2;
  else
    m_deblockingParams.nNeighbour[HORIZONTAL_DEBLOCKING] =
      m_CurMBAddr - mb_width * 2;
  ...
}

The PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. h264_dec umc_h264_segment_decoder_deblocking_mbaff.cpp 340

If you look at the nearby code, you will understand that the programmer forgot to add 1 in the copied line. This is the correct code:

if (GetMBFieldDecodingFlag(m_gmbinfo->mbs[m_CurMBAddr 
                                          - mb_width * 2]))
  m_deblockingParams.nNeighbour[HORIZONTAL_DEBLOCKING] =
    m_CurMBAddr - mb_width * 2;
else
  m_deblockingParams.nNeighbour[HORIZONTAL_DEBLOCKING] =
    m_CurMBAddr - mb_width * 2 + 1;

Not far from this place, there is the same error with missing "+ 1" in the function "H264CoreEncoder_ResetDeblockingVariablesMBAFF".

The PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. h264_enc umc_h264_deblocking_mbaff_tmpl.cpp.h 366

Remove that does not remove anything

void H264ThreadGroup::RemoveThread(H264Thread * thread)
{
    AutomaticUMCMutex guard(m_mGuard);
    std::remove(m_threads.begin(), m_threads.end(), thread);
}

The PVS-Studio's diagnostic message: V530 The return value of function 'remove' is required to be utilized. h264_dec umc_h264_thread.cpp 226

This is quite an interesting combination. On the one hand, everything is cool. We have mutex to correctly remove items in a multithreaded application. On the other hand, the developers simply forgot that the std::remove function does not remove items from the array but only rearranges them. Actually, this code must look this way:

m_threads .erase(
  std::remove(m_threads.begin(), m_threads.end(), thread),
  m_threads.end());

Comparing structures' fields to themselves

I was looking through the errors and noticed that implementation of the H264 video compression standard is somewhat defective. A lot of errors we have found relate to this very project. For instance, the programmer was in a hurry and used two wrong variable names at once.

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 PVS-Studio's diagnostic messages:

V501 There are identical sub-expressions 'p_newHeader.pic_parameter_set_id' to the left and to the right of the '!=' operator. h264_spl umc_h264_au_stream.cpp 478

V501 There are identical sub-expressions 'p_newHeader.field_pic_flag' to the left and to the right of the '!=' operator. h264_spl umc_h264_au_stream.cpp 479

The comparison function does not work because some members of the structure are compared to themselves. Here are the two corrected lines:

(p_newHeader.pic_parameter_set_id != m_lastSlice.pic_parameter_set_id)
(p_newHeader.field_pic_flag != m_lastSlice.field_pic_flag)

Incorrect data copying

Errors related to use of wrong objects occur not only in comparison operations but in operations of copying objects' states:

Ipp32s ippVideoEncoderMPEG4::Init(mp4_Param *par)
{
  ...
  VOL.sprite_width = par->sprite_width;
  VOL.sprite_height = par->sprite_height;
  VOL.sprite_left_coordinate = par->sprite_left_coordinate;
  VOL.sprite_top_coordinate = par->sprite_left_coordinate;
  ...
}

The PVS-Studio's diagnostic message: V537 Consider reviewing the correctness of 'sprite_left_coordinate' item's usage. mpeg4_enc mp4_enc_misc.cpp 387

A wrong value is saved into "VOL.sprite_top_coordinate". This is the correct assignment operation:

VOL.sprite_top_coordinate = par->sprite_top_coordinate;

Two loops for one variable

JERRCODE CJPEGDecoder::DecodeScanBaselineNI(void)
{
  ...
  for(c = 0; c < m_scan_ncomps; c++)
  {
    block = m_block_buffer + (DCTSIZE2*m_nblock*(j+(i*m_numxMCU)));

    // skip any relevant components
    for(c = 0; c < m_ccomp[m_curr_comp_no].m_comp_no; c++)
    {
      block += (DCTSIZE2*m_ccomp[c].m_nblocks);
    }
  ...
}

The PVS-Studio's diagnostic message: V535 The variable 'c' is being used for this loop and for the outer loop. jpegcodec jpegdec.cpp 4652

One variable 'c' is used for two loops nested in each other. A decoding function like this may cause strange and unpredicted results.

Double assignment for additional safety

H264EncoderFrameType*
H264ENC_MAKE_NAME(H264EncoderFrameList_findOldestToEncode)(...)
{
  ...
  MaxBrefPOC = 
    H264ENC_MAKE_NAME(H264EncoderFrame_PicOrderCnt)(pCurr, 0, 3);
  MaxBrefPOC = 
    H264ENC_MAKE_NAME(H264EncoderFrame_PicOrderCnt)(pCurr, 0, 3);
  ...
}

The PVS-Studio's diagnostic message: V519 The 'MaxBrefPOC' object is assigned values twice successively. Perhaps this is a mistake. h264_enc umc_h264_enc_cpb_tmpl.cpp.h 784

When I saw this code, I recalled an old programmers' joke:

- Why do you have two identical GOTO one right after the other in your code?

- What if the first one doesn't work!

Well, this error is not crucial yet it is an error.

Code making you alert

AACStatus sbrencResampler_v2_32f(Ipp32f* pSrc, Ipp32f* pDst)
{
  ...
  k = nCoef-1;
  k = nCoef;
  ...
}

The PVS-Studio's diagnostic message: V519 The 'k' object is assigned values twice successively. Perhaps this is a mistake. aac_enc sbr_enc_resampler_fp.c 90

This double assignment alerts me much more than in the previous sample. It seems as if the programmer was not confident. Or as if he decided to try "nCoef-1" first and then "nCoef". It is also called "programming through experiment method". Anyway, it is that very case when you should stop for a while and think it over on encountering such a fragment.

Minimum value which is not quite minimum

void MeBase::MakeVlcTableDecision()
{
  ...
  Ipp32s BestMV= IPP_MIN(IPP_MIN(m_cur.MvRate[0],m_cur.MvRate[1]),
                         IPP_MIN(m_cur.MvRate[2],m_cur.MvRate[3]));
  Ipp32s BestAC= IPP_MIN(IPP_MIN(m_cur.AcRate[0],m_cur.AcRate[1]),
                         IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[2]));
  ...
}

The PVS-Studio's diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '<' operator: (m_cur.AcRate [2]) < (m_cur.AcRate [2]) me umc_me.cpp 898

Here is another misprint in the array's index. The last index must be 3, not 2. This is the correct code:

Ipp32s BestAC= IPP_MIN(IPP_MIN(m_cur.AcRate[0],m_cur.AcRate[1]),
                       IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[3]));

What is unpleasant about such errors is that the code "almost works". The error occurs only if the minimum item is stored in "m_cur.AcRate[3]". Such errors like to hide during testing and show up on users' computers at user input data.

Maximum value which is not quite maximum

There are problems with maximum values too:

Ipp32s ippVideoEncoderMPEG4::Init(mp4_Param *par)
{
  ...
  i = IPP_MAX(mBVOPsearchHorBack, mBVOPsearchHorBack);
  ...
}

The PVS-Studio's diagnostic message: V501 There are identical sub-expressions '(mBVOPsearchHorBack)' to the left and to the right of the '>' operator. mpeg4_enc mp4_enc_misc.cpp 547

The mBVOPsearchHorBack variable is used twice. Actually, the programmer intended to use mBVOPsearchHorBack and mBVOPsearchVerBack:

i = IPP_MAX(mBVOPsearchHorBack, mBVOPsearchVerBack);

A bad shot

typedef struct
{
  ...
  VM_ALIGN16_DECL(Ipp32f)      nb_short[2][3][__ALIGNED(MAX_PPT_SHORT)];
  ...
} mpaPsychoacousticBlock;

static void mp3encPsy_short_window(...)
{
  ...
  if (win_counter == 0) {
    nb_s = pBlock->nb_short[0][3];
  }
  ...
}

The PVS-Studio's diagnostic message: V557 Array overrun is possible. The '3' index is pointing beyond array bound. mp3_enc mp3enc_psychoacoustic_fp.c 726

There must be a simple misprint here. It is index '3' used accidentally instead of '2'. I think you understand the consequences.

Error causing a slowdown

void lNormalizeVector_32f_P3IM(Ipp32f *vec[3], Ipp32s* mask, 
                               Ipp32s len) {
  Ipp32s  i;
  Ipp32f  norm;

  for(i=0; i

The PVS-Studio's diagnostic message: V503 This is a nonsensical comparison: pointer < 0. ipprsample ippr_sample.cpp 501

This is a nice example of code that works slower than it could due to an error. The algorithm must normalize only those items which are specified in the mask array. But this code normalizes all the items. The error is located in the "if(mask<0)" condition. The programmer forgot to use the "i" index. The "mask" pointer will be almost all the time above or equal to zero and therefore we will process all the items.

This is the correct code:

if(mask[i]<0) continue;

Subtraction result always amounts to 0

int ec_fb_GetSubbandNum(void *stat)
{
    _fbECState *state=(_fbECState *)stat;
    return (state->freq-state->freq);
}

The PVS-Studio's diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '-' operator: state->freq - state->freq speech ec_fb.c 250

A misprint here causes the function to return 0 all the time. We are subtracting something wrong here. I do not know what it actually must be.

Incorrect processing of buffer overflow

typedef unsigned int    Ipp32u;

UMC::Status Init(..., Ipp32u memSize, ...)
{
  ...
  memSize -= UMC::align_value(m_nFrames*sizeof(Frame));
  if(memSize < 0)
      return UMC::UMC_ERR_NOT_ENOUGH_BUFFER;
  ...
}

The PVS-Studio's diagnostic message: V547 Expression 'memSize < 0' is always false. Unsigned type value is never < 0. vc1_enc umc_vc1_enc_planes.h 200

Processing of situation when the buffer's size is not sufficient is implemented incorrectly. The program will continue working instead of returning the error's code and most likely will crash. The point is that the "memSize" variable has the "unsigned int" type. So, the "memSize < 0" condition is always false and we go on working with a buffer overflow.

I think it is a good example of software attack vulnerability. You may cause a buffer overflow by feeding incorrect data into the program and use it for your own purposes. By the way, we found about 10 such vulnerabilities in the code. I will not describe them here not to overload the text.

Overrun in the wake of incorrect check

Ipp32u m_iCurrMBIndex;
VC1EncoderMBInfo* VC1EncoderMBs::GetPevMBInfo(Ipp32s x, Ipp32s y)
{
  Ipp32s row = (y>0)? m_iPrevRowIndex:m_iCurrRowIndex;
  return ((m_iCurrMBIndex - x <0 || row <0)? 0 :
    &m_MBInfo[row][m_iCurrMBIndex - x]);
}

The PVS-Studio's diagnostic message: V547 Expression 'm_iCurrMBIndex - x < 0' is always false. Unsigned type value is never < 0. vc1_enc umc_vc1_enc_mb.cpp 188

The "m_iCurrMBIndex" variable has the "unsigned" type. Because of it, the "m_iCurrMBIndex - x" expression also has the "unsigned" type. Therefore, the "m_iCurrMBIndex - x < 0" condition is always false. Let's see what consequences it has.

Let the "m_iCurrMBIndex" variable amount to 5 and "x" variable amount to 10.

The "m_iCurrMBIndex - x" expression equals 5u - 10i = 0xFFFFFFFBu.

The "m_iCurrMBIndex - x < 0" condition is false.

The "m_MBInfo[row][0xFFFFFFFBu]" expression is executed and an overrun occurs.

Error of using '?:' ternary operator

The ternary operator is rather dangerous because you may easily make an error using it. Nevertheless, programmers like to write code as short as possible and use the interesting language construct. The C++ language punishes them for this.

vm_file* vm_file_fopen(...)
{
  ...
  mds[3] = FILE_ATTRIBUTE_NORMAL |
           (islog == 0) ? 0 : FILE_FLAG_NO_BUFFERING;
  ...
}

The PVS-Studio's diagnostic message: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. vm vm_file_win.c 393

There must be a combination of flags FILE_ATTRIBUTE_NORMAL and FILE_FLAG_NO_BUFFERING. But actually, the "mds[3]" item is always assigned 0.

The programmer forgot that the priority of "|" operator is higher than that of "?:" operator. So it turns out that we have the following expression in the code (note the brackets):

(FILE_ATTRIBUTE_NORMAL | (islog == 0)) ?

0 : FILE_FLAG_NO_BUFFERING;

The "FILE_ATTRIBUTE_NORMAL | (islog == 0)" condition is always true and we assign 0 to "mds[3]" item.

This is the correct expression (note the brackets once again):

FILE_ATTRIBUTE_NORMAL |
  ((islog == 0) ? 0 : FILE_FLAG_NO_BUFFERING);

Strange handling of array

AACStatus alsdecGetFrame(...)
{
  ...
  for (i = 0; i < num; i++) {
    ...
    *tmpPtr = (Ipp32s)((tmp << 24) + ((tmp & 0xff00) << 8) +
                      ((tmp >> 8) & 0xff00) + (tmp >> 24));
    *tmpPtr = *srcPrt;
    ...
  }
  ...
}

The PVS-Studio's diagnostic message: V519 The '* tmpPtr' object is assigned values twice successively. Perhaps this is a mistake. aac_dec als_dec_api.c 928

I suggest that the readers examine the code themselves and draw conclusions. I would just call this code "peculiar".

Paranormal assignments

static
IPLStatus ownRemap8u_Pixel(...) {
  ...
  saveXMask    = xMap->maskROI;
  saveXMask    = NULL;
  saveYMask    = yMap->maskROI;
  saveYMask    = NULL;  
  ...
}

The PVS-Studio's diagnostic messages:

V519 The 'saveXMask' object is assigned values twice successively. Perhaps this is a mistake. ipl iplremap.c 36

V519 The 'saveYMask' object is assigned values twice successively. Perhaps this is a mistake. ipl iplremap.c 38

I cannot see the reason for such strange code. Note that this block is repeated 8 times in different functions!

There are also other strange assignments of one variable:

Ipp32s ippVideoEncoderMPEG4::Init(mp4_Param *par)
{
  ...
  mNumOfFrames = par->NumOfFrames;
  mNumOfFrames = -1;
  ...
}

The PVS-Studio's diagnostic message: V519 The 'mNumOfFrames' object is assigned values twice successively. Perhaps this is a mistake. mpeg4_enc mp4_enc_misc.cpp 276

Summary

I described only some of the errors detected in IPP Samples for Windows in this article. I have not listed some errors because they are twins with those I have discussed in the article, so it would not be interesting to read about them. I also have not given inessential errors here. For instance, take assert() which always has a true condition because of a misprint. I skipped many code fragments because I simply did not know if there were errors or just poor code. But I think I have described enough defects to show you how difficult it is to write large projects even for skilled developers.

Let me once again formulate the idea I have mentioned in the beginning of the article. Even a good programmer is not secure from misprints, absent-mindedness, urge to use Copy-Paste and logical errors. I think this article will be a good answer for those people who believe that the phrase "you must write correct code" will protect them against any errors.

I wish you luck in all your C/C++/C++0x projects. May you find as many errors as possible using the static analysis methodology I love so much!