Intel IPP Samples for Windows - bug fixes

This is my next article about how PVS-Studio makes programs more reliable. That is where, and what errors it detects. This time under the hammer were examples that demonstrate the work with the library IPP 7.0 (Intel Performance Primitives Library). I wanted to post this post on Intel at first, but then I decided that it would be quite ...

Intel Parallel Studio 2011 includes the Performance Primitives Library. This library includes a large number of primitives that allow you to create effective video and audio codecs, signal processing programs, image rendering mechanisms, archivers, and much more. Naturally, it is not easy to work with such a library. Therefore, Intel has produced a large number of demonstration programs based on this library. You can get acquainted with the description of the examples and download them through the following links:

• Code Samples for Intel Integrated Performance Primitives (Intel IPP) Library
• Intel Integrated Performance Primitives Library 7.0 Samples for Parallel Studio 2011

All examples are divided into four groups:

• IPP Samples for Windows
• IPP UIC Demo for Windows
• IPP DMIP Samples for Windows
• IPP Cryptography Samples for Windows

Each set contains a large number of projects, so for starters, I took for verification only the first set of IPP Samples for Windows. I carried out the check using PVS-Studio version 4.10.

With this post I want to show that static analysis is useful regardless of the professionalism of the programmers and the level of the solution being developed. The idea of ​​“taking professionals and writing right away without mistakes” does not work. Even highly skilled developers are not immune from errors and typos in the coding process. The errors in the examples for IPP demonstrate this very well.

I emphasize that IPP Samples for Windows is a high-quality project. However, due to its size of 1.6 million lines of code, it inevitably contains a variety of errors. Consider some of them.
')

Unsuccessful replacement of array indices

This example could have been a great addition to my previous article " The consequences of using Copy-Paste technology when programming in C ++ 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;
   }
   ...
 } 

Diagnostic message of PVS-Studio: 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 value of the array indices. But at the very end, his hand shook. He entered the number 3, but did not delete the number 0. As a result, an index of 30 is obtained and, when the code is executed, it will be accessed far beyond the array boundaries.

Same code branches

Once we started by copying the code, here’s another example on this topic:

  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];
     }
   }
   ...
 } 

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

But this time, on the contrary, they forgot to edit the copied code. Both branches of the conditional “if” operator perform the same actions.

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

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

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

The “nLeftBord” pointer uses the “sbrencConflictResolution” function to return a value from the function. First, the value "nBordNext - 1" is written to the specified address. Under certain conditions, this value should be reduced by one. To decrease the value, the programmer used the following code:

  * nLeftBord--; 

The error is that instead of the value, the pointer itself is reduced. The correct code should look like this:

  (* nLeftBord) -; 

Even more confusing situation with the increment operation "++" and dereference of the pointer "*"

The following code is completely incomprehensible to me. I do not know how to fix it so that it acquires meaning. Perhaps something is missing here.

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

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

Now, the loop in the example is equivalent to the following code:

  tbl + = num_tbl; 

The PVS-Studio analyzer suggested that perhaps the parentheses were forgotten and you should write "(* tbl) ++;". But this code does not make sense. Then the cycle will be equivalent:

  * tbl + = num_tbl; 

In general, a very strange kind of cycle. There is an error, but only the author of the code can fix it.

Loss of symptom that an error has occurred

The code has a function "GetTrackByPidOrCreateNew", which returns the value "-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;
   ...
 } 

With the function "GetTrackByPidOrCreateNew" everything is fine. But there is an error when 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);
   ...
 } 

PVS-Studio 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 stored in unsigned int format. This means that "-1" will turn into "4294967295", and the condition "* pTrack> = 0" is always true.

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

Copy-Paste and Forgotten +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;
   ...
 } 

PVS-Studio 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 code side by side, it becomes clear that you forgot to add one in the copied line. The correct code should look like this:

  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 away in the function “H264CoreEncoder_ResetDeblockingVariablesMBAFF” there is exactly the same error with the forgotten “+ 1”.

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

Remove, which removes nothing

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

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

An interesting combination. On the one hand, everything is solid. Used by mutex to correctly remove an element in a multi-threaded application. On the other hand, it is banal to forget that the std :: remove function does not remove elements from an array, but only changes their order. In fact, it should be written like this:

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

Comparison of structure fields with yourself

I look at the errors and noticed that the implementation of the H264 video compression standard is kind of buggy. This project includes a fairly large number of errors found. For example, someone hurried while programming and used two incorrect 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;
   }
   ...
 } 

PVS-Studio diagnostic messages:

V501 There are identical sub-expressions "p_newHeader.pic_parameter_set_id" to the left! h264_spl umc_h264_au_stream.cpp 478

V501 There are identical sub-expressions "p_newHeader.field_pic_flag" h264_spl umc_h264_au_stream.cpp 479

The comparison function does not work, since 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

There are errors in using the wrong objects, not only when comparing, but also when copying object 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;
   ...
 } 

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

An invalid value is being placed in VOL.sprite_top_coordinate. The correct assignment should look like this:

  VOL.sprite_top_coordinate = par-> sprite_top_coordinate; 

Two cycles in 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);
     }
   ...
 } 

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

For two cycles nested into each other, one 'c' variable is used. The result of this decoding function may be very strange and unexpected.

Double assignment for greater reliability

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

PVS-Studio 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 remembered an old programmer joke:

- Why do you have two identical GOTOs in your code in a row?

- What if the first does not work!

This error, perhaps, is not critical, but still it is a mistake.

Code that alarming

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

PVS-Studio 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 is much more alarming than in the previous example. It seems that the programmer was not confident. Or I decided, at the beginning, to try "nCoef-1", and then "nCoef". This is also called "programming method of experiment." And in any case, this is exactly the place in the code, after seeing which one should linger and give oneself to reflections.

The minimum value that 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]));
   ...
 } 

PVS-Studio diagnostic message: V501 operator: (m_cur.AcRate [2]) <(m_cur.AcRate [2]) me umc_me.cpp 898

Again a typo in the array index. The last index should be 3, not 2. Correct version of the 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])); 

Such errors are unpleasant because the code “almost works”. An error will manifest itself only if the minimum element is stored in “m_cur.AcRate [3]”. Such errors like to manifest themselves not when testing, but at the user on their input data sets.

The maximum value, which is not quite the maximum

With maximum values ​​also does not always go well:

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

PVS-Studio diagnostic message: V501 There are identical sub-expressions (mBVOPsearchHorBack) 'the operator. mpeg4_enc mp4_enc_misc.cpp 547

The mBVOPsearchHorBack variable is used twice. In fact, it was planned to use mBVOPsearchHorBack and mBVOPsearchVerBack:

  i = IPP_MAX (mBVOPsearchHorBack, mBVOPsearchVerBack); 

We fall into the sky

  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];
   }
   ...
 } 

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

Here, apparently a simple typo. Randomly used the index '3' instead of '2'. The consequences, I think, are clear.

Error resulting in slower work

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

   for (i = 0; i <len; i ++) {
     if (mask <0) continue;
     norm = 1.0f / sqrt (vec [0] [i] * vec [0] [i] +
            vec [1] [i] * vec [1] [i] +
            vec [2] [i] * vec [2] [i]);
            vec [0] [i] * = norm;  vec [1] [i] * = norm;  vec [2] [i] * = norm;
   }
 } 

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

This is a beautiful example of code that, because of an error, runs slower than it could. The algorithm should normalize only those elements that are marked in the array of masks. But the above code does the normalization of all elements. The error is in the condition "if (mask <0)". They forgot to use the index "i". The “mask” pointer will almost always be greater than or equal to zero, which means that we will process all the elements.

Correct check:

  if (mask [i] <0) continue; 

The result of the subtraction is always 0

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

PVS-Studio diagnostic message: V501 operator - state:> freq - state -> freq speech ec_fb.c 250

Here, because of a typo, the function will always return a value of 0. Something is not right here we subtract. What I need to deduct, I do not know.

Invalid buffer shortage processing

  typedef unsigned int Ipp32u;

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

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

Incorrectly, the situation when the buffer size is insufficient for work is processed. Instead of returning the error code, the program will continue to work and most likely will crash. The fact is that the variable "memSize" is of type "unsigned int". Therefore, the “memSize <0” condition is always false and we continue to work with an insufficiently sized buffer.

This is probably a good example of the program’s vulnerability to attack. Having slipped the incorrect data, you can overflow the buffer and try to use it. By the way, there were about 10 such vulnerabilities. I will not cite them in order not to overload the text.

Incorrect checkup and, as a consequence, going beyond the array boundary.

  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]);
 } 

PVS-Studio 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 variable "m_iCurrMBIndex" is of type "unsigned". Because of this, the expression "m_iCurrMBIndex - x" also has the type "unsigned". Therefore, the condition "m_iCurrMBIndex - x <0" is always false. Consider the consequences.

Let the variable "m_iCurrMBIndex" be 5, and the variable "x" be 10.

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

The condition “m_iCurrMBIndex - x <0” is false.

The expression “m_MBInfo [row] [0xFFFFFFFBu]” is executed and the array is exceeded.

Error using ternary operator '?:'.

The ternary operator is dangerous enough because it is easy to make a mistake. However, programmers like to write shorter and use interesting language design. C ++ language punishes this.

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

PVS-Studio diagnostic message: V502 Perhaps the '?:' Operator works in different ways. The '?:' Operator has a lower than the '|' operator. vm vm_file_win.c 393

The code must be a combination of the FILE_ATTRIBUTE_NORMAL and FILE_FLAG_NO_BUFFERING flags. But in fact, the element "mds [3]" is always assigned the value 0.

The programmer has forgotten that the operator priority is "|" higher than the priority of the operator "?:". It turns out that the following expression is written in the code (note the brackets):

(FILE_ATTRIBUTE_NORMAL | (islog == 0))?

0: FILE_FLAG_NO_BUFFERING;

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

The correct expression should look like this (pay attention to the brackets again):

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

Strange work with an array

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

PVS-Studio 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 the reader to study the code and draw conclusions. I will describe this code in one word only - “original”.

Paranormal assignments

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

PVS-Studio 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

The reason for the appearance of such a strange code is incomprehensible to me. And this block is repeated in different functions 8 times!

There are other suspicious assignments of one variable:

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

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

Conclusion

This article lists only a fraction of the errors that I discovered in IPP Samples for Windows. I did not cite some mistakes, as they are twin brothers of the examples I reviewed in the article, and it will not be interesting to read about them. I did not give here minor errors. An example would be assert (), in which the condition is always true due to a typo. I missed many parts of the code, because I just don’t know if this is a mistake or just plain ugly. However, I think that the described defects are enough to show the difficulty of writing large projects, even by professional developers.

Once again I will formulate the thought voiced at the beginning of the article. Even a good programmer is not immune from typos, forgetfulness, the desire to make Copy-Paste and mistakes in logic. I think, in the future, a link to this article will be a good answer for people who are sure that pronouncing the phrase “you must write the code correctly” will protect them from any mistakes.

Good luck to everyone in your C / C ++ / C ++ 0x projects. And I wish you to find more mistakes using the methodology of static analysis that I love so much.