Прогресс не стоит на месте. Развивается и мой любимый статический анализатор кода PVS-Studio. И недавно я понял, что те проекты, которые мы уже проверяли, можно вполне проверять заново. Писать про это новые статьи как-то странно и вряд ли они получатся интересными. Однако одну такую статью я всё-таки думаю надо сделать. Она станет еще одним аргументом в пользу утверждения, что настоящую пользу от статического анализа можно получить только при его регулярном использовании, а не при проверках от случая к случаю. Итак, посмотрим, что же удалось найти нового интересного в проекте Intel IPP Samples.
Предыдущая заметка «Intel IPP Samples for Windows — работа над ошибками» [1] была опубликована 27 января 2011 года. Прошло около 9 месяцев. За это время в IPP Samples были поправлены многие ошибки, описанные в статье. Вышло несколько обновлений библиотеки. Однако поскольку, разработчики из Intel не заинтересовались в PVS-Studio, то проверка получилась разовая. И теперь можно хорошо увидеть, какие новые интересные ошибки найдет анализатор через 9 месяцев своего развития.
Не будем предаваться пустословию, мы ведь программисты, и перейдем побыстрее к примерам кода. Анализ выполнялся PVS-Studio 4.37 (предыдущая заметка относится к версии 4.10). Конечно, приведены будут не все дефекты, а только интересные и не сильно повторяющиеся. Тот, кто хочет увидеть все подводные камни, может воспользоваться PVS-Studio и изучить отчёт. Но мы здесь не для этого.
Классическое неопределенное поведение
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:
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
Просто канонический пример, который приводится в статьях для демонстрации неопределенного поведения [2]. Невозможно сказать, увеличатся переменные pTemp и pDst или нет, так как они два раза изменяются в рамках одной точки следования. Результат зависит от компилятора и настроек оптимизации.
Есть и дугой аналогичный фрагмент кода:
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:
V567 Undefined behavior. The 'm_curIndex' variable is modified while being used twice between sequence points. vc1_enc umc_vc1_enc_planes.h 630
А вот еще один пример неопределенного поведения. Хотя здесь используется префиксный инкремент, по сути, это ничего не меняет. Все равно здесь одна точка следования, а переменная m_curIndex изменяется 2 раза. Теоретически компилятор вправе создать следующий псевдокод:
A = m_curIndex + 1;
B = A % m_maxN;
m_curIndex = B;
m_curIndex = A;
На практике это вряд ли произойдет, и переменная сразу будет увеличена, но полагаться на это никак нельзя.
Опечатка в имени объекта
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:
V501 There are identical sub-expressions '(srcB->depth == 16)' to the left and to the right of the '||' operator. ipl iplmpy2d.c 457
Если присмотреться к коду, то можно заметить вкравшуюся опечатку. Потерялась проверка: "(srcA->depth == IPL_DEPTH_16U)".
Неполная очистка буфера
UMC::Status
VC1EncoderADV::SetMEParams_I_Field(UMC::MeParams* MEParams)
{
UMC::Status umcSts UMC::UMC_OK;
memset(MEParams,0,sizeof(MEParams));
...
}Диагностическое сообщение PVS-Studio:
V512 A call of the 'memset' function will lead to underflow of the buffer 'MEParams'. vc1_enc umc_vc1_enc_adv.cpp 1767
Очищается только часть буфера, так как «sizeof(MEParams)» возвращает размер указателя, а не структуры. Для вычисления корректного размера указатель необходимо разыменовать: «sizeof(*MEParams)».
Ошибка Copy-Paste
Status VC1VideoDecoder::ResizeBuffer()
{
...
if(m_pContext && m_pContext->m_seqLayerHeader &&
m_pContext->m_seqLayerHeader->heightMB &&
m_pContext->m_seqLayerHeader->heightMB)
...
}Диагностическое сообщение PVS-Studio:
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
Скорее всего, для упрощения строчку скопировали, но забыли поправить. Как мне кажется, здесь должно было быть так:
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:
V557 Array overrun is possible. The value of 'j' index could reach 2. mp3_enc mp3enc_psychoacoustic_fp.c 361
Этот код приводит к ошибке сегментации. Переменная 'j' может принимать значение 2. Однако допустимыми индексами являются только числа 0 и 1. Скорее всего, цикл должен был выглядеть так:
for (j = 0; j < 2; j++)Были найдены и другие циклы, которые, скорее всего, приведут к выходу за рамки массивов.
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:
V557 Array overrun is possible. The value of 't * 12 + j' index could reach 35. mp3_enc mp3enc_quantization_12_fp.c 275
Если возможна ситуация, что t == 2, а j == 11, то произойдет выход за границы массива. Как должен был выглядеть этот код, я затрудняюсь ответить.
С использованием массива 'samplefbout' вообще не всё гладко. Ещё один фрагмент кода:
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:
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:
V535 The variable 'c' is being used for this loop and for the outer loop. jpegcodec jpegdec.cpp 4652
Проблема этого кода в том, что переменная 'c' используется одновременно во внешнем и вложенном цикле. В зависимости от граничных значений циклов, этот код может обработать не все данные или привести к бесконечному циклу.
Неисправленные ошибки
Многие ошибки, описанные в первой статье, были поправлены. Но на некоторые разработчики IPP Samples или не обратили внимания или посчитали, что это не ошибки. Например, подобная ситуация вот с этим фрагментом:
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:
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
Странный код
Есть немало участков кода, про которые сложно сказать, настоящая ошибка это или просто избыточный код. Приведу несколько примеров.
int ec_fb_GetSubbandNum(void* stat)
{
_fbECState* state = (_fbECState*)stat;
return (state->freq - state->freq);
}Диагностическое сообщение PVS-Studio:
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
Очень странная функция. Здесь или странным образом боролись с неиспользуемыми переменными, или оператор 'return' должен возвращать результат другого выражения.
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:
V523 The 'then' statement is equivalent to the 'else' statement. aac_dec als_dec_api.c 923
Ну что тут сказать? Подозрительно! Зачем нужны два идентичных цикла при разных условиях?
void rrGetNextBunch_Spiral(...)
{
int x,y;
...
if(x < 0)
if(x < 0) goto _begine;
...
if(y < 0)
if(y < 0) goto _begine;
...
}Диагностические сообщения PVS-Studio:
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
Странные дублирующиеся проверки.
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:
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
Случайно скопировали строку? Или хотели обнулить другую переменную? Непонятно.
Заключение
Попробуйте интегрировать статический анализ в процесс разработки своих проектов. Поначалу это кажется лишней тратой усилий. Но потом вы прочувствуете, как необычно и приятно получить сообщение об ошибке, еще до того момента как вы начнете тестировать только что написанный фрагмент кода. А ещё вы будете с интересом наблюдать, найдется что-то или нет после выхода очередной версии анализатора.
Библиографический список
- Андрей Карпов. Intel IPP Samples for Windows — работа над ошибками. http://www.viva64.com/go.php?url=741
- Елена Сагалаева. Точки следования (sequence points). http://www.viva64.com/go.php?url=665
