The progress keeps on going. My favorite static code analyzer PVS-Studio is developing too. It has occurred to me recently that those projects we already checked, we can well check again. It would be strange if we wrote articles on this topic, and they would hardly be interesting. But I think we can write one: it will become one more argument for the idea that you can get real benefit from static analysis only using it regularly and not from time to time. So, let's see what new interesting things we have managed to find in the Intel IPP Samples project.
The previous post "Intel IPP Samples for Windows - error correction" [1] was published on 27th January, 2011. About 9 months have passed since then. During this time, developers have fixed many errors in IPP Samples described in the article. Since Intel developers had showed no interest in PVS-Studio, the check was performed only once. Now we can see clear what new interesting errors the analyzer has found after 9 months of development.
Let's not fall into idle talk, for we are developers at last. So let's go over to code samples. Analysis was performed by PVS-Studio 4.37 (the previous post refers to version 4.10). Of course, we will cite not all the defects but only interesting and not recurrent ones. Those who want to see all the issues, may use PVS-Studio and study the report. But our purpose is different this time.
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's diagnostic messages:
V567 Undefined behavior. The 'pTemp' variable is modified while being used twice between sequence points. me umc_me_cost_func.h 168
V567 Undefined behavior. The 'pDst' variable is modified while being used twice between sequence points. me umc_me_cost_func.h 174
It's just a canonical example cited in articles to demonstrate undefined behavior [2]. You cannot tell whether or not variables pTemp and pDst will be incremented because they are changed twice within one sequence point. The result depends on the compiler and optimization settings.
There is another similar code fragment:
void VC1BRC_I::CompleteFrame(ePType picType)
{
...
m_Quant.LimIQuant = m_Quant.LimIQuant--;
...
m_Quant.IQuant = m_Quant.IQuant--;
...
}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's diagnostic message:
V567 Undefined behavior. The 'm_curIndex' variable is modified while being used twice between sequence points. vc1_enc umc_vc1_enc_planes.h 630
Here you are another example of undefined behavior. Although prefix increment is being used here, it doesn't make the difference actually. There is still one sequence point, while the m_curIndex variable changes twice. Theoretically, the compiler might well create the following pseudocode:
A = m_curIndex + 1;
B = A % m_maxN;
m_curIndex = B;
m_curIndex = A;
It will hardly happen in practice and the variable will be incremented at once, but you should not rely on that.
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's diagnostic message:
V501 There are identical sub-expressions '(srcB->depth == 16)' to the left and to the right of the '||' operator. ipl iplmpy2d.c 457
If you look attentively at the code, you will notice a misprint creeping into the code. The check "(srcA->depth == IPL_DEPTH_16U)" is missing.
UMC::Status
VC1EncoderADV::SetMEParams_I_Field(UMC::MeParams* MEParams)
{
UMC::Status umcSts UMC::UMC_OK;
memset(MEParams,0,sizeof(MEParams));
...
}PVS-Studio's 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 pointer's size and not structure's size. To calculate the correct size, the pointer must be dereferenced: "sizeof(*MEParams)".
Status VC1VideoDecoder::ResizeBuffer()
{
...
if(m_pContext && m_pContext->m_seqLayerHeader &&
m_pContext->m_seqLayerHeader->heightMB &&
m_pContext->m_seqLayerHeader->heightMB)
...
}PVS-Studio's diagnostic message:
V501 There are identical sub-expressions 'm_pContext->m_seqLayerHeader->heightMB' to the left and to the right of the '&&' operator. vc1_dec umc_vc1_video_decoder.cpp 1351
Most likely, the programmer wanted to simplify the task and copied the string but forgot to fix it. I think there should be the following code here:
if(m_pContext && m_pContext->m_seqLayerHeader &&
m_pContext->m_seqLayerHeader->heightMB &&
m_pContext->m_seqLayerHeader->widthMB)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's 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 'j' variable can take value 2, but acceptable indexes are only 0 and 1. The loop most likely should look this way:
for (j = 0; j < 2; j++)We have found some other loops which seem to cause array overruns.
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's 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 it is possible that t == 2, while j == 11, an array overrun will occur. I cannot say what the correct version of this code is.
There are some problems about using the 'samplefbout' array. Here you are another code fragment:
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's 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
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's diagnostic message:
V535 The variable 'c' is being used for this loop and for the outer loop. jpegcodec jpegdec.cpp 4652
The problem with this code is that the 'c' variable is being used in an outer and a nested loop at the same time. This code might process only part of the data or result in an eternal loop depending on the loop's boundary values.
Many errors described in the first article have been fixed. But regarding some others, IPP Samples developers either paid no attention to them, or didn't take them as errors. For example, one of them is in the following 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'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 are many code fragments about which I just cannot say exactly whether there is a real error or just redundant code. Here you are some examples.
int ec_fb_GetSubbandNum(void* stat)
{
_fbECState* state = (_fbECState*)stat;
return (state->freq - state->freq);
}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 253
This is a very strange function. Either the developers were fighting unused variables in a strange way, or the 'return' operator must return a result of some other 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's diagnostic message:
V523 The 'then' statement is equivalent to the 'else' statement. aac_dec als_dec_api.c 923
What can you say here? It's suspicious! What for do you need two identical loops at 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's 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 duplicating 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's 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
A string was accidentally copied? Or it was intended to zero another variable? I don't know.
Try to integrate static analysis into the development process of your projects. It seems to be just a waste of efforts at first. But then you will feel it clear how unusual and pleasant it is to get an error message before you start testing a freshly written code fragment. And you will also watch with interest if something else can be found in your code after the new version of the analyzer is released.