Я временами бываю озадачен, рассматривая ошибки в очередном программном проекте. Многие из этих ошибок живут в проектах годами. Смотришь на сотню ляпов в коде и удивляешься, как программа вообще работает. И ведь как-то работает. Ей даже пользуются. Причем, я говорю не о коде, рисующем покемона в игре. А, например, о математических библиотеках. Да, вы верно догадались. В этой статье пойдет речь о проверке кода математической библиотеки Scilab.
Сегодня мы будем рассматривать подозрительные фрагменты кода в математическом пакете Scilab. Анализа выполнен с помощью инструмента PVS-Studio.
Scilab - пакет прикладных математических программ, предоставляющий мощное открытое окружение для инженерных (технических) и научных расчётов [Wikipedia].
Официальный сайт: http://www.scilab.org/
В системе доступно множество инструментов:
Приготовьтесь. Статья будет длинная. Я ведь не виноват, что тут так много разных пакостей. И хочется показать разные классы ошибок.
Конечно, найденные недостатки не имеют отношения к математике. Возможно, все алгоритмы в библиотеке правильные и эффективные. Однако, раз для написания выбран язык Си++, надо не забывать, что помимо ошибок в алгоритмах, бывают ещё и опечатки, разыменование нулевых указателей и прочие ляпы. Собственно, пользователю не важно, встретится ему логическая ошибка в численном алгоритме или он станет жертвой неинициализированной переменной.
Да, статический анализ находит только некоторые виды ошибок. Но раз их легко обнаружить, зачем себе отказывать в таком удовольствии. Лучше поправить ещё 10% ошибок, чем вовсе ничего не поправить.
Итак, давайте посмотрим, что рассказал мне анализатор PVS-Studio о проекте Scilab.
int sci_champ_G(....)
{
....
char * strf = NULL ;
....
if ( isDefStrf( strf ) )
{
char strfl[4];
strcpy(strfl,DEFSTRFN);
strf = strfl;
if ( !isDefRect( rect ) )
{
strf[1]='5';
}
}
(*func)(stk(l1), stk(l2), stk(l3), stk(l4),
&m3, &n3, strf, rect, arfact, 4L);
....
}Сообщение PVS-Studio: V507 Pointer to local array 'strfl' is stored outside the scope of this array. Such a pointer will become invalid. sci_champ.c 103
Ссылка на временный массив 'strfl' сохраняется в переменной 'strf'. При выходе из блока "if () { ... }" этот массив перестаёт существовать. Однако, в программе работают с указателем 'strf'.
Поведение такой программы непредсказуемо. Нельзя работать с массивом, которого уже нет. Конечно, программа может вполне корректно отработать. Однако, это везение. Память, где располагался массив, в любой момент может быть занята под хранение других массивов или переменных.
Аналогичные проблемы:
int C2F(pmatj)
(char *fname, int *lw, int *j, unsigned long fname_len)
{
....
ix1 = il2 + 4;
m2 = Max(m, 1);
ix1 = il + 9 + m * n;
....
}Предупреждение PVS-Studio: V519 The 'ix1' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2387, 2389. stack1.c 2389
С переменной 'ix1' что-то неладно. Мне кажется, здесь какая-то опечатка.
Интересный фрагмент кода. Нужно получить некоторые значения, а затем проверить их. Но получилось наоборот.
int sci_Playsound (char *fname,unsigned long fname_len)
{
....
int m1 = 0, n1 = 0;
....
if ( (m1 != n1) && (n1 != 1) )
{
Scierror(999,_("%s: Wrong size for input argument #%d: ")
_("A string expected.\n"),fname,1);
return 0;
}
sciErr = getMatrixOfWideString(pvApiCtx, piAddressVarOne,
&m1,&n1,&lenStVarOne, NULL);
....
}Предупреждения PVS-Studio: V560 A part of conditional expression is always false: (m1 != n1). sci_playsound.c 66; V560 A part of conditional expression is always true: (n1 != 1). sci_playsound.c 66
Переменные m1 и n1 должны получить значения при вызове функции getMatrixOfWideString(). Потом эти переменные должны быть проверены. Вот только получилось так, что проверка осуществляется до вызова функции getMatrixOfWideString().
В момент проверки переменные m1 и n1 равны 0. Условие "if ( (m1 != n1) && (n1 != 1) )" не выполняется. В результате, поверка никак не влияет на работу программы.
Итого. Не осуществляется проверка переменных m1 и n1.
void CreCommon(f,var)
FILE *f;
VARPTR var;
{
....
if ( strncmp(var->fexternal, "cintf", 4)==0 )
....
}Предупреждение PVS-Studio: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. crerhs.c 119
Используется магическое число 4. И это число неправильное. В строке "cintf" пять символов, а не четыре. Не используйте такие магические числа.
Я бы сделал специальный макрос для вычисления длины строковых литералов и использовал бы его так:
if ( strncmp(var->fexternal, "cintf", litlen("cintf"))==0 )Как изготовить макрос 'litlen', обсуждать не будем. Есть масса способов на любой вкус. Главное, избавиться от числа.
Другие неправильные размеры строк:
int C2F(run)(void)
{
....
static int *Lpt = C2F(iop).lpt - 1;
....
Lpt[1] = Lin[1 + k];
Lpt[2] = Lin[2 + k];
Lpt[3] = Lin[3 + k];
Lpt[4] = Lin[4 + k];
Lct[4] = Lin[6 + k ];
Lpt[6] = k;
....
}Предупреждение PVS-Studio: V525 The code containing the collection of similar blocks. Check items '1', '2', '3', '4', '4' in lines 1005, 1006, 1007, 1008, 1009. run.c 1005
Опечатка в последовательности чисел. В результате один элемент массива останется неинициализированным. Можно массу интересных математических результатов получить.
int write_xml_states(
int nvar, const char * xmlfile, char **ids, double *x)
{
....
FILE *fd = NULL;
....
wcfopen(fd, (char*)xmlfile, "wb");
if (fd < 0)
{
sciprint(_("Error: cannot write to '%s' \n"), xmlfile);
....
}Предупреждение PVS-Studio: V503 This is a nonsensical comparison: pointer < 0. scicos.c 5826
Я почти уверен, что когда-то в этом коде для открытия файла использовалась функция open. Затем, код переписали и стали использовать функцию на _wfopen. Её вызов спрятан в макрос 'wcfopen'.
А вот проверку, что файл успешно открыт, поправить забыли. Функция open() возвращает в случае ошибки значение -1. Проверять же, что указатель меньше нуля, не имеет никакого практического смысла.
Ещё одно место, где прослеживается история.
void taucs_ccs_genmmd(taucs_ccs_matrix* m,
int** perm, int** invperm)
{
int n, maxint, delta, nofsub;
....
maxint = 32000;
assert(sizeof(int) == 4);
maxint = 2147483647; /* 2**31-1, for 32-bit only! */
....
}Предупреждение PVS-Studio: V519 The 'maxint' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 154, 157. taucs_scilab.c 157
Ошибки здесь нет, но код забавен.
Видно, что давным-давно, было написано "maxint = 32000;". Затем ниже появилось:
assert(sizeof(int) == 4);
maxint = 2147483647; /* 2**31-1, for 32-bit only! */char *getCommonPart(char **dictionary, int sizeDictionary)
{
....
char *currentstr = dictionary[0];
qsort(dictionary, sizeof dictionary / sizeof dictionary[0],
sizeof dictionary[0], cmp);
....
}Предупреждение PVS-Studio: V514 Dividing sizeof a pointer 'sizeof dictionary' by another value. There is a probability of logical error presence. getcommonpart.c 76
Второй аргумент функции qsort() - это количество элементов в массиве. Из-за ошибки, количество элементов всегда равно одному.
Рассмотрим выражение "sizeof dictionary / sizeof dictionary[0]". Здесь размер указателя делится на размер указателя. Результат равен единице.
Наверное, правильный код должен был быть таким:
qsort(dictionary, sizeDictionary, sizeof dictionary[0], cmp);Аналогичная ошибка здесь: getfilesdictionary.c 105
void GetenvB(char *name, char *env, int len)
{
int ierr = 0, one = 1;
C2F(getenvc)(&ierr,name,env,&len,&one);
if (ierr == 0)
{
char *last = &env[len-1];
while ( *last == ' ' ) { last = '\0' ; }
last--;
}
....
}V527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *last = '\0'. getenvb.c 24
Эта строка ужасна. Или прекрасна, если мы говорим об интересных ошибках.
while ( *last == ' ' ) { last = '\0' ; }Если первый символ в строке пробел, то указатель станет равен нулю. Далее возникнет обращение по нулевому указателю.
Мне кажется, этот код должен был заменить все пробелы на '\0'. Тогда код должен быть таким:
while ( *last == ' ' ) { *last++ = '\0' ; }Забавно, что есть ещё одно место в коде, где тоже хотят менять пробелы на нули. И его тоже не удалось написать правильно.
static int msg_101(int *n, int *ierr)
{
....
for (i=0;i<(int)strlen(line);i++)
{
if (line[i]==' ') line[i]='\0';
break;
}
....
}Предупреждение PVS-Studio: V612 An unconditional 'break' within a loop. msgs.c 1293
Всё бы хорошо, если бы не оператор 'break'. Будет заменён только один пробел. Впрочем, если убрать 'break' это не поможет. Функция strlen() вернёт ноль, и цикл всё равно остановится.
Аналогичные "одноразовые" циклы:
char **splitLineCSV(....)
{
....
if (retstr[curr_str] == NULL)
{
*toks = 0;
FREE(substitutedstring);
substitutedstring = NULL;
freeArrayOfString(retstr, strlen(substitutedstring));
return NULL;
}
....
}Предупреждение PVS-Studio: V575 The null pointer is passed into 'strlen' function. Inspect the first argument. splitline.c 107
Странный код. В начале явно обнулили указатель 'substitutedstring'. Затем, отдали его на растерзание в функцию strlen().
Скорее всего, вызов функции freeArrayOfString() должен быть расположен выше, чем вызов FREE().
Это была разминка. Теперь рассмотрим более сложный случай.
inline static void create(void * pvApiCtx, const int position,
const int rows, const int cols, long long * ptr)
{
int * dataPtr = 0;
alloc(pvApiCtx, position, rows, cols, dataPtr);
for (int i = 0; i < rows * cols; i++)
{
dataPtr[i] = static_cast<int>(ptr[i]);
}
}V522 Dereferencing of the null pointer 'dataPtr' might take place. scilababstractmemoryallocator.hxx 222
В этой функции хотят выделить память, используя функцию alloc(). Может показаться, что функция возвращает значение по ссылке. Последним аргументом является указатель 'dataPtr'. Кажется, что в него и будет записан указатель на выделенный буфер памяти.
Но это не так. Указатель останется равен нулю. Давайте посмотрим, как объявлена функция alloc():
inline static int *alloc(
void * pvApiCtx, const int position, const int rows,
const int cols, int * ptr)Видите, последний аргумент не является ссылкой. Кстати, вообще не понятно, зачем он нужен. Заглянем внутрь функции alloc():
inline static int *alloc(
void * pvApiCtx, const int position, const int rows,
const int cols, int * ptr)
{
int * _ptr = 0;
SciErr err = allocMatrixOfInteger32(
pvApiCtx, position, rows, cols, &_ptr);
checkError(err);
return _ptr;
}Последний аргумент 'ptr' вообще не используется.
В любом случае, код выделения памяти неверен. Код должен быть таким:
inline static void create(void * pvApiCtx, const int position,
const int rows, const int cols, long long * ptr)
{
int *dataPtr = alloc(pvApiCtx, position, rows, cols, 0);
for (int i = 0; i < rows * cols; i++)
{
dataPtr[i] = static_cast<int>(ptr[i]);
}
}Аналогичные ситуации:
Анализатор PVS-Studio часто находит опечатки в обработчиках ошибок. Этот код редко выполняется, и ошибки подолгу остаются незамеченными. Я думаю, из-за таких ошибок мы часто не можем понять, что же с программой не так. Выданное программой диагностическое сообщение не соответствует действительности.
Пример неправильного формирования сообщения об ошибке:
static SciErr fillCommonSparseMatrixInList(....)
{
....
addErrorMessage(&sciErr, API_ERROR_FILL_SPARSE_IN_LIST,
_("%s: Unable to create list item #%d in Scilab memory"),
_iComplex ? "createComplexSparseMatrixInList" :
"createComplexSparseMatrixInList",
_iItemPos + 1);
....
}Сообщение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: "createComplexSparseMatrixInList". api_list.cpp 2398
В независимости от значения переменной '_iComplex', всегда будет распечатано "createComplexSparseMatrixInList".
Аналогично:
Теперь рассмотрим обработчик ошибки, который никогда не получит управление:
#define __GO_FIGURE__ 9
#define __GO_UIMENU__ 21
int sci_uimenu(char *fname, unsigned long fname_len)
{
....
if (iParentType == __GO_FIGURE__ &&
iParentType == __GO_UIMENU__)
{
Scierror(999, _("%s: Wrong type for input argument #%d: ")
_("A '%s' or '%s' handle expected.\n"),
fname, 1, "Figure", "Uimenu");
return FALSE;
}
....
}Предупреждение PVS-Studio: V547 Expression 'iParentType == 9 && iParentType == 21' is always false. Probably the '||' operator should be used here. sci_uimenu.c 99
Условие (iParentType == __GO_FIGURE__ && iParentType == __GO_UIMENU__) никогда не выполняется. Переменная не может быть одновременно равна числу 9 и числу 21. Я думаю, здесь хотели написать так:
if (iParentType != __GO_FIGURE__ &&
iParentType != __GO_UIMENU__)Ещё один, особенно сладкий пример.
int set_view_property(....)
{
BOOL status = FALSE;
....
status = setGraphicObjectProperty(
pobjUID, __GO_VIEW__, &viewType, jni_int, 1);
if (status = TRUE)
{
return SET_PROPERTY_SUCCEED;
}
else
{
Scierror(999, _("'%s' property does not exist ")
_("for this handle.\n"), "view");
return SET_PROPERTY_ERROR ;
}
....
}Предупреждение PVS-Studio: V559 Suspicious assignment inside the condition expression of 'if' operator: status = 1. set_view_property.c 61
Ошибка здесь: "if (status = TRUE)". Вместо сравнения, происходит присваивание.
Функция, которую можно явно сократить. Видимо, она написана с помощью Copy-Paste, и в скопированном коде что-то забыли поправить.
static int uf_union (int* uf, int s, int t) {
if (uf_find(uf,s) < uf_find(uf,t))
{
uf[uf_find(uf,s)] = uf_find(uf,t);
return (uf_find(uf,t));
}
else
{
uf[uf_find(uf,s)] = uf_find(uf,t);
return (uf_find(uf,t));
}
}Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. taucs_scilab.c 700
Независимо от условия выполняются идентичные действия.
Теперь другая ситуация. Здесь совпадают условия:
int sci_xset( char *fname, unsigned long fname_len )
{
....
else if ( strcmp(cstk(l1), "mark size") == 0)
....
else if ( strcmp(cstk(l1), "mark") == 0)
....
else if ( strcmp(cstk(l1), "mark") == 0)
....
else if ( strcmp(cstk(l1), "colormap") == 0)
....
}Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 175, 398. sci_xset.c 175
Есть ещё несколько неправильных условий:
Пожалуй, я выделил самый частый ляп в программах на языке Си/Си++. В начале указатель разыменовывается, а уже затем проверяется на равенство нулю. Это не всегда приводит к ошибке. Но безобразие, оно и есть безобразие.
static void appendData(....)
{
....
sco_data *sco = (sco_data *) * (block->work);
int maxNumberOfPoints = sco->internal.maxNumberOfPoints;
int numberOfPoints = sco->internal.numberOfPoints;
if (sco != NULL && numberOfPoints >= maxNumberOfPoints)
....
}Предупреждение PVS-Studio: V595 The 'sco' pointer was utilized before it was verified against nullptr. Check lines: 305, 311. canimxy3d.c 305
В начале, осуществляли доступ к членам, используя указатель 'sco':
int maxNumberOfPoints = sco->internal.maxNumberOfPoints;
int numberOfPoints = sco->internal.numberOfPoints;А потом вдруг вспомнили, что этот указатель надо проверить:
if (sco != NULL .....Анализатор выдал ещё 61 одно предупреждение V595. Перечислять их в статье не вижу смысла. Привожу их отдельным списком: scilab-v595.txt.
Ещё одна распространённая ситуация - использование неправильных спецификаторов формата (format specifiers) при работе с функцией sprintf() и аналогичных ей. Почти всё, что нашлось, не интересно. Печатаем беззнаковые значения, как знаковые. Поэтому привожу все эти предупреждения тоже списком: scilab-v576.txt.
Из интересного можно отметить только вот это:
#define FORMAT_SESSION "%s%s%s"
char *getCommentDateSession(BOOL longFormat)
{
....
sprintf(line, FORMAT_SESSION, SESSION_PRAGMA_BEGIN,
STRING_BEGIN_SESSION, time_str, SESSION_PRAGMA_END);
....
}Предупреждение PVS-Studio: V576 Incorrect format. A different number of actual arguments is expected while calling 'sprintf' function. Expected: 5. Present: 6. getcommentdatesession.c 68
Не будет распечатана строка SESSION_PRAGMA_END.
short ezxml_internal_dtd(ezxml_root_t root, char *s, size_t len)
{
....
while (*(n = ++s + strspn(s, EZXML_WS)) && *n != '>') {
....
}Предупреждение PVS-Studio: V567 Undefined behavior. The 's' variable is modified while being used twice between sequence points. ezxml.c 385
Неизвестно, будет вычислено в начале выражение "++s' или выражение "strspn(s, EZXML_WS)". Соответственно, результат может отличаться на разных компиляторах, платформах и так далее.
Другой, более интересный случай. Здесь неопределённое поведение возникает из-за опечатки.
static char **replaceStrings(....)
{
....
int i = 0;
....
for (i = 0; i < nr; i = i++)
....
}V567 Undefined behavior. The 'i' variable is modified while being used twice between sequence points. csvread.c 620
Беда здесь: i = i++.
По всей видимости, хотел написать так:
for (i = 0; i < nr; i++)char *PLD_strtok(....)
{
....
if ((st->start)&&(st->start != '\0'))
....
}Предупреждение PVS-Studio: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *st->start != '\0'. pldstr.c 303
Хотели проверить, что строка не пустая. Но на самом деле указатель два раза сравнивается с NULL. Правильный код:
if ((st->start)&&(st->start[0] != '\0'))Аналогичный ляп:
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: ** category == '\0'. sci_xcospalload.cpp 57
Следующий код по всей видимости недописан:
int sci_displaytree(char *fname, unsigned long fname_len)
{
....
string szCurLevel = "";
....
//Add node level
if (szCurLevel != "")
{
szCurLevel + ".";
}
....
}Предупреждение PVS-Studio: V655 The strings was concatenated but are not utilized. Consider inspecting the 'szCurLevel + "."' expression. sci_displaytree.cpp 80
static int sci_toprint_two_rhs(void* _pvCtx,
const char *fname)
{
....
sprintf(lines, "%s%s\n", lines, pStVarOne[i]);
....
}Предупреждение PVS-Studio: V541 It is dangerous to print the string 'lines' into itself. sci_toprint.cpp 314
Функция sprintf() сохраняет результат своей работы в буфер 'lines'. При этом, этот же буфер является одной из входных строк. Так делать не хорошо. Код вполне может работать. Но это опасно. При смене компилятора можно получить неожиданный и неприятный результат.
Аналогичная ситуация: sci_coserror.c 94
Пример кода, который работает, хотя и не верен:
typedef struct JavaVMOption {
char *optionString;
void *extraInfo;
} JavaVMOption;
JavaVMOption *options;
BOOL startJVM(char *SCI_PATH)
{
....
fprintf(stderr, "%d: %s\n", j, vm_args.options[j]);
....
}Предупреждение PVS-Studio: V510 The 'fprintf' function is not expected to receive class-type variable as fourth actual argument. jvm.c 247
Здесь хотели распечатать строку, на которую ссылается указатель 'optionString'. Правильный код должен был быть таким:
fprintf(stderr, "%d: %s\n", j, vm_args.options[j].optionString);Но на самом деле, в качестве аргумента функция fprintf() примет объект типа JavaVMOption. Код работает благодаря чудесному стечению обстоятельств.
Во-первых, член 'optionString' расположен в начале структуры. Поэтому именно его возьмет функция fprintf() и обработает его как указатель на строку.
Во-вторых, после этого функция ничего не распечатывает. Следовательно, не будет распечатан мусор (содержимое переменной 'extraInfo', которая тоже попадёт в стек).
Аллилуйя!
static void reinitdoit(double *told)
{
int keve = 0, kiwa = 0;
....
kiwa = 0;
....
for (i = 0; i < kiwa; i++)
....
}V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. scicos.c 4432
Здесь что-то не так. Переменная 'kiwa' всегда равна нулю. Цикл не выполняется. Возможно, код недописан.
Если честно, я уже устал просматривать отчёт и писать эту статью. Поэтому я решил остановиться. Пожалуй, можно было упомянуть про ещё пару подозрительных мест. Но я посчитал их несущественными, и лень победила. Плюс наверняка я что-то пропустил, так как не знаком с проектом. Поэтому рекомендую авторам самостоятельно проверить проект, используя анализатор PVS-Studio.
Примечание. Тем, кто решил проверить один раз проверить проект и не покупать анализатор, хочу напомнить, что это совершенно бессмысленное действие. Вся суть статического анализа в регулярных проверках, а не в разовых запусках. Вы допускаете опечатку и анализатор сразу её обнаруживает. Сокращается время на тестирование, отладку и работу с ошибками, появляющимися в bug-трекере. См. также статью: "Лев Толстой и статический анализ кода".
Обязательно кто-то спросит, какая версия Scilab проверялась. К сожалению, не самая свежая. Где-то полтора месяца я проверил этот проект, выписал подозрительные фрагменты кода. И... И забыл про этот файл. Было много работы, связанной со сравнением анализаторов. Сейчас я набрёл на этот файл, и долго вспоминал, что это такое было. Я проверяю так много проектов, что у меня в голове уже всё перепуталось, и я даже не помню, смотрел я какой-то проект или нет.
Впрочем, ничего страшного. Сейчас я напишу эту статью. Её увидят авторы Scilab и сами проверят проект. Цель моих статей показать возможности методологии статического анализа, а не найти ошибки в самой последней версии проекта.
Используйте статический анализ на регулярной основе. Вы сократите время на устранение глупых ошибок и сможете потратить больше времени на что-то полезное.
Для средних и больших проектов, где требуются ночные проверки, доработка анализатора, интеграция с MSBuild, поддержка Visual Studio 2005/2008 и так далее, мы предлагаем инструмент PVS-Studio.