# Можем ли мы доверять используемым библиотекам? Сейчас любое крупное приложение состоит из множества сторонних библиотек\. Хочется поднять такую тему, как доверие к этим библиотекам\. В книгах и статьях можно встретить очень много рассуждений о качестве кода, методах тестирования, методологиях разработки и так далее\. Но я не помню, чтобы кто\-то рассуждал о качестве кирпичей, из которых строятся приложения\. Давайте немного поговорим об этом\. Например, есть Medicine Insight Segmentation and Registration Toolkit \(ITK\)\. Мне кажется, он написан весьма качественно\. По крайней мере, я заметил в коде весьма мало ошибок\. Но я не могу сказать, что код используемых библиотек столь же качественен\. Тогда вопрос\. Насколько мы можем доверять таким системам? Есть повод для размышлений\. ![0271_ITK_ru/image1.png](https://import.viva64.com/docx/blog/0271_ITK_ru/image1.png) При разработке медицинских приложений все говорят о качестве, стандартах кодирования\. При написании кода требуют придерживаться таких стандартов, как [MISRA](https://ru.wikipedia.org/wiki/MISRA_C) и так далее\. Признаюсь, я плохо знаком с методологиями, используемыми при написании ответственных приложений\. Однако, у меня есть подозрение, что часто вопрос используемых сторонних библиотек обходится стороной\. Код приложения и код сторонних библиотек живут отдельными жизнями\. Такой вывод я делаю из своих субъективных наблюдений\. Нередко мне попадаются очень качественные приложения, где я не могу найти и пяток серьезных ошибок\. При этом, в составе таких приложений могут быть включены сторонние библиотеки отвратительного качества\. Предположим, врач поставит неправильный диагноз из\-за артефактов на изображении, которые возникают вследствие ошибки в программном обеспечении\. В такой случае, глубоко всё равно, была ошибка в самой программе или в библиотеке для работы с изображениями\. Это повод подумать\. В очередной раз на такие размышления меня навело рассматривание исходных кодов проекта [ITK](https://itk.org/): ![0271_ITK_ru/image2.png](https://import.viva64.com/docx/blog/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](https://pvs-studio.ru/ru/pvs-studio/) я вновь обратил внимание на следующее\. Я вижу мало подозрительных мест в коде, относящихся к ITK\. Но при этом полно подозрительных мест и явных ошибок в файлах, которые расположены в папке "ThirdParty"\. Удивительного в этом нет\. В состав ITK входит достаточно много библиотек\. Но ведь это печально\. Некоторые из ошибок в библиотеках могут сказаться на работе ITK\. Я не буду призывать к каким\-то действиям или давать рекомендации\. Моя цель, чтобы люди обратили на моё наблюдение внимание и задумались\. Чтобы мои слова запомнились, я покажу некоторые подозрительные места, на которые я обратил внимание\. ## Начнём, например, с библиотеки OpenJPEG **Неудачный case** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: RLCP\. pi\.c 1708 Кто\-то забыл, как правильно использовать оператор 'case'\. Запись "case LRCP\|\|RLCP:" эквивалентна записи "case 1:"\. Это явно не то, что хотел программист\. Правильным вариантом будет: ```cpp case LRCP: case RLCP: ``` Именно так и написано в других местах\. Впрочем, я бы ещё добавил комментарий\. Например, такой: ```cpp case LRCP: // fall through case RLCP: ``` **Разыменование нулевого указателя** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v522/) Dereferencing of the null pointer 'l\_image' might take place\. j2k\.c 5205 Указатель 'l\_image' инициализируется нулём, и больше нигде не изменяется\. Таким образом, при вызове функции j2k\_write\_rgn\(\) произойдёт разыменование нулевого указателя\. **Переменная присваивается сама себе** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v570/) The 'p\_stream\-\>m\_current\_data' variable is assigned to itself\. cio\.c 675 В коде что\-то напутано\. Переменной присваивается её же собственное значение\. **Неправильная проверка** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v555/) The expression of the 'A \- B \> 0' kind will work as 'A \!\= B'\. itkopenjpeg j2k\.c 3421 Сложно быстро заметить, что не так с этим кодом\. Поэтому я составлю упрощенный синтетический пример: ```cpp unsigned A, B; .... X = (A - B > 0) ? (A - B) : 0; ``` Как я понимаю, программист хотел следующее\. Если переменная A больше, чем B, то посчитать разницу\. Если нет, то выражение должно быть равно нулю\. Сравнение он написал неудачно\. Так как выражение \(A \- B\) имеет тип 'unsigned', оно всегда будет больше или равно 0\. Например, если "A \= 3, B \= 5', то \(A \- B\) равно 0xFFFFFFFE \(4294967294\)\. Получается, что выражение можно упростить: ```cpp X = (A != B) ? (A - B) : 0; ``` Если \(A \=\= B\), то при вычитании мы получим 0\. Значит можно упростить выражение ещё больше: ```cpp X = A - B; ``` Явно что\-то не так\. Правильное сравнение можно записать так: ```cpp X = (A > B) ? (A - B) : 0; ``` ## GDCM Но хватит про Jpeg\. Нельзя превращать статью в справочник\. Есть ведь и другие сторонние библиотеки\. Например, Grassroots DICOM library \([GDCM](http://gdcm.sourceforge.net/wiki/index.php/Main_Page)\)\. **Неправильное условие цикла** ```cpp bool Sorter::StableSort(std::vector const & filenames) { .... std::vector< SmartPointer >::iterator it2 = filelist.begin(); for( Directory::FilenamesType::const_iterator it = filenames.begin(); it != filenames.end(), it2 != filelist.end(); ++it, ++it2) { .... } ``` Предупреждение PVS\-Studio: [V521](https://pvs-studio.ru/ru/docs/warnings/v521/) Such expressions using the ',' operator are dangerous\. Make sure the expression is correct\. gdcmsorter\.cxx 82 Оператор запятая ',' в условии цикла не имеет смысла\. Результатом работы оператора запятая ',' является его правая часть\. Таким образом условие "it \!\= filenames\.end\(\)" никак не учитывается\. Наверное, цикл должен быть таким: ```cpp for(Directory::FilenamesType::const_iterator it = ....; it != filenames.end() && it2 != filelist.end(); ++it, ++it2) ``` Чуть ниже можно найти ещё один такой неправильный цикл \(gdcmsorter\.cxx 123\)\. **Возможно разыменовывание нулевого указателя** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'str' pointer was utilized before it was verified against nullptr\. Check lines: 26, 27\. gdcmprivatetag\.cxx 26 Из условия видно, что указатель 'str' может быть равен nullptr\. Тем не менее, без проверки выполняется разыменовывание этого указателя в строке: ```cpp owner.resize( strlen(str) ); ``` **Unspecified behavior** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v610/) Unspecified behavior\. Check the shift operator '\>\>\. The left operand 'nmask' is negative\. gdcmimagecodec\.cxx 397 Сдвиг отрицательных значений с помощью оператора "\>\>" приводит к [unspecified behavior](https://en.wikipedia.org/wiki/Unspecified_behavior)\. Для подобных библиотек полагаться на везение не допустимо\. **Опасное чтение из файла** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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\. Да, кое что интересное я приметил\. **Эффект последней строки в действии** Недавно я написал забавную статью "[Эффект последней строки](https://pvs-studio.ru/ru/blog/posts/cpp/0260/)"\. Если не читали, то очень рекомендую\. Вот очередное проявление этого эффекта\. В последней третьей строке, индекс должен быть '2', а не '1'\. ```cpp int itkPointSetToSpatialObjectDemonsRegistrationTest(....) { .... // Set its position EllipseType::TransformType::OffsetType offset; offset[0]=50; offset[1]=50; offset[1]=50; .... } ``` Предупреждение PVS\-Studio: [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'offset\[1\]' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 41, 42\. itkpointsettospatialobjectdemonsregistrationtest\.cxx 42 **Опечатка** Ещё одна опечатка с индексом массива: ```cpp template< typename TCoordRepType > void VoronoiDiagram2D< TCoordRepType >::SetOrigin(PointType vorsize) { m_VoronoiBoundaryOrigin[0] = vorsize[0]; m_VoronoiBoundaryOrigin[0] = vorsize[1]; } ``` Предупреждение PVS\-Studio: [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'm\_VoronoiBoundaryOrigin\[0\]' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 74, 75\. itkvoronoidiagram2d\.hxx 75 **Забыли индекс** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v600/) Consider inspecting the condition\. The 'process\_id' pointer is always not equal to NULL\. itkmultithreaderwinthreads\.cxx 90 Проверка "if \( process\_id \=\= 0 \)" не имеет смысла\. Хотели проверить элемент массива и код должен быть таким: ```cpp if ( process_id[thread_loop] == 0 ) ``` **Одинаковые проверки** ```cpp template< typename T > void WriteCellDataBufferAsASCII(....) { .... if( this->m_NumberOfCellPixelComponents == 3 ) { .... } else if( this->m_NumberOfCellPixelComponents == 3 ) { .... } .... } ``` Предупреждения PVS\-Studio: [V517](https://pvs-studio.ru/ru/docs/warnings/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 **Подозрительный конструктор** ```cpp template QuickPropLearningRule ::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](https://pvs-studio.ru/ru/docs/warnings/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\. Подозрительно\. **Неправильная очистка кэша** ```cpp template void PatchBasedDenoisingImageFilter ::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](https://pvs-studio.ru/ru/docs/warnings/v530/) The return value of function 'empty' is required to be utilized\. itkpatchbaseddenoisingimagefilter\.hxx 85 * [V530](https://pvs-studio.ru/ru/docs/warnings/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 выдаёт много ложных срабатываний\. Много ложных срабатываний возникает из\-за некоторых макросов\. Результаты можно существенно улучшить, проведя минимальную настройку\. В случае необходимости я готов дать подсказки\. ## Заключение Уважаемые читатели, не забывайте, что разовые проверки статическим анализатором мало, что дают\. Экономия времени достигается при регулярном использовании\. Подробнее эта мысль раскрыта в заметке "[Лев Толстой и статический анализ кода](https://pvs-studio.ru/ru/blog/posts/0105/)"\. Желаю всем безглючных программ и безглючных библиотек\.