Проверка Intel IPP Samples for Windows - продолжение

• Классическое неопределенное поведение
• Неопределенное поведение и префиксный инкремент
• Опечатка в имени объекта
• Неполная очистка буфера
• Ошибка Copy-Paste
• Выход за границы массива
• Использование одной переменной для двух циклов
• Неисправленные ошибки
• Странный код
• Заключение
• Библиографический список

Прогресс не стоит на месте. Развивается и мой любимый статический анализатор кода 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/ru/a/0069/
• Елена Сагалаева. Точки следования (sequence points). http://alenacpp.blogspot.ru/2005/11/sequence-points.html