Можем ли мы доверять используемым библиотекам?

• Начнём, например, с библиотеки OpenJPEG
• GDCM
• ITK
• Другие ошибки
• Заключение

Сейчас любое крупное приложение состоит из множества сторонних библиотек. Хочется поднять такую тему, как доверие к этим библиотекам. В книгах и статьях можно встретить очень много рассуждений о качестве кода, методах тестирования, методологиях разработки и так далее. Но я не помню, чтобы кто-то рассуждал о качестве кирпичей, из которых строятся приложения. Давайте немного поговорим об этом. Например, есть Medicine Insight Segmentation and Registration Toolkit (ITK). Мне кажется, он написан весьма качественно. По крайней мере, я заметил в коде весьма мало ошибок. Но я не могу сказать, что код используемых библиотек столь же качественен. Тогда вопрос. Насколько мы можем доверять таким системам? Есть повод для размышлений.

При разработке медицинских приложений все говорят о качестве, стандартах кодирования. При написании кода требуют придерживаться таких стандартов, как MISRA и так далее. Признаюсь, я плохо знаком с методологиями, используемыми при написании ответственных приложений. Однако, у меня есть подозрение, что часто вопрос используемых сторонних библиотек обходится стороной. Код приложения и код сторонних библиотек живут отдельными жизнями.

Такой вывод я делаю из своих субъективных наблюдений. Нередко мне попадаются очень качественные приложения, где я не могу найти и пяток серьезных ошибок. При этом, в составе таких приложений могут быть включены сторонние библиотеки отвратительного качества.

Предположим, врач поставит неправильный диагноз из-за артефактов на изображении, которые возникают вследствие ошибки в программном обеспечении. В такой случае, глубоко всё равно, была ошибка в самой программе или в библиотеке для работы с изображениями. Это повод подумать.

В очередной раз на такие размышления меня навело рассматривание исходных кодов проекта ITK:

Insight Segmentation and Registration Toolkit (ITK). ITK is an open-source, cross-platform system that provides developers with an extensive suite of software tools for image analysis. Developed through extreme programming methodologies, ITK employs leading-edge algorithms for registering and segmenting multidimensional data.

Проверяя проект ITK с помощью PVS-Studio я вновь обратил внимание на следующее. Я вижу мало подозрительных мест в коде, относящихся к ITK. Но при этом полно подозрительных мест и явных ошибок в файлах, которые расположены в папке "ThirdParty".

Удивительного в этом нет. В состав ITK входит достаточно много библиотек. Но ведь это печально. Некоторые из ошибок в библиотеках могут сказаться на работе ITK.

Я не буду призывать к каким-то действиям или давать рекомендации. Моя цель, чтобы люди обратили на моё наблюдение внимание и задумались. Чтобы мои слова запомнились, я покажу некоторые подозрительные места, на которые я обратил внимание.

Начнём, например, с библиотеки OpenJPEG

Неудачный case

typedef enum PROG_ORDER {
  PROG_UNKNOWN = -1,
  LRCP = 0,
  RLCP = 1,
  RPCL = 2,
  PCRL = 3,
  CPRL = 4
} OPJ_PROG_ORDER;

OPJ_INT32 pi_check_next_level(....)
{
  ....
  case 'P':
    switch(tcp->prg)
    {
      case LRCP||RLCP:
        if(tcp->prc_t == tcp->prcE){
          l=pi_check_next_level(i-1,cp,tileno,pino,prog);
  ....
}

Предупреждение PVS-Studio: V560 A part of conditional expression is always true: RLCP. pi.c 1708

Кто-то забыл, как правильно использовать оператор 'case'. Запись "case LRCP||RLCP:" эквивалентна записи "case 1:". Это явно не то, что хотел программист.

Правильным вариантом будет:

case LRCP:
case RLCP:

Именно так и написано в других местах. Впрочем, я бы ещё добавил комментарий. Например, такой:

case LRCP: // fall through
case RLCP:

Разыменование нулевого указателя

bool j2k_write_rgn(....)
{
  OPJ_BYTE * l_current_data = 00;
  OPJ_UINT32 l_nb_comp;
  OPJ_UINT32 l_rgn_size;
  opj_image_t *l_image = 00;
  opj_cp_t *l_cp = 00;
  opj_tcp_t *l_tcp = 00;
  opj_tccp_t *l_tccp = 00;
  OPJ_UINT32 l_comp_room;

  // preconditions
  assert(p_j2k != 00);
  assert(p_manager != 00);
  assert(p_stream != 00);

  l_cp = &(p_j2k->m_cp);
  l_tcp = &l_cp->tcps[p_tile_no];
  l_tccp = &l_tcp->tccps[p_comp_no];

  l_nb_comp = l_image->numcomps;
  ....
}

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'l_image' might take place. j2k.c 5205

Указатель 'l_image' инициализируется нулём, и больше нигде не изменяется. Таким образом, при вызове функции j2k_write_rgn() произойдёт разыменование нулевого указателя.

Переменная присваивается сама себе

OPJ_SIZE_T opj_stream_write_skip (....)
{
  ....
  if (!l_is_written)
  {
    p_stream->m_status |= opj_stream_e_error;
    p_stream->m_bytes_in_buffer = 0;
    p_stream->m_current_data = p_stream->m_current_data;
    return (OPJ_SIZE_T) -1;
  }
  ....
}

Предупреждение PVS-Studio: V570 The 'p_stream->m_current_data' variable is assigned to itself. cio.c 675

В коде что-то напутано. Переменной присваивается её же собственное значение.

Неправильная проверка

typedef struct opj_stepsize
{
  OPJ_UINT32 expn;
  OPJ_UINT32 mant;
};

bool j2k_read_SQcd_SQcc(
  opj_j2k_t *p_j2k,
  OPJ_UINT32 p_comp_no,
  OPJ_BYTE* p_header_data,
  OPJ_UINT32 * p_header_size,
  struct opj_event_mgr * p_manager
  )
{  
  ....
  OPJ_UINT32 l_band_no;
  ....
  l_tccp->stepsizes[l_band_no].expn =
    ((l_tccp->stepsizes[0].expn) - ((l_band_no - 1) / 3) > 0) ?
      (l_tccp->stepsizes[0].expn) - ((l_band_no - 1) / 3) : 0;
  ....
}

Предупреждение PVS-Studio: V555 The expression of the 'A - B > 0' kind will work as 'A != B'. itkopenjpeg j2k.c 3421

Сложно быстро заметить, что не так с этим кодом. Поэтому я составлю упрощенный синтетический пример:

unsigned A, B;
....
X = (A - B > 0) ? (A - B) : 0;

Как я понимаю, программист хотел следующее. Если переменная A больше, чем B, то посчитать разницу. Если нет, то выражение должно быть равно нулю.

Сравнение он написал неудачно. Так как выражение (A - B) имеет тип 'unsigned', оно всегда будет больше или равно 0. Например, если "A = 3, B = 5', то (A - B) равно 0xFFFFFFFE (4294967294).

Получается, что выражение можно упростить:

X = (A != B) ? (A - B) : 0;

Если (A == B), то при вычитании мы получим 0. Значит можно упростить выражение ещё больше:

X = A - B;

Явно что-то не так. Правильное сравнение можно записать так:

X = (A > B) ? (A - B) : 0;

GDCM

Но хватит про Jpeg. Нельзя превращать статью в справочник. Есть ведь и другие сторонние библиотеки. Например, Grassroots DICOM library (GDCM).

Неправильное условие цикла

bool Sorter::StableSort(std::vector<std::string> const & filenames)
{
  ....
  std::vector< SmartPointer<FileWithName> >::iterator
    it2 = filelist.begin();

  for( Directory::FilenamesType::const_iterator it =
         filenames.begin();
       it != filenames.end(), it2 != filelist.end();
       ++it, ++it2)
  {
  ....
}

Предупреждение PVS-Studio: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. gdcmsorter.cxx 82

Оператор запятая ',' в условии цикла не имеет смысла. Результатом работы оператора запятая ',' является его правая часть. Таким образом условие "it != filenames.end()" никак не учитывается.

Наверное, цикл должен быть таким:

for(Directory::FilenamesType::const_iterator it = ....;
    it != filenames.end() && it2 != filelist.end();
    ++it, ++it2)

Чуть ниже можно найти ещё один такой неправильный цикл (gdcmsorter.cxx 123).

Возможно разыменовывание нулевого указателя

bool PrivateTag::ReadFromCommaSeparatedString(const char *str)
{
  unsigned int group = 0, element = 0;
  std::string owner;
  owner.resize( strlen(str) );
  if( !str || sscanf(str, "%04x,%04x,%s", &group ,
                     &element, &owner[0] ) != 3 )
  {
    gdcmDebugMacro( "Problem reading Private Tag: " << str );
    return false;
  }
  ....
}

Предупреждение PVS-Studio: V595 The 'str' pointer was utilized before it was verified against nullptr. Check lines: 26, 27. gdcmprivatetag.cxx 26

Из условия видно, что указатель 'str' может быть равен nullptr. Тем не менее, без проверки выполняется разыменовывание этого указателя в строке:

owner.resize( strlen(str) );

Unspecified behavior

bool ImageCodec::DoOverlayCleanup(
  std::istream &is, std::ostream &os)
{
  ....
  // nmask : to propagate sign bit on negative values
  int16_t nmask = (int16_t)0x8000;
  nmask = nmask >>
          ( PF.GetBitsAllocated() - PF.GetBitsStored() - 1 );
  ....
}

Предупреждение PVS-Studio: V610 Unspecified behavior. Check the shift operator '>>. The left operand 'nmask' is negative. gdcmimagecodec.cxx 397

Сдвиг отрицательных значений с помощью оператора ">>" приводит к unspecified behavior. Для подобных библиотек полагаться на везение не допустимо.

Опасное чтение из файла

void LookupTable::Decode(....) const
{
  ....
  while( !is.eof() )
  {
    unsigned short idx;
    unsigned short rgb[3];
    is.read( (char*)(&idx), 2);
    if( is.eof() ) break;
    if( IncompleteLUT )
    {
      assert( idx < Internal->Length[RED] );
      assert( idx < Internal->Length[GREEN] );
      assert( idx < Internal->Length[BLUE] );
    }
    rgb[RED]   = rgb16[3*idx+RED];
    rgb[GREEN] = rgb16[3*idx+GREEN];
    rgb[BLUE]  = rgb16[3*idx+BLUE];
    os.write((char*)rgb, 3*2);
  }
  ....
}

Предупреждение PVS-Studio: V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression. gdcmMSFF gdcmlookuptable.cxx 280

Дело в том, что в этом месте программа может зависнуть. Если по какой-то причине произойдёт ошибка чтения файла, то проверка "is.eof()" не остановит цикл. В случае ошибки, из файла нельзя читать. Но файл ещё не кончился. Это разные вещи.

Необходима дополнительная проверка, которую можно сделать с помощью вызова функции is.fail().

Таких опасных чтений из файлов достаточно много. Я бы рекомендовал просмотреть все места, где вызывается функция eof(). Это встречается как в GDCM, так и в других библиотеках.

ITK

На библиотеках я остановлюсь. Думаю, я смог донести свои переживания.

Наверное, читателю интересно, нашлось ли что-то в самой библиотеке ITK. Да, кое что интересное я приметил.

Эффект последней строки в действии

Недавно я написал забавную статью "Эффект последней строки". Если не читали, то очень рекомендую.

Вот очередное проявление этого эффекта. В последней третьей строке, индекс должен быть '2', а не '1'.

int itkPointSetToSpatialObjectDemonsRegistrationTest(....)
{
  ....
  // Set its position
  EllipseType::TransformType::OffsetType offset;
  offset[0]=50;
  offset[1]=50;
  offset[1]=50;
  ....
}

Предупреждение PVS-Studio: V519 The 'offset[1]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 41, 42. itkpointsettospatialobjectdemonsregistrationtest.cxx 42

Опечатка

Ещё одна опечатка с индексом массива:

template< typename TCoordRepType >
void
VoronoiDiagram2D< TCoordRepType >::SetOrigin(PointType vorsize)
{
  m_VoronoiBoundaryOrigin[0] = vorsize[0];
  m_VoronoiBoundaryOrigin[0] = vorsize[1];
}

Предупреждение PVS-Studio: V519 The 'm_VoronoiBoundaryOrigin[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 74, 75. itkvoronoidiagram2d.hxx 75

Забыли индекс

void MultiThreader::MultipleMethodExecute()
{
  ....
  HANDLE process_id[ITK_MAX_THREADS];
  ....
  process_id[thread_loop] = (void *) _beginthreadex(0, 0, ....);

  if ( process_id == 0 )
  {
    itkExceptionMacro("Error in thread creation !!!");
  }
  ....
}

Предупреждение PVS-Studio: V600 Consider inspecting the condition. The 'process_id' pointer is always not equal to NULL. itkmultithreaderwinthreads.cxx 90

Проверка "if ( process_id == 0 )" не имеет смысла. Хотели проверить элемент массива и код должен быть таким:

if ( process_id[thread_loop] == 0 )

Одинаковые проверки

template< typename T >
void WriteCellDataBufferAsASCII(....)
{
  ....
  if( this->m_NumberOfCellPixelComponents == 3 )
  {
    ....
  }
  else if( this->m_NumberOfCellPixelComponents == 3 )
  {
    ....
  }
  ....
}

Предупреждения PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 948, 968. itkvtkpolydatameshio.h 948

Подозрительный конструктор

template<typename LayerType, typename TTargetVector>
QuickPropLearningRule <LayerType,TTargetVector>
::QuickPropLearningRule()
{
  m_Momentum = 0.9; //Default
  m_Max_Growth_Factor = 1.75;
  m_Decay = -0.0001;
  m_SplitEpsilon = 1;
  m_Epsilon = 0.55;
  m_Threshold = 0.0;
  m_SigmoidPrimeOffset = 0;
  m_SplitEpsilon = 0;
}

Предупреждения PVS-Studio: V519 The 'm_SplitEpsilon' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 35, 39. itkquickproplearningrule.hxx 39

Обратите внимание на инициализацию 'm_SplitEpsilon'. В начале этому члену класса присваивают значение 1, а потом 0. Подозрительно.

Неправильная очистка кэша

template <typename TInputImage, typename TOutputImage>
void
PatchBasedDenoisingImageFilter<TInputImage, TOutputImage>
::EmptyCaches()
{
  for (unsigned int threadId = 0;
       threadId < m_ThreadData.size(); ++threadId)
  {
    SizeValueType cacheSize =
      m_ThreadData[threadId].eigenValsCache.size();
    for (SizeValueType c = 0; c < cacheSize; ++c)
    {
      delete m_ThreadData[threadId].eigenValsCache[c];
      delete m_ThreadData[threadId].eigenVecsCache[c];
    }
    m_ThreadData[threadId].eigenValsCache.empty();
    m_ThreadData[threadId].eigenVecsCache.empty();
  }
}

Предупреждения PVS-Studio:

• V530 The return value of function 'empty' is required to be utilized. itkpatchbaseddenoisingimagefilter.hxx 85
• V530 The return value of function 'empty' is required to be utilized. itkpatchbaseddenoisingimagefilter.hxx 86

По невнимательности, вместо функции 'clear()', вызывается функция 'empty()'. В результате, кэш начнёт содержать мусор, и пользоваться им будет опасно. Эта ошибка, которую сложно найти и, которая может давать очень странные побочные эффекты.

Другие ошибки

Есть и другие ошибки, как в ITK, так и в сторонних библиотеках. Но я обещал себе уложиться в 12 страниц, набирая статью в Microsoft Word. Мне не нравится, что мои статьи становятся с каждым разом всё больше и больше. Приходится ограничивать себя. Причиной роста статей является то, что анализатор PVS-Studio учится находить всё больше ошибок.

То, что я описал не все подозрительные места - не страшно. Если признаться честно, я вообще смотрел отчёт поверхностно и многое пропустил. Не стоит рассматривать эту статью, как сборник предупреждений. Пусть эта статья лучше подтолкнёт кого-то к регулярному использованию статических анализаторов в своей работе. От этого будет намного больше пользы. Я не могу проверить все программы в мире.

Если авторы ITK проверят проект самостоятельно, это будет более полезно, чем делать правки, основываясь на моей статье. К сожалению, PVS-Studio в случае ITK выдаёт много ложных срабатываний. Много ложных срабатываний возникает из-за некоторых макросов. Результаты можно существенно улучшить, проведя минимальную настройку. В случае необходимости я готов дать подсказки.

Заключение

Уважаемые читатели, не забывайте, что разовые проверки статическим анализатором мало, что дают. Экономия времени достигается при регулярном использовании. Подробнее эта мысль раскрыта в заметке "Лев Толстой и статический анализ кода".

Желаю всем безглючных программ и безглючных библиотек.