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

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

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

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

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 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 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)0×8000; 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 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 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 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 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?.

© Habrahabr.ru