Можем ли мы доверять используемым библиотекам?
Сейчас любое крупное приложение состоит из множества сторонних библиотек. Хочется поднять такую тему, как доверие к этим библиотекам. В книгах и статьях можно встретить очень много рассуждений о качестве кода, методах тестирования, методологиях разработки и так далее. Но я не помню, чтобы кто-то рассуждал о качестве кирпичей, из которых строятся приложения. Давайте немного поговорим об этом. Например, есть 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
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
Неправильная очистка кэша
template
Если авторы ITK проверят проект самостоятельно, это будет более полезно, чем делать правки, основываясь на моей статье. К сожалению, PVS-Studio в случае ITK выдаёт много ложных срабатываний. Много ложных срабатываний возникает из-за некоторых макросов. Результаты можно существенно улучшить, проведя минимальную настройку. В случае необходимости я готов дать подсказки.
Заключение Уважаемые читатели, не забывайте, что разовые проверки статическим анализатором мало, что дают. Экономия времени достигается при регулярном использовании. Подробнее эта мысль раскрыта в заметке «Лев Толстой и статический анализ кода».Желаю всем безглючных программ и безглючных библиотек.
Эта статья на английском Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Can We Trust the Libraries We Use?.
