![Can We Trust the Libraries We Use?](http://www.viva64.com/media/images/content/b/0270_ITK_ru/image1.png)
Сейчас любое крупное приложение состоит из множества сторонних библиотек. Хочется поднять такую тему, как доверие к этим библиотекам. В книгах и статьях можно встретить очень много рассуждений о качестве кода, методах тестирования, методологиях разработки и так далее. Но я не помню, чтобы кто-то рассуждал о качестве кирпичей, из которых строятся приложения. Давайте немного поговорим об этом. Например, есть 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
<b>Неудачный case</b> 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 выдаёт много ложных срабатываний. Много ложных срабатываний возникает из-за некоторых макросов. Результаты можно существенно улучшить, проведя минимальную настройку. В случае необходимости я готов дать подсказки.
Заключение
Уважаемые читатели, не забывайте, что разовые проверки статическим анализатором мало, что дают. Экономия времени достигается при регулярном использовании. Подробнее эта мысль раскрыта в заметке „Лев Толстой и статический анализ кода“.
Желаю всем безглючных программ и безглючных библиотек.
Эта статья на английском
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Can We Trust the Libraries We Use?.
ссылка на оригинал статьи http://habrahabr.ru/company/pvs-studio/blog/232873/
Добавить комментарий