Статический анализ помогает разработчикам отлавливать ошибки на раннем этапе и повышает качество и надёжность кода. В этой статье приводится разбор некоторых потенциальных уязвимостей и ошибок, найденных в исходном коде библиотеки STP с помощью PVS-Studio.
Описание проекта STP весьма лаконичное и наполнено сложными терминами. Поэтому понять, для чего он нужен, задача нетривиальная. Можно предположить, что проект предназначен для проверки ограничений массивов или битовых векторов. Но что это значит на практике, я не знаю. Да и неважно, ведь наша задача – проверить его на предмет программных ошибок, а не математических :). Библиотека распространяется с открытым исходным кодом под лицензией MIT. Она написана на языке C++. Ее применяют в качестве компонента в различных инструментах поиска ошибок в исходном коде. Проект использует сборочную систему CMake, так что особых проблем со сборкой и проверкой не возникло. Код написан с использованием 14-ого стандарта и насчитывает 65 тысяч строк:
|
Язык |
Файлы |
Пустые строки |
Комментарии |
Код |
|---|---|---|---|---|
|
C++ |
68 |
4732 |
4714 |
27569 |
|
C |
44 |
3961 |
5855 |
25680 |
|
C/C++ Header |
89 |
3171 |
5031 |
8571 |
|
yacc |
3 |
323 |
303 |
3083 |
|
lex |
3 |
84 |
81 |
571 |
|
CMake |
15 |
74 |
323 |
319 |
|
Perl |
1 |
23 |
33 |
106 |
|
Суммарно |
233 |
12469 |
16340 |
65899 |
Проект маленький и качественный, поэтому ошибок оказалось не так много. Но они все равно достойны внимания и исправления.
В первую очередь рассмотрим ошибки, приводящие к утечке ресурсов или падению программы.
Предупреждение N1
c_interface.cpp:1808: V773 The function was exited without closing the file referenced by the 'cvcin' handle. A resource leak is possible.
Expr vc_parseExpr(VC vc, const char* infile)
{
extern FILE *cvcin, *smtin;
cvcin = fopen(infile, "r"); // <=
if (cvcin == NULL)
{
fprintf(stderr, "STP: Error: cannot open %s\n", infile);
stp::FatalError("Cannot open file");
return 0;
}
CONSTANTBV::ErrCode c = CONSTANTBV::BitVector_Boot();
if (0 != c)
{
cout << CONSTANTBV::BitVector_Error(c) << endl;
return 0; // <=
}
....
return output; // <=
}В этом примере анализатор обнаружил утечку файлового дескриптора cvcin. Обратите внимание, функция fopen открывает файл, но при этом дальше в коде нет вызова fclose, которая бы закрыла его. В случае если cvcin == NULL, программа завершится с ошибкой: файл не найден. Но если выполнение достигнет тела второго условного оператора, дескриптор cvcin будет потерян. Исправленный вариант:
Expr vc_parseExpr(VC vc, const char* infile)
{
extern FILE *cvcin, *smtin;
cvcin = fopen(infile, "r");
if (cvcin == NULL)
{
....
stp::FatalError("Cannot open file");
return 0;
}
CONSTANTBV::ErrCode c = CONSTANTBV::BitVector_Boot();
if (0 != c)
{
cout << CONSTANTBV::BitVector_Error(c) << endl;
fclose(cvcin); // <=
return 0;
}
....
if (b->UserFlags.smtlib1_parser_flag)
{
smtin = cvcin;
cvcin = NULL; // <=
....
}
....
if(smtin != NULL)
fclose(smtin); // <=
else
fclose(cvcin); // <=
return output;
}Отметим, что такое решение неидеально. Если между fopen и fclose случится исключение или в результате внесенных изменений появится ещё одна точка выхода из функции, fclose вызвана не будет. Эту проблему можно решить, применив идиому RAII (Resource Acquisition Is Initialization). В C++ это реализуется с помощью деструкторов. Или можно воспользоваться unique_ptr:
template<typename T>
using DeletedPtr = std::unique_ptr<T, std::function<void(T*)>>;
Expr vc_parseExpr(VC vc, const char* infile)
{
DeletedPtr<FILE> cvcin(fopen(infile, "r"),
[](FILE* f)
{
fclose(f);
});
....
if (!cvcin)
{
....
stp::FatalError("Cannot open file");
return 0;
}
....
}Предупреждение N2
MutableASTNode.h:269: V505 The 'alloca' function is used inside the loop. This can quickly overflow stack.
Анализатор обнаружил использование функции alloca внутри цикла. Поскольку функция alloca использует стековую память, то ее многократный вызов в теле цикла может неожиданно привести к переполнению стека.
static void getDisjointExtractVariables(....)
{
const int size = all.size();
for (int i = size - 1; i >= 0; i--)
{
....
// TODO remove alloca
bool* found = (bool*)alloca(sizeof(bool) * node.GetValueWidth());
for (size_t j = 0; j < node.GetValueWidth(); j++)
found[j] = false;
....
}
}Функция alloca выделяет участок памяти на стеке, который будет очищен при выходе из функции. Даже если переменная found объявлена внутри цикла, выделенная под неё память все равно не освободится по окончании итерации. Такой код необязательно является ошибкой. Это зависит от размера стека, количества итераций в цикле и объема выделяемой памяти. Однако разработчик сам оставил комментарий, что alloca лучше убрать (или заменить на динамическую аллокацию, например). Код выше можно исправить, используя динамическую аллокацию на куче. Но и такой способ имеет свои минусы.
const int size = all.size();
for (int i = size - 1; i >= 0; i--)
{
....
// TODO remove alloca
bool* found = (bool*)calloc(sizeof(bool), node.GetValueWidth());
....
free(found);
}Есть ещё несколько предупреждений такого рода в коде:
bool changed = true;
while (changed)
{
changed = false;
signed* columnH = (signed*)alloca(sizeof(signed) * bitWidth);//(1)
signed* columnL = (signed*)alloca(sizeof(signed) * bitWidth);//(2)
signed* sumH = (signed*)alloca(sizeof(signed) * bitWidth); //(3)
signed* sumL = (signed*)alloca(sizeof(signed) * bitWidth); //(4)
....
// working with 'changed';
....
}Предупреждение N3
STPManager.cpp:549: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 543, 549.
Анализатор обнаружил код, в котором рядом находятся два оператора if с одинаковыми условиями. Это является потенциальной ошибкой или избыточным кодом.
bool STPMgr::VarSeenInTerm(const ASTNode& var, const ASTNode& term)
{
if (READ == term.GetKind() && WRITE == term[0].GetKind()
/*&& !GetRemoveWritesFlag()*/)
{
return false; // <=
}
if (READ == term.GetKind() && WRITE == term[0].GetKind()
/*&& GetRemoveWritesFlag()*/)
{
return true; // <= (unreachable statement)
}
....
}В данном примере при одинаковых условиях выполняется противоположный по смыслу код. Скорее всего, закомментированный код в условных выражениях играл важную роль. Если его убрать, то вторая проверка становится лишней. Или, возможно, разработчик имел виду term[1] во втором выражении:
if (READ == term.GetKind())
{
if(WRITE == term[0].GetKind())
return false;
if(WRITE == term[1].GetKind()) // <=
return true;
}Предупреждение N4
FixedBits.h:194: V524 It is odd that the body of 'minimum_numberOfTrailingZeroes' function is fully equivalent to the body of 'minimum_trailingOne' function.
unsigned minimum_numberOfTrailingZeroes() // <=
{
unsigned i = 0;
for (; i < getWidth(); i++)
{
if (!isFixed(i) || getValue(i))
break;
}
return i;
}
unsigned minimum_trailingOne() // <=
{
unsigned i = 0;
for (; i < getWidth(); i++)
{
if (!isFixed(i) || getValue(i))
break;
}
return i;
}Данное предупреждение выдается в случае, если анализатор обнаружил две функции, тела которых полностью идентичны. Наличие двух одинаковых функций само по себе необязательно является ошибкой, но это повод обратить на них внимание. Так как мы не разрабатываем эту библиотеку, то можем только предположить наличие ошибки. Ведь названия функций имеют совершенно разные значения. Если это не ошибка, то разумно переписать код так, чтобы не возникала путаница.
unsigned minimum_numberOfTrailingZeroes()
{
unsigned i = 0;
for (; i < getWidth(); i++)
{
if (!isFixed(i) || getValue(i))
break;
}
return i;
}
unsigned minimum_trailingOne
{
return minimum_numberOfTrailingZeroes();
}В таком примере уже становится понятно, что имел в виду разработчик. Кроме того, уменьшается вероятность того, что ошибка будет допущена в результате изменения только одной из функций.
Есть ещё похожие срабатывания:
Предупреждение N5
UnsignedIntervalAnalysis.cpp:276: V547 Expression 'bottomChanged' is always false.
UnsignedInterval* UnsignedIntervalAnalysis::visit(....)
{
....
if (bottomChanged) // might have been zero. // <=
{
if (CONSTANTBV::BitVector_Lexicompare(result->minV, c1Min) > 0)
{
CONSTANTBV::BitVector_Copy(result->minV,
c1Min); //c1 should still be 1
}
if (CONSTANTBV::BitVector_Lexicompare(result->maxV, c1Min) < 0)
{
CONSTANTBV::BitVector_Copy(result->maxV,
c1Min); //c1 should still be 1
}
}
}Анализатор обнаружил, что bottomChanged всегда равен false. Вполне возможно, что это не ошибка. Но, если посмотреть на код выше, становится понятно, что, скорее всего, что-то тут не так.
UnsignedInterval* UnsignedIntervalAnalysis::visit(....)
{
switch(n.GetCind())
{
....
case BVDIV:
{
....
bool bottomChanged = false;
if (CONSTANTBV::BitVector_is_empty(c1->minV)) // <= (1)
{
if (CONSTANTBV::BitVector_is_empty(c1->maxV))
{
....
break; // result is [1111..111, 11...11111] // <= (2)
}
bottomChanged = true; // <= (3)
CONSTANTBV::BitVector_Destroy(c1Min);
break; // TODO fix so that it can run-on.
}
....
if (bottomChanged). // <= (4)
{
.... //// <= (unreachable statement)
}
break;
}
}
}Выражение if (bottomChanged) находится в теле оператора switch. В случае когда bottomChanged устанавливается в true (см. метку 2), происходит выход из текущей ветки выполнения. В результате, если поток управления пришел к метке 4, bottomChanged равен false.
Анализатор обнаружил достаточно много похожих срабатываний, вот некоторые из них:
Не все ошибки проявляют себя сразу после того, как появились на свет. Чаще они терпеливо ждут своего часа: когда программист что-то поменяет в коде или поток выполнения зайдёт в потаённый угол. Своевременное исправление таких ошибок может сэкономить много времени в будущем.
Предупреждение N6
В следующем примере нет ошибки, но она может возникнуть в результате рефакторинга или изменения логики кода:
Dependencies.h:151: V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop.
Анализатор обнаружил ситуацию, при которой итератор перекрывается внутри цикла:
void print() const
{
auto it = dependents.begin(); // <=
for (/**/; it != dependents.end(); it++)
{
cout << (it->first).GetNodeNum();
const set<ASTNode>* dep = it->second;
set<ASTNode>::iterator it = dep->begin(); // <=
while (it != dep->end())
{
cout << " " << (*it).GetNodeNum();
it++;
}
cout << endl;
}
}Если по ошибке перенести it++ в конец цикла, то это приведёт к неправильной работе программы. Надежнее будет переименовать внутренний итератор или использовать цикл for:
void print() const
{
for (const auto &depnt : dependents)
{
cout << (depnt.first).GetNodeNum();
const set<ASTNode>* dep = depnt.second;
for (const auto &inDep : dep)
{
cout << " " << inDep.GetNodeNum();
}
cout << endl;
}
}Предупреждение N7
AssortedPrinters.cpp:93: V688 The 'ListOfDeclaredVars' function argument possesses the same name as one of the class members, which can result in a confusion.
void STPMgr::printVarDeclsToStream(ostream& os, ASTNodeSet& ListOfDeclaredVars)
{
for (ASTNodeSet::iterator i = ListOfDeclaredVars.begin(),
iend = ListOfDeclaredVars.end();
{
....
}
}Вот ещё похожее предупреждение. Переменная ListOfDeclaredVars заменяет собой член класса с тем же именем:
class STPMgr
{
....
// For printing purposes
// Used just by the CVC parser.
ASTVec ListOfDeclaredVars;
....
}Пусть это и не ошибка, но может запутать программиста, который будет править код. Лучше избежать такой ситуации и переименовать локальную переменную.
В следующих примерах анализатор обнаружил возможности рефакторинга для повышения быстродействия или читаемости кода.
Предупреждение N8
SimplifyingNodeFactory.cpp:1379: V560 A part of conditional expression is always true: children.size() == 2.
ASTNode SimplifyingNodeFactory::CreateTerm(....)
{
if (children.size() == 2) // <=(1)
{
if (children.size() == 2 && children[0] == children[1]) // <=(2)
{
result = bm.CreateZeroConst(width);
}
else if (children.size() == 2 && // <=(3)
children[1] == bm.CreateZeroConst(width))
{
result = children[0];
}
else
{
result = NodeFactory::CreateTerm(
BVPLUS, width, children[0],
NodeFactory::CreateTerm(BVUMINUS, width, children[1]));
}
}
}Так на метке 1 уже проверяется размер контейнера, дополнительно делать это в условиях 2 и 3 не требуется. В данном случае нет ошибки только потому, что условие проверяется через 'И'. Это может измениться в будущем. Исправленный вариант:
ASTNode SimplifyingNodeFactory::CreateTerm(....)
{
if (children.size() == 2) // <= (1)
{
if (children[0] == children[1]) // <= (2)
....
else if (children[1] == bm.CreateZeroConst(width))
....
else
....
}
}Предупреждение N9
FixedBits.cpp:405: warning: V560 A part of conditional expression is always true: i < width.
void FixedBits::fromUnsigned(unsigned val)
{
for (unsigned i = 0; i < width; i++)
{
if (i < width && i < sizeof(unsigned) * 8) // <=
{
setFixed(i, true);
setValue(i, (val & (1 << i)));
}
else if (i < width) // <=
{
setFixed(i, true);
setValue(i, false);
}
else // The unsigned value is bigger than the bitwidth of this.
{ // so it can't be represented.
if (val & (1 << i)) // <= (unreachable statement)
{
stp::FatalError(LOCATION "Cant be represented.");
}
}
}
}Счётчик цикла существует в пределах от 0 до width, соответственно условие i < width всегда истинно. Исправленный вариант:
void FixedBits::fromUnsigned(unsigned val)
{
for (unsigned i = 0; i < width; i++)
{
setFixed(i, true);
if (i < sizeof(unsigned) * 8)
setValue(i, (val & (1 << i)));
else
setValue(i, false);
}
}Предупреждение N10
cpp_interface.cpp:151: V669 The 'strval' argument is a non-constant reference. The analyzer is unable to determine the position at which this argument is being modified. It is possible that the function contains an error.
ASTNode Cpp_interface::CreateBVConst(string& strval,
int base,
int bit_width)
{
return bm.CreateBVConst(strval, base, bit_width);
}Анализатор обнаружил, что в функции параметр strval передан через ссылку, но нигде не модифицируется. Если мы посмотрим на функцию bm.CreateBVConst, то увидим, что передача параметра strval происходит по значению:
ASTNode STPMgr::CreateBVConst(string strval,
int base,
int bit_width)
{
....
}Это может свидетельствовать об ошибке, но, скорее всего, параметр strval следует сделать ссылкой на константу. Если мы посмотрим в тело функции STPMgr::CreateBVConst, то обнаружим, что strval также не модифицируется. Это позволяет нам передавать строку по ссылке и избегать лишнего копирования:
ASTNode Cpp_interface::CreateBVConst(const string& strval,
int base,
int bit_width)
{
return bm.CreateBVConst(strval, base, bit_width);
}
ASTNode STPMgr::CreateBVConst(const string& strval,
int base,
int bit_width)
{
if (bit_width <= 0)
{
FatalError("Bit width of constant must be greater than 0");
}
assert(bit_width > 0);
return charToASTNode((unsigned char*)strval.c_str(), base,
bit_width);
}Функция charToASTNode также не модифицирует строку. На неё стоит обратить внимание, если принять это исправление.
В связи с болезнями, карантинами и новогодними праздниками написание статьи затянулось на пару месяцев, и, возможно, некоторые из описанных здесь ошибок к моменту публикации уже исправлены. Статья просто демонстрирует возможности анализатора, а не ставит своей целью выявить как можно больше ошибок. Стоит понимать, что статические анализаторы кода дают наибольшую эффективность при постоянном использовании. Такой подход позволяет устранять ошибки до того, как они на что-то повлияют.
Как видно, анализатор нашел много подозрительных мест. Возможно, найденные потенциальные проблемы пока никак себя не проявляют, но их наличие не может не тревожить. Ведь они способны выстрелить в самый неожиданный момент. Выявлять ошибки на этапе разработки лучше, чем чинить бесконечный поток багов перед релизом. Для того чтобы попробовать статический анализатор PVS-Studio на своём проекте, вы можете перейти по ссылке.