Check Intel IPP Samples for Windows - continued

Progress does not stand still. My favorite static code analyzer PVS-Studio is also developing. And recently I realized that those projects that we have already tested can be completely checked again. Writing about this new article is somehow strange and is unlikely to be interesting. However, I still need to do one such article. It will be another argument in favor of the assertion that the real benefits of static analysis can be obtained only with its regular use, and not in case-by-case checks. So, let's see what we managed to find the new interesting in the Intel IPP Samples project.

The previous post “Intel IPP Samples for Windows - work on the bugs” [ 1 ] was published on January 27, 2011. It took about 9 months. During this time, many of the errors described in the article were corrected in IPP Samples. There were several library updates. However, since the developers from Intel were not interested in PVS-Studio, the test turned out to be a one-time. And now you can see well what new interesting errors the analyzer will find after 9 months of its development.

We will not indulge in idle talk, because we are programmers, and we will quickly move on to code examples. The analysis was performed by PVS-Studio 4.37 (the previous note relates to version 4.10). Of course, not all defects will be given, but only interesting and not very repetitive. Anyone who wants to see all the pitfalls can use PVS-Studio and study the report. But we are not here for this.

Classic indefinite behavior

template<typename T, Ipp32s size> void HadamardFwdFast(..., Ipp16s* pDst) { Ipp32s *pTemp; ... for(j=0;j<4;j++) { a[0] = pTemp[0*4] + pTemp[1*4]; a[1] = pTemp[0*4] - pTemp[1*4]; a[2] = pTemp[2*4] + pTemp[3*4]; a[3] = pTemp[2*4] - pTemp[3*4]; pTemp = pTemp++; pDst[0*4] = (Ipp16s)(a[0] + a[2]); pDst[1*4] = (Ipp16s)(a[1] + a[3]); pDst[2*4] = (Ipp16s)(a[0] - a[2]); pDst[3*4] = (Ipp16s)(a[1] - a[3]); pDst = pDst++; } ... } 

PVS-Studio diagnostic messages:
')
V567 Undefined behavior. The 'pTemp' variable me umc_me_cost_func.h 168

V567 Undefined behavior. The variable is variable. me umc_me_cost_func.h 174

Just a canonical example, which is given in articles to demonstrate undefined behavior [ 2 ]. It is impossible to say whether the variables pTemp and pDst will increase or not, as they change twice within the same sequence point. The result depends on the compiler and optimization settings.

There is also an arc of similar code snippet:

 void VC1BRC_I::CompleteFrame(ePType picType) { ... m_Quant.LimIQuant = m_Quant.LimIQuant--; ... m_Quant.IQuant = m_Quant.IQuant--; ... } 

Undefined behavior and prefix increment

 bool MoveOnNextFrame() { if (m_nFrames>0) { m_pFrame[m_curIndex] = 0; m_curIndex = (++m_curIndex)%m_maxN; m_nFrames--; return true; } return false; } 

PVS-Studio diagnostic message:

V567 Undefined behavior. The 'm_curIndex' variable being modified while being used between sequence points. vc1_enc umc_vc1_enc_planes.h 630

And here is another example of undefined behavior. Although the prefix increment is used here, in fact, it changes nothing. All the same here is one point of the sequence, and the variable m_curIndex changes 2 times. Theoretically, the compiler can create the following pseudocode:

A = m_curIndex + 1;

B = A% m_maxN;

m_curIndex = B;

m_curIndex = A;

In practice, this is unlikely to happen, and the variable will be immediately increased, but you can’t rely on it at all.

Typo in object name

 IPLFUN(void, iplMpyRCPack2D, (IplImage* srcA, IplImage* srcB, IplImage* dst)) { ... if( (srcA->depth == IPL_DEPTH_8U ) || (srcB->depth == IPL_DEPTH_8U ) || (srcB->depth == IPL_DEPTH_16U) || (srcB->depth == IPL_DEPTH_16U) || (srcA->depth == IPL_DEPTH_1U ) || (srcB->depth == IPL_DEPTH_1U ) ) ... } 

PVS-Studio diagnostic message:

V501 There are identical sub-expressions' (srcB-> depth == 16) operator. ipl iplmpy2d.c 457

If you look at the code, you can see crept typo. Lost verification: "(srcA-> depth == IPL_DEPTH_16U)".

Partial buffer flush

 UMC::Status VC1EncoderADV::SetMEParams_I_Field(UMC::MeParams* MEParams) { UMC::Status umcSts UMC::UMC_OK; memset(MEParams,0,sizeof(MEParams)); ... } 

PVS-Studio diagnostic message:

V512 A call of the memset function will lead to underflow of the buffer MEParams. vc1_enc umc_vc1_enc_adv.cpp 1767

Only part of the buffer is cleared, since sizeof (MEParams) returns the size of the pointer, not the structure. To calculate the correct size, the pointer must be dereferenced: “sizeof (* MEParams)”.

Copy-Paste Error

 Status VC1VideoDecoder::ResizeBuffer() { ... if(m_pContext && m_pContext->m_seqLayerHeader && m_pContext->m_seqLayerHeader->heightMB && m_pContext->m_seqLayerHeader->heightMB) ... } 

PVS-Studio diagnostic message:

V501 There are identical sub-expressions 'm_pContext-> m_seqLayerHeader-> heightMB' the operator. vc1_dec umc_vc1_video_decoder.cpp 1351

Most likely, to simplify the line copied, but forgot to fix it. It seems to me that it should have been like this:

 if(m_pContext && m_pContext->m_seqLayerHeader && m_pContext->m_seqLayerHeader->heightMB && m_pContext->m_seqLayerHeader->widthMB) 

Array bounds

 Ipp32f pa_nb_long[NUM_CHANNELS][2][MAX_PPT_LONG]; MP3Status mp3enc_psychoacousticInit(...) { ... for (ch = 0; ch < NUM_CHANNELS; ch++) for (i = 0; i < MAX_PPT_LONG; i++) { for (j = 0; j < 3; j++) state->pa_nb_long[ch][j][i] = (Ipp32f)1.0e30; } ... } 

PVS-Studio diagnostic message:

V557 Array overrun is possible. The value of 'j' index could reach 2. mp3_enc mp3enc_psychoacoustic_fp.c 361

This code causes a segmentation fault. The variable 'j' can take the value 2. However, only the numbers 0 and 1 are valid. Most likely, the loop should look like this:

 for (j = 0; j < 2; j++) 

Were found and other cycles that are likely to lead to going beyond the arrays.

 typedef Ipp32f samplefbout[2][18][32]; samplefbout fbout_data[NUM_CHANNELS]; static void mp3enc_scale_factor_calc_l2(MP3Enc *state) { ... for (ch = 0; ch < stereo + state->com.mc_channel; ch++) { for (t = 0; t < 3; t++) { for (sb = 0; sb < sblimit_real; sb++){ for (j = 0; j < 12; j++) fbout[j] = state->fbout_data[ch][0][t * 12 + j][sb]; ... } 

PVS-Studio diagnostic message:

V557 Array overrun is possible. The value of 't * 12 + j' index could reach 35. mp3_enc mp3enc_quantization_12_fp.c 275

If the situation is possible, that t == 2, and j == 11, then the array will go beyond the bounds. How this code should look like, I find it difficult to answer.

Using the 'samplefbout' array doesn't do everything at all smoothly. Another code snippet:

 typedef Ipp32f samplefbout[2][18][32]; samplefbout fbout_data[NUM_CHANNELS]; static void mp3enc_join_LR_l2(MP3Enc *state) { Ipp32s sb, j; Ipp32s sblimit_real = state->com.sblimit_real; for (sb = 0; sb < sblimit_real; sb++) for (j = 0; j < 36; j++) state->fbout_data[2][0][j][sb] = 0.5f * (state->fbout_data[0][0][j][sb] + state->fbout_data[1][0][j][sb]); } 

PVS-Studio diagnostic messages:

V557 Array overrun is possible. The value of 'j' index could reach 35. mp3_enc mp3enc_quantization_12_fp.c 639

V557 Array overrun is possible. The value of 'j' index could reach 35. mp3_enc mp3enc_quantization_12_fp.c 640

Using one variable for two cycles

 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. jpegcodec jpegdec.cpp 4652

The problem with this code is that the 'c' variable is used in both the outer and nested loops. Depending on the boundary values ​​of the cycles, this code may not process all the data or lead to an infinite loop.

Uncorrected errors

Many of the errors described in the first article were corrected. But some IPP Samples developers either didn’t pay attention or thought that these were not errors. For example, a similar situation with this fragment:

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

PVS-Studio diagnostic message:

V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower than the '|' operator. vm vm_file_win.c 393

Strange code

There are quite a few sections of code that are hard to say, a real error or just a redundant code. I will give a few examples.

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

PVS-Studio diagnostic message:

V501 There are identical to the left and the right of the '-' operator: state-> freq - state-> freq speech ec_fb.c 253

Very strange feature. Here, or strangely struggled with unused variables, or the operator 'return' should return the result of another expression.

 AACStatus alsdecGetFrame(...) { ... if (state->msbFirst == 0) { for (i = 0; i < num; i++) { *tmpPtr = *srcPrt; tmpPtr += state->numChannels; srcPrt++; } } else { for (i = 0; i < num; i++) { *tmpPtr = *srcPrt; tmpPtr += state->numChannels; srcPrt++; } } ... } 

PVS-Studio diagnostic message:

V523 The 'then' statement is equivalent to the 'else' statement. aac_dec als_dec_api.c 923

Well what can I say? Suspicious! Why do we need two identical cycles under different conditions?

 void rrGetNextBunch_Spiral(...) { int x,y; ... if(x < 0) if(x < 0) goto _begine; ... if(y < 0) if(y < 0) goto _begine; ... } 

PVS-Studio diagnostic messages:

V571 Recurring check. The 'if (x <0)' condition was already verified in line 1025. 3d-viewer rrdemosupport.cpp 1026

V571 Recurring check. The 'if (y <0)' condition was already verified in line 1028. 3d-viewer rrdemosupport.cpp 1029

Strange duplicate checks.

 Status H264ENC_MAKE_NAME(H264CoreEncoder_UpdateRefPicMarking) (void* state) { ... // set frame_num to zero for this picture, for correct // FrameNumWrap core_enc->m_pCurrentFrame->m_FrameNum = 0; core_enc->m_pCurrentFrame->m_FrameNum = 0; ... } 

PVS-Studio diagnostic message:

V519 The 'core_enc-> m_pCurrentFrame-> m_FrameNum' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1804, 1805. h264_enc umc_h264_video_encoder_tmpl.cpp.h 1805

Randomly copied the string? Or would you like to reset another variable? Unclear.

Conclusion

Try integrating static analysis into your project development process. At first, this seems like a waste of effort. But then you will feel how unusual and pleasant it is to receive an error message, even before you start testing a newly written code snippet. You will also observe with interest whether something will be found or not after the release of the next version of the analyzer.

Bibliographic list

1. Andrey Karpov. Intel IPP Samples for Windows - work on the bugs. http://www.viva64.com/go.php?url=741
2. Elena Sagalaeva. Sequence points. http://www.viva64.com/go.php?url=665