to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Можем ли мы доверять используемым библи…

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

11 Авг 2014

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

0271_ITK_ru/image1.png

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

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

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

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

0271_ITK_ru/image2.png

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 выдаёт много ложных срабатываний. Много ложных срабатываний возникает из-за некоторых макросов. Результаты можно существенно улучшить, проведя минимальную настройку. В случае необходимости я готов дать подсказки.

Заключение

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

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

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Ложные представления программистов о неопределённом поведении

Дата: 17 Янв 2023

Автор: Гость

Неопределённое поведение (UB) – непростая концепция в языках программирования и компиляторах. Я слышал много заблуждений в том, что гарантирует компилятор при наличии UB. Это печально, но неудивитель…
Топ-10 ошибок в C++ проектах за 2022 год

Дата: 29 Дек 2022

Автор: Владислав Столяров

Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл PVS-Studio в 2022 году.
PVS-Studio и RPCS3: лучшие предупреждения в один клик

Дата: 12 Дек 2022

Автор: Александр Куренев

Best Warnings — режим анализатора, оставляющий в окне вывода 10 лучших предупреждений. Мы предлагаем вам ознакомиться с обновлённым режимом Best Warnings на примере проверки проекта RPCS3.
Holy C++

Дата: 23 Ноя 2022

Автор: Гость

В этой статье постараюсь затронуть все вещи, которые можно без зазрения совести выкинуть из С++, не потеряв ничего (кроме боли), уменьшить стандарт, нагрузку на создателей компиляторов, студентов, из…

Комментарии (0)

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо