# Проверка STP с помощью PVS\-Studio

Статический анализ помогает разработчикам отлавливать ошибки на раннем этапе и повышает качество и надёжность кода\. В этой статье приводится разбор некоторых потенциальных уязвимостей и ошибок, найденных в исходном коде библиотеки STP с помощью PVS\-Studio\.

## О проекте

![0810_stp_ru/image1.png](https://import.viva64.com/docx/blog/0810_stp_ru/image1.png)

Описание проекта [STP](https://github.com/stp/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](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without closing the file referenced by the 'cvcin' handle\. A resource leak is possible\.

```cpp
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_ будет потерян\. Исправленный вариант:

```cpp
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_:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v505/) The 'alloca' function is used inside the loop\. This can quickly overflow stack\.

Анализатор обнаружил использование функции _alloca_ внутри цикла\. Поскольку функция _alloca_ использует стековую память, то ее многократный вызов в теле цикла может неожиданно привести к переполнению стека\. 

```cpp
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_ лучше убрать \(или заменить на динамическую аллокацию, например\)\. Код выше можно исправить, используя динамическую аллокацию на куче\. Но и такой способ имеет свои минусы\. 

```cpp
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);
}
```

Есть ещё несколько предупреждений такого рода в коде:

* ConstantBitP\_Multiplication\.cpp:599:
* ConstantBitP\_Multiplication\.cpp:602:
* ConstantBitP\_Multiplication\.cpp:603:
* ConstantBitP\_Multiplication\.cpp:604:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v581/) The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 543, 549\.

Анализатор обнаружил код, в котором рядом находятся два оператора _if_ с одинаковыми условиями\. Это является потенциальной ошибкой или избыточным кодом\. 

```cpp
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\] во втором выражении:

```cpp
if (READ == term.GetKind())
{
  if(WRITE == term[0].GetKind())
    return false; 
  if(WRITE == term[1].GetKind()) // <=
    return true;
}
```

**Предупреждение N4**

FixedBits\.h:194: [V524](https://pvs-studio.ru/ru/docs/warnings/v524/) It is odd that the body of 'minimum\_numberOfTrailingZeroes' function is fully equivalent to the body of 'minimum\_trailingOne' function\. 

```cpp
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;
}
```

Данное предупреждение выдается в случае, если анализатор обнаружил две функции, тела которых полностью идентичны\. Наличие двух одинаковых функций само по себе необязательно является ошибкой, но это повод обратить на них внимание\. Так как мы не разрабатываем эту библиотеку, то можем только предположить наличие ошибки\. Ведь названия функций имеют совершенно разные значения\. Если это не ошибка, то разумно переписать код так, чтобы не возникала путаница\.

```cpp
unsigned minimum_numberOfTrailingZeroes()
{
  unsigned i = 0;
  for (; i < getWidth(); i++)
  {
    if (!isFixed(i) || getValue(i))
      break;
  }
  return i;
}

unsigned minimum_trailingOne
{
  return minimum_numberOfTrailingZeroes(); 
}
```

В таком примере уже становится понятно, что имел в виду разработчик\. Кроме того, уменьшается вероятность того, что ошибка будет допущена в результате изменения только одной из функций\.

Есть ещё похожие срабатывания:

* c\_interface\.cpp:1526: note: V524 It is odd that the body of 'vc\_bvBoolExtract\_Zero' function is fully equivalent to the body of 'vc\_bvBoolExtract' function\.
* c\_interface\.cpp:1181: note: V524 It is odd that the body of 'vc\_bvRemExpr' function is fully equivalent to the body of 'vc\_bvModExpr' function\.
* constantBitP/FixedBits\.h:205: note: V524 It is odd that the body of 'maximum\_numberOfTrailingZeroes' function is fully equivalent to the body of 'maximum\_trailingOne' function\.

**Предупреждение N5**

UnsignedIntervalAnalysis\.cpp:276: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'bottomChanged' is always false\.

```cpp
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_\. Вполне возможно, что это не ошибка\. Но, если посмотреть на код выше, становится понятно, что, скорее всего, что\-то тут не так\.

```cpp
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_\.

Анализатор обнаружил достаточно много похожих срабатываний, вот некоторые из них:

* ConstantBitP\_Division\.cpp:197: error: V547 Expression 'whatIs \=\= QUOTIENT\_IS\_OUTPUT' is always true\.
* DifficultyScore\.cpp:87: warning: V547 Expression 'k \=\= EQ' is always false\.
* ConstantBitP\_Multiplication\.cpp:695: error: V547 Expression 'r \!\= CONFLICT' is always true\.
* FixedBits\.cpp:410: warning: V547 Expression 'i < width' is always true\.

## Потенциальные ошибки

Не все ошибки проявляют себя сразу после того, как появились на свет\. Чаще они терпеливо ждут своего часа: когда программист что\-то поменяет в коде или поток выполнения зайдёт в потаённый угол\. Своевременное исправление таких ошибок может сэкономить много времени в будущем\.

**Предупреждение N6**

В следующем примере нет ошибки, но она может возникнуть в результате рефакторинга или изменения логики кода:

Dependencies\.h:151: [V711](https://pvs-studio.ru/ru/docs/warnings/v711/) It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop\.

Анализатор обнаружил ситуацию, при которой итератор перекрывается внутри цикла:

```cpp
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_:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v688/) The 'ListOfDeclaredVars' function argument possesses the same name as one of the class members, which can result in a confusion\. 

```cpp
void STPMgr::printVarDeclsToStream(ostream& os, ASTNodeSet& ListOfDeclaredVars)
{
  for (ASTNodeSet::iterator i = ListOfDeclaredVars.begin(),
                            iend = ListOfDeclaredVars.end();
  {
    ....
  }
}
```

Вот ещё похожее предупреждение\. Переменная _ListOfDeclaredVars_ заменяет собой член класса с тем же именем:

```cpp
class STPMgr
{
  ....
  // For printing purposes
  // Used just by the CVC parser.
  ASTVec ListOfDeclaredVars;
  ....
}
```

Пусть это и не ошибка, но может запутать программиста, который будет править код\. Лучше избежать такой ситуации и переименовать локальную переменную\.

## Возможности для оптимизации/упрощения кода

В следующих примерах анализатор обнаружил возможности рефакторинга для повышения быстродействия или читаемости кода\.

**Предупреждение N8**

SimplifyingNodeFactory\.cpp:1379: [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: children\.size\(\) \=\= 2\.

```cpp
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 не требуется\. В данном случае нет ошибки только потому, что условие проверяется через 'И'\. Это может измениться в будущем\. Исправленный вариант:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: i < width\.

```cpp
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_ всегда истинно\. Исправленный вариант:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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\.

```cpp
ASTNode Cpp_interface::CreateBVConst(string& strval, 
                                     int base, 
                                     int bit_width)
{
  return bm.CreateBVConst(strval, base, bit_width);
}
```

Анализатор обнаружил, что в функции параметр _strval_ передан через ссылку, но нигде не модифицируется\. Если мы посмотрим на функцию _bm\.CreateBVConst_, то увидим, что передача параметра _strval_ происходит по значению:

```cpp
ASTNode STPMgr::CreateBVConst(string strval, 
                              int base, 
                              int bit_width)
{
  ....
}
```

Это может свидетельствовать об ошибке, но, скорее всего, параметр _strval_ следует сделать ссылкой на константу\. Если мы посмотрим в тело функции _STPMgr::CreateBVConst_, то обнаружим, что _strval_ также не модифицируется\. Это позволяет нам передавать строку по ссылке и избегать лишнего копирования:

```cpp
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 на своём проекте, вы можете перейти по [ссылке](https://pvs-studio.ru/ru/pvs-studio/download/)\.