Это моя очередная заметка о том, как PVS-Studio делает программы более надёжными. То есть где, и какие ошибки он обнаруживает. На этот раз под молоток попали примеры, демонстрирующие работу с библиотекой IPP 7.0 (Intel Performance Primitives Library).
В состав Intel Parallel Studio 2011 входит библиотека Performance Primitives Library. Эта библиотека включает в себя большое количество примитивов, позволяющих создавать эффективные видео и аудио кодеки, программы обработки сигналов, механизмы рендеринга изображений, архиваторы и многое-многое другое. Естественно, с такой библиотекой работать не просто. Поэтому компания Intel подготовила большое количество демонстрационных программ, построенных на основе этой библиотеки. Вы можете познакомиться с описанием примеров и скачать их по следующим ссылкам: Code Samples for the Intel Integrated Performance Primitives (Intel IPP) Library
Все примеры разбиты на четыре группы:
В каждом наборе находится большое количество проектов, так что для начала я взял для проверки только первый набор IPP Samples for Windows. Проверку я осуществил, используя PVS-Studio версии 4.10.
Этим постом я хочу показать, что статический анализ полезен вне зависимости от профессионализма программистов и уровня разрабатываемого решения. Идея "надо брать профессионалов и писать сразу без ошибок" не работает. Даже высококвалифицированные разработчики не застрахованы от ошибок и опечаток в процессе кодирования. Ошибки в примерах для IPP очень хорошо это демонстрируют.
Подчеркну, что IPP Samples for Windows является высококачественным проектом. Однако, в силу своего размера в 1.6 миллионов строк кода, он неизбежно содержит в себе разнообразнейшие ошибки. Рассмотрим некоторые из них.
Этот пример мог бы замечательно пополнить мою предыдущую статью "Последствия использования технологии Copy-Paste при программировании на Си++ и как с этим быть":
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;
}
...
}Диагностическое сообщение 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
Программист несколько раз скопировал фрагмент кода и изменил значение индексов массивов. Но в самом конце его рука дрогнула. Он вписал число 3, но не удалил число 0. В результате получился индекс 30 и при исполнении кода произойдет доступ далеко за границы массива.
Раз мы начали с копирования кода, то вот ещё один пример на эту тему:
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: V523 The 'then' statement is equivalent to the 'else' statement. aac_enc aac_enc_api_fp.c 1379
Но в этот раз, наоборот, забыли отредактировать скопированный код. Обе ветки условного оператора "if" выполняют одни и те же действия.
static void
sbrencConflictResolution (..., Ipp32s *nLeftBord)
{
...
*nLeftBord = nBordNext - 1;
...
if (*lenBordNext > 1) {
...
*nLeftBord--;
}
...
}Диагностическое сообщение PVS-Studio: V532 Consider inspecting the statement of '*pointer--' pattern. Probably meant: '(*pointer)--'. aac_enc sbr_enc_frame_gen.c 428
Указатель "nLeftBord" использует для возврата значения из функции "sbrencConflictResolution". Сначала по указанному адресу записывается значение "nBordNext - 1". При определенных условиях это значение должно уменьшаться на единицу. Для уменьшения значения программист использовал следующий код:
*nLeftBord--;Ошибка в том, что вместо значения уменьшается сам указатель. Корректный код должен выглядеть так:
(*nLeftBord)--;Следующий код мне совершенно не понятен. Я не знаю, как его исправить, чтобы он приобрел смысл. Возможно, здесь чего-то не хватает.
static IppStatus mp2_HuffmanTableInitAlloc(Ipp32s *tbl, ...)
{
...
for (i = 0; i < num_tbl; i++) {
*tbl++;
}
...
}Диагностическое сообщение PVS-Studio: V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. mpeg2_dec umc_mpeg2_dec.cpp 59
Сейчас, приведенный в примере цикл, эквивалентен следующему коду:
tbl += num_tbl;Анализатор PVS-Studio предположил, что ,возможно, здесь забыты скобки и следовало написать "(*tbl)++;". Но и этот код не имеет смысла. Тогда цикл будет эквивалентен:
*tbl += num_tbl;В общем, очень странный какой-то цикл. Ошибка есть, но исправить ее, видимо, может только автор кода.
В коде имеется функция "GetTrackByPidOrCreateNew", которая возвращает значение "-1" , если возникает ошибка.
typedef signed int Ipp32s;
typedef unsigned int Ipp32u;
Ipp32s StreamParser::GetTrackByPidOrCreateNew(
Ipp32s iPid, bool *pIsNew)
{
...
else if (!pIsNew || m_uiTracks >= MAX_TRACK)
return -1;
...
}С функцией "GetTrackByPidOrCreateNew" всё нормально. Но имеется ошибка при её использовании:
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: V547 Expression '* pTrack >= 0' is always true. Unsigned type value is always >= 0. demuxer umc_stream_parser.cpp 179
Значение, которое возвращает функция "GetTrackByPidOrCreateNew" сохраняется в беззнаковом формате (unsigned int). Это значит, что "-1" превратится в "4294967295", а условие "*pTrack >= 0" всегда истинно.
В итоге, если функция "GetTrackByPidOrCreateNew " вернет значение "-1", то произойдет Access Violation при выполнении кода "m_pInfo[*pTrack]->m_Type".
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: V523 The 'then' statement is equivalent to the 'else' statement. h264_dec umc_h264_segment_decoder_deblocking_mbaff.cpp 340
Если посмотреть код рядом, то становится понятно, что в скопированной строчке забыли прибавить единицу. Корректный код должен выглядеть так:
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;Неподалеку в функции "H264CoreEncoder_ResetDeblockingVariablesMBAFF" есть ещё в точности такая же ошибка с забытым "+ 1".
Диагностическое сообщение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. h264_enc umc_h264_deblocking_mbaff_tmpl.cpp.h 366
void H264ThreadGroup::RemoveThread(H264Thread * thread)
{
AutomaticUMCMutex guard(m_mGuard);
std::remove(m_threads.begin(), m_threads.end(), thread);
}Диагностическое сообщение PVS-Studio: V530 The return value of function 'remove' is required to be utilized. h264_dec umc_h264_thread.cpp 226
Интересное сочетание. С одной стороны, всё солидно. Используется mutex, чтобы корректно удалять элемент в многопоточном приложении. А с другой стороны, банально забыли, что функция std::remove не удаляет элементы из массива, а только меняет их порядок. На самом деле должно быть написано так:
m_threads .erase(
std::remove(m_threads.begin(), m_threads.end(), thread),
m_threads.end());Смотрю на ошибки и обратил внимание, что реализация стандарта сжатия видео H264 какая-то глючноватая. К этому проекту относится достаточно большое количество найденных ошибок. Вот, например, кто-то спешил при программировании и использовал сразу два неверных имени переменных.
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:
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
Функция сравнения не работает, так как некоторые члены структуры сравниваются сами с собою. Вот две исправленные строчки:
(p_newHeader.pic_parameter_set_id != m_lastSlice.pic_parameter_set_id)
(p_newHeader.field_pic_flag != m_lastSlice.field_pic_flag)Бывают ошибки использования не тех объектов, не только при сравнении, но и при копировании состояний объектов:
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: V537 Consider reviewing the correctness of 'sprite_left_coordinate' item's usage. mpeg4_enc mp4_enc_misc.cpp 387
В "VOL.sprite_top_coordinate" помещается неверное значение. Корректное присваивание должно выглядеть так:
VOL.sprite_top_coordinate = par->sprite_top_coordinate;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'. Результат работы такой функции декодирования может быть весьма странным и неожиданным.
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: V519 The 'MaxBrefPOC' object is assigned values twice successively. Perhaps this is a mistake. h264_enc umc_h264_enc_cpb_tmpl.cpp.h 784
Когда я увидел этот код, то мне вспомнился старый программистский анекдот:
- А почему у тебя в коде подряд два одинаковых GOTO стоят?
- А вдруг первый не сработает!
Данная ошибка, пожалуй, не критична, но все-таки это ошибка.
AACStatus sbrencResampler_v2_32f(Ipp32f* pSrc, Ipp32f* pDst)
{
...
k = nCoef-1;
k = nCoef;
...
}Диагностическое сообщение PVS-Studio: V519 The 'k' object is assigned values twice successively. Perhaps this is a mistake. aac_enc sbr_enc_resampler_fp.c 90
Это двойное присваивание настораживает гораздо больше, чем в предыдущем примере. Такое ощущение, что программист был не уверен в себе. Или решил, в начале, попробовать "nCoef-1", а потом "nCoef". Ещё это называют "программированием методом эксперимента". И в любом случае, это именно то место в коде, увидев которое следует задержаться и предаться размышлениям.
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: 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
Вновь опечатка в индексе массива. Последний индекс должен быть 3, а не 2. Корректный вариант кода:
Ipp32s BestAC= IPP_MIN(IPP_MIN(m_cur.AcRate[0],m_cur.AcRate[1]),
IPP_MIN(m_cur.AcRate[2],m_cur.AcRate[3]));Подобные ошибки неприятны тем, что код "почти работает". Ошибка проявит себя только в том случае, если минимальный элемент хранится в "m_cur.AcRate[3]". Такие ошибки любят проявлять себя не при тестировании, а у пользователя на их наборах входных данных.
С максимальными значениями тоже не всегда ладится:
Ipp32s ippVideoEncoderMPEG4::Init(mp4_Param *par)
{
...
i = IPP_MAX(mBVOPsearchHorBack, mBVOPsearchHorBack);
...
}Диагностическое сообщение PVS-Studio: V501 There are identical sub-expressions '(mBVOPsearchHorBack)' to the left and to the right of the '>' operator. mpeg4_enc mp4_enc_misc.cpp 547
Два раза используется переменная mBVOPsearchHorBack. На самом деле планировалось использовать mBVOPsearchHorBack и mBVOPsearchVerBack:
i = IPP_MAX(mBVOPsearchHorBack, mBVOPsearchVerBack);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];
}
...
}Диагностическое сообщение PVS-Studio: V557 Array overrun is possible. The '3' index is pointing beyond array bound. mp3_enc mp3enc_psychoacoustic_fp.c 726
Здесь видимо простая опечатка. Случайно использовали индекс '3' вместо '2'. Последствия, думаю, понятны.
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: V503 This is a nonsensical comparison: pointer < 0. ipprsample ippr_sample.cpp 501
Это красивый пример кода, который из-за ошибки работает медленнее, чем мог бы. Алгоритм должен нормализовать только те элементы, которые отмечены в массиве масок. Но приведенный код делает нормализацию всех элементов. Ошибка находится в условии "if(mask<0)". Здесь забыли использовать индекс "i". Указатель "mask" будет практически всегда больше или равен нулю, а ,значит, мы обработаем все элементы.
Корректная проверка:
if(mask[i]<0) continue;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 250
Здесь из-за опечатки функция всегда будет возвращать значение 0. Что-то не то мы здесь вычитаем. Что нужно вычитать, я не знаю.
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: V547 Expression 'memSize < 0' is always false. Unsigned type value is never < 0. vc1_enc umc_vc1_enc_planes.h 200
Неверно, обрабатывается ситуация, когда размер буфера для работы недостаточен. Вместо возврата кода ошибки программа продолжит работу и скорее всего, аварийно завершится. Дело в том, что переменная "memSize" имеет тип "unsigned int". Следовательно, условие "memSize < 0" всегда ложно и мы продолжаем работу с буфером недостаточного размера.
Наверное, это хороший пример уязвимости программы к атаке. Подсунув некорректные данные, можно переполнить буфер и попробовать воспользоваться этим. Кстати, таких уязвимых мест нашлось около 10. Приводить их не буду, чтобы не загромождать текст.
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: V547 Expression 'm_iCurrMBIndex - x < 0' is always false. Unsigned type value is never < 0. vc1_enc umc_vc1_enc_mb.cpp 188
Переменная "m_iCurrMBIndex" имеет тип "unsigned". Из-за этого выражение "m_iCurrMBIndex - x" также имеет тип "unsigned". Следовательно, условие "m_iCurrMBIndex - x < 0" всегда ложно. Рассмотрим последствия.
Пусть переменная "m_iCurrMBIndex" равна 5, а переменная "x" равна 10.
Выражение "m_iCurrMBIndex - x" равно 5u - 10i = 0xFFFFFFFBu.
Условие "m_iCurrMBIndex - x < 0" имеет значение false.
Выполняется выражение "m_MBInfo[row][0xFFFFFFFBu]" и происходит выход за границу массива.
Тернарный оператор достаточно опасен, так как легко допустить ошибку. Тем не менее, программисты любят написать покороче и использовать интересную конструкцию языка. Язык Си++ наказывает за это.
vm_file* vm_file_fopen(...)
{
...
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
Код должен составлять комбинацию из флагов FILE_ATTRIBUTE_NORMAL и FILE_FLAG_NO_BUFFERING. Но на самом деле элементу "mds[3]" всегда присваивается значение 0.
Программист забыл, что приоритет оператора "|" выше, чем приоритет оператора "?:". Получается, что в коде написано следующее выражение (обратите внимание на скобки):
(FILE_ATTRIBUTE_NORMAL | (islog == 0)) ?
0 : FILE_FLAG_NO_BUFFERING;Условие "FILE_ATTRIBUTE_NORMAL | (islog == 0)" всегда истинно и мы присваиваем элементу "mds[3]" значение 0.
Корректное выражение должно выглядеть так (вновь обратите внимание на скобки):
FILE_ATTRIBUTE_NORMAL |
((islog == 0) ? 0 : FILE_FLAG_NO_BUFFERING);AACStatus alsdecGetFrame(...)
{
...
for (i = 0; i < num; i++) {
...
*tmpPtr = (Ipp32s)((tmp << 24) + ((tmp & 0xff00) << 8) +
((tmp >> 8) & 0xff00) + (tmp >> 24));
*tmpPtr = *srcPrt;
...
}
...
}Диагностическое сообщение PVS-Studio: V519 The '* tmpPtr' object is assigned values twice successively. Perhaps this is a mistake. aac_dec als_dec_api.c 928
Предлагаю читателю самому изучить код и сделать выводы. Я опишу этот код только одним словом - "оригинально".
static
IPLStatus ownRemap8u_Pixel(...) {
...
saveXMask = xMap->maskROI;
saveXMask = NULL;
saveYMask = yMap->maskROI;
saveYMask = NULL;
...
}Диагностические сообщения PVS-Studio:
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
Причина появления такого странного кода мне непонятна. Причем подобный блок повторяется в разных функциях 8 раз!
Встречаются и другие подозрительные присваивания одной переменной:
Ipp32s ippVideoEncoderMPEG4::Init(mp4_Param *par)
{
...
mNumOfFrames = par->NumOfFrames;
mNumOfFrames = -1;
...
}Диагностическое сообщение PVS-Studio: V519 The 'mNumOfFrames' object is assigned values twice successively. Perhaps this is a mistake. mpeg4_enc mp4_enc_misc.cpp 276
В этой статье перечислена только часть ошибок, обнаруженных мною в IPP Samples for Windows. Некоторые ошибки я не привел, так как они являются братьями-близнецами тех примеров, которые я рассмотрел в статье, и читать про них будет не интересно. Я не стал приводить здесь несущественные ошибки. Примером может служить assert(), у которого из-за опечатки условие всегда истинно. Многие участков кода я пропустил, так как просто не знаю, ошибка это или просто некрасиво написано. Однако я думаю, что описанных дефектов достаточно, чтобы показать сложность написания больших проектов даже профессиональными разработчиками.
Ещё раз сформулирую мысль, озвученную в начале статьи. Даже хороший программист не застрахован от опечаток, забывчивости, желания сделать Copy-Paste и ошибок в логике. Я думаю, в будущем ссылка на эту статью станет хорошим ответом людям, которые уверены что произнесение фразы "надо правильно писать код" защитит их от любых ошибок.
Удачи всем в ваших C/C++/C++0x проектах. И желаю вам находить побольше ошибок, используя так любимую мной методологию статического анализа.