Сейчас любое крупное приложение состоит из множества сторонних библиотек. Хочется поднять такую тему, как доверие к этим библиотекам. В книгах и статьях можно встретить очень много рассуждений о качестве кода, методах тестирования, методологиях разработки и так далее. Но я не помню, чтобы кто-то рассуждал о качестве кирпичей, из которых строятся приложения. Давайте немного поговорим об этом. Например, есть 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?.
Автор: Andrey2008
