Проверка Clang 11 с помощью PVS-Studio27.10.2020 16:47

PVS-Studio: Всё ещё достоин!


Время от времени нам приходится писать статьи о проверке очередной версии какого-то компилятора. Это неинтересно. Однако, как показывает практика, если этого долго не делать, люди начинают сомневаться, достоин ли анализатор PVS-Studio звания хорошего ловца багов и потенциальных уязвимостей. Возможно, новый компилятор уже умеет это делать? Да, компиляторы не стоят на месте. Однако развивается и PVS-Studio, вновь и вновь демонстрируя возможности находить ошибки даже в коде таких высококачественных проектов, как компиляторы.

Время перепроверить код Clang


Честно говоря, за основу этого текста я взял статью «Проверка компилятора GCC 10 с помощью PVS-Studio». Так что если вам покажется, что вы уже читали где-то некоторые абзацы, то вам не кажется :).

Не секрет, что в компиляторах существуют свои собственные встроенные статические анализаторы кода и они тоже развиваются. Поэтому, время от времени мы пишем статьи о том, что статический анализатор PVS-Studio может находить ошибки даже внутри компиляторов и что мы не зря едим хлеб :).

На самом деле, сравнивать классические статические анализаторы с компиляторами нельзя. Статические анализаторы — это не только поиск ошибок в коде, но и развитая инфраструктура. Например, это интеграция с такими системами, как SonarQube, PlatformIO, Azure DevOps, Travis CI, CircleCI, GitLab CI/CD, Jenkins, Visual Studio. Это развитые механизмы массового подавления предупреждений, что позволяет быстро начать использовать PVS-Studio даже в большом старом проекте. Это рассылка уведомлений. И так далее, и так далее. Однако, всё равно первый вопрос, который задают: «Может ли PVS-Studio найти такое, что не могут найти компиляторы?». А значит, мы будем вновь и вновь писать статьи о проверке этих самих компиляторов.

Вернёмся к теме проверки проекта Clang. Нет нужды останавливаться на этом проекте и рассказывать, что он из себя представляет. На самом деле, был проверен код не только самого Clang 11, но и библиотеки LLVM 11, на которой он построен. С точки зрения этой статьи, нет разницы, найден дефект в коде компилятора или библиотеки.

Код Clang/LLVM показался мне гораздо понятнее кода GCC. По крайней мере, нет всех этих ужасных макросов, и активно применяются современные возможности языка C++.

Несмотря на это, проект большой, и без предварительных настроек анализатора просматривать отчёт всё равно весьма утомительно. В основном, мешают «полуложные» срабатывания. Под «полуложными» срабатываниями я понимаю ситуации, когда формально анализатор прав, но толку от предупреждения никакого нет. Например, множество таких срабатываний выдаётся на юнит-тесты и сгенерированный код.

Пример для тестов: 

Spaces.SpacesInParentheses = false;               // <=
Spaces.SpacesInCStyleCastParentheses = true;      // <=
verifyFormat("Type *A = ( Type * )P;", Spaces);
verifyFormat("Type *A = ( vector )P;", Spaces);
verifyFormat("x = ( int32 )y;", Spaces);
verifyFormat("int a = ( int )(2.0f);", Spaces);
verifyFormat("#define AA(X) sizeof((( X * )NULL)->a)", Spaces);
verifyFormat("my_int a = ( my_int )sizeof(int);", Spaces);
verifyFormat("#define x (( int )-1)", Spaces);

// Run the first set of tests again with:
Spaces.SpacesInParentheses = false;               // <=
Spaces.SpaceInEmptyParentheses = true;
Spaces.SpacesInCStyleCastParentheses = true;      // <=
verifyFormat("call(x, y, z);", Spaces);
verifyFormat("call( );", Spaces);


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

  • V1048 The 'Spaces.SpacesInParentheses' variable was assigned the same value. FormatTest.cpp 11554
  • V1048 The 'Spaces.SpacesInCStyleCastParentheses' variable was assigned the same value. FormatTest.cpp 11556


Формально анализатор выдал верное срабатывание, и это является тем фрагментом кода, который стоит упростить или исправить. При этом понятно, что на самом деле всё хорошо и что править что-то тоже смысла нет.

Другой пример: анализатор выдаёт огромное количество предупреждений на автосгенерированный файл Options.inc. В файле можно видеть «простыню кода»:

Код


И на всё это PVS-Studio сыплет предупреждения:

  • V501 There are identical sub-expressions to the left and to the right of the '==' operator: nullptr == nullptr Options.inc 26
  • V501 There are identical sub-expressions to the left and to the right of the '==' operator: nullptr == nullptr Options.inc 27
  • V501 There are identical sub-expressions to the left and to the right of the '==' operator: nullptr == nullptr Options.inc 28
  • и так далее на каждую строчку…


Всё это нестрашно. Всё это можно победить: отключить проверку лишних файлов, разметить некоторые макросы и функции, подавить определённые типы срабатываний и так далее. Можно, но заниматься этим в рамках задачи по написанию статьи неинтересно. Поэтому, я поступил точно так же, как и в статье про компилятор GCC. Я изучал отчёт, пока у меня не набралось 11 интересных примеров кода для написания статьи. Почему 11? Я подумал, что раз версия Clang равна 11, то пусть и фрагментов будет 11:).

11 подозрительных фрагментов кода


Фрагмент N1, деление по модулю на единицу

% 1 - деление по модулю на 1


Классная ошибка! Люблю такие! 

void Act() override {
  ....
  // If the value type is a vector, and we allow vector select, then in 50%
  // of the cases generate a vector select.
  if (isa(Val0->getType()) && (getRandom() % 1)) {
    unsigned NumElem =
        cast(Val0->getType())->getNumElements();
    CondTy = FixedVectorType::get(CondTy, NumElem);
  }
  ....
}


Предупреждение PVS-Studio: V1063 The modulo by 1 operation is meaningless. The result will always be zero. llvm-stress.cpp 631

Для того, чтобы получать случайное значение 0 или 1, используют деление по модулю. Но, видимо, это значение 1 сбивает с толку, и люди допускают классический паттерн ошибки, деля на единицу, хотя необходимо делит на двойку. Операция X % 1 не имеет смысла, так как в результате даёт всегда 0. Правильный вариант кода: 

if (isa(Val0->getType()) && (getRandom() % 2)) {


Диагностика V1063, недавно появившаяся в PVS-Studio, до безобразия простая, но, как видите, работает.

Как мы знаем, разработчики компиляторов посматривают на то, что мы делаем, и заимствуют наши наработки. Ничего плохого в этом нет. Нам приятно, что PVS-Studio является двигателем прогресса. Засекаем время, через сколько такая же диагностика появится в Clang и GCC:).

Фрагмент N2, опечатка в условии

class ReturnValueSlot {
  ....
  bool isNull() const { return !Addr.isValid(); }
  ....
};

static bool haveSameParameterTypes(ASTContext &Context, const FunctionDecl *F1,
                                   const FunctionDecl *F2, unsigned NumParams) {
  ....
  unsigned I1 = 0, I2 = 0;
  for (unsigned I = 0; I != NumParams; ++I) {
    QualType T1 = NextParam(F1, I1, I == 0);
    QualType T2 = NextParam(F2, I2, I == 0);
    if (!T1.isNull() && !T1.isNull() && !Context.hasSameUnqualifiedType(T1, T2))
      return false;
  }
  return true;
}


Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: ! T1.isNull () && ! T1.isNull () SemaOverload.cpp 9493

Дважды выполняется проверка ! T1.isNull (). Это явная опечатка, и вторая часть условия должна проверять переменную T2.

Фрагмент N3, потенциальный выход за границу массива

std::vector DeclsLoaded;

SourceLocation ASTReader::getSourceLocationForDeclID(GlobalDeclID ID) {
  ....
  unsigned Index = ID - NUM_PREDEF_DECL_IDS;

  if (Index > DeclsLoaded.size()) {
    Error("declaration ID out-of-range for AST file");
    return SourceLocation();
  }

  if (Decl *D = DeclsLoaded[Index])
    return D->getLocation();
  ....
}


Предупреждение PVS-Studio: V557 Array overrun is possible. The 'Index' index is pointing beyond array bound. ASTReader.cpp 7318

Предположим, в массиве хранится один элемент, и значение переменной Index тоже равно единице. Условие (1 > 1) является ложным, и как результат произойдёт выход за границу массива. Правильная проверка: 

if (Index >= DeclsLoaded.size()) {


Фрагмент N4, порядок вычислений аргументов

void IHexELFBuilder::addDataSections() {
  ....
  uint32_t SecNo = 1;
  ....
  Section = &Obj->addSection(
      ".sec" + std::to_string(SecNo++), RecAddr,
      ELF::SHF_ALLOC | ELF::SHF_WRITE, SecNo);
  ....
}


Предупреждение PVS-Studio: V567 Unspecified behavior. The order of argument evaluation is not defined for 'addSection' function. Consider inspecting the 'SecNo' variable. Object.cpp 1223

Обратите внимание, что аргумент SecNo используется дважды, да ещё попутно инкрементируется. Вот только невозможно сказать, в каком порядке будут вычисляться аргументы. Поэтому результат будет меняться в зависимости от версии компилятора или настроек компиляции.

Поясню это синтетическим примером: 

#include 
int main()
{
  int i = 1;
  printf("%d, %d\n", i, i++);
  return 0;
}


В зависимости от компилятора, может быть распечатано, как »1, 2», так и »2, 1». Воспользовавшись Compiler Explorer я получит следующие результаты:

  • программа, скомпилированная с помощью Clang 11.0.0, выдаёт: 1, 1.
  • программа, скомпилированная с помощью GCC 10.2, выдаёт: 2, 1.


Что интересно, для этого простого случая компилятор Clang выдаёт предупреждение: 

:6:26: warning:
unsequenced modification and access to 'i' [-Wunsequenced]
printf("%d, %d\n", i, i++);


Видимо, в реальных условиях это предупреждение не помогло. Диагностика или отключена, так как неудобна для практического использования, или компилятор не смог предупредить о более сложном случае.

Фрагмент N5, странная повторная проверка

template 
void GNUStyle::printVersionSymbolSection(const ELFFile *Obj,
                                               const Elf_Shdr *Sec) {

  ....
  Expected NameOrErr =
      this->dumper()->getSymbolVersionByIndex(Ndx, IsDefault);
  if (!NameOrErr) {
    if (!NameOrErr) {
      unsigned SecNdx = Sec - &cantFail(Obj->sections()).front();
      this->reportUniqueWarning(createError(
          "unable to get a version for entry " + Twine(I) +
          " of SHT_GNU_versym section with index " + Twine(SecNdx) + ": " +
          toString(NameOrErr.takeError())));
    }
    Versions.emplace_back("");
    continue;
  }
  ....
}


Предупреждение PVS-Studio: V571 Recurring check. The 'if (! NameOrErr)' condition was already verified in line 4666. ELFDumper.cpp 4667

Вторая проверка дублирует первую и является избыточной. Возможно, вторую проверку можно просто удалить. Но скорее всего, это опечатка, и во втором условии должна использоваться другая переменная.

Фрагмент N6, разыменование потенциально нулевого указателя

void RewriteObjCFragileABI::RewriteObjCClassMetaData(
  ObjCImplementationDecl *IDecl, std::string &Result)
{
  ObjCInterfaceDecl *CDecl = IDecl->getClassInterface();

  if (CDecl->isImplicitInterfaceDecl()) {
    RewriteObjCInternalStruct(CDecl, Result);
  }

  unsigned NumIvars = !IDecl->ivar_empty()
  ? IDecl->ivar_size()
  : (CDecl ? CDecl->ivar_size() : 0);
  ....
}


Предупреждение PVS-Studio: V595 The 'CDecl' pointer was utilized before it was verified against nullptr. Check lines: 5275, 5284. RewriteObjC.cpp 5275

В процессе выполнения первой проверки указатель CDecl всегда смело разыменовывается: 

if (CDecl->isImplicitInterfaceDecl())


И только из кода, написанного ниже, становится понятно, что этот указатель может быть нулевым: 

(CDecl ? CDecl->ivar_size() : 0)


Скорее всего, первая проверка должна выглядеть так: 

if (CDecl && CDecl->isImplicitInterfaceDecl())


Фрагмент N7, разыменование потенциально нулевого указателя

bool
Sema::InstantiateClass(....)
{
  ....
  NamedDecl *ND = dyn_cast(I->NewDecl);
  CXXRecordDecl *ThisContext =
      dyn_cast_or_null(ND->getDeclContext());
  CXXThisScopeRAII ThisScope(*this, ThisContext, Qualifiers(),
                              ND && ND->isCXXInstanceMember());
  ....
}


Предупреждение PVS-Studio: V595 The 'ND' pointer was utilized before it was verified against nullptr. Check lines: 2803, 2805. SemaTemplateInstantiate.cpp 2803

Разновидность предыдущей ошибки. Опасно разыменовывать указатель без предварительной проверки, если его значение получено с помощью динамического приведения типа. Более того, из кода, расположенного ниже, видно, что такая проверка нужна.

Фрагмент N8, функция продолжает выполнение, несмотря на ошибочное состояние

bool VerifyObject(llvm::yaml::Node &N,
                  std::map Expected) {
  ....
  auto *V = llvm::dyn_cast_or_null(Prop.getValue());
  if (!V) {
    ADD_FAILURE() << KS << " is not a string";
    Match = false;
  }
  std::string VS = V->getValue(Tmp).str();
  ....
}


Предупреждение PVS-Studio: V1004 The 'V' pointer was used unsafely after it was verified against nullptr. Check lines: 61, 65. TraceTests.cpp 65

Указатель V может оказаться нулевым. Это явно ошибочное состояние, о чём даже выдаётся сообщение. Однако, после этого, функция как ни в чём ни бывало продолжает выполнение, что приведёт к разыменованию этого самого нулевого указателя. Возможно, здесь забыли прервать работу функции, и правильный вариант должен выглядеть так: 

auto *V = llvm::dyn_cast_or_null(Prop.getValue());
if (!V) {
  ADD_FAILURE() << KS << " is not a string";
  Match = false;
  return false;
}
std::string VS = V->getValue(Tmp).str();


Фрагмент N9, опечатка

const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
                                  const InputInfo &Output) {
  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
    if (StringRef(A->getValue()) == "single")
      return Args.MakeArgString(Output.getFilename());

  Arg *FinalOutput = Args.getLastArg(options::OPT_o);
  if (FinalOutput && Args.hasArg(options::OPT_c)) {
    SmallString<128> T(FinalOutput->getValue());
    llvm::sys::path::replace_extension(T, "dwo");
    return Args.MakeArgString(T);
  } else {
    // Use the compilation dir.
    SmallString<128> T(
        Args.getLastArgValue(options::OPT_fdebug_compilation_dir));
    SmallString<128> F(llvm::sys::path::stem(Input.getBaseInput()));
    llvm::sys::path::replace_extension(F, "dwo");
    T += F;
    return Args.MakeArgString(F);       // <=
  }
}


Предупреждение PVS-Studio: V1001 The 'T' variable is assigned but is not used by the end of the function. CommonArgs.cpp 873

Обратите внимание на конец функции. Локальная переменная T изменяется, но никак не используется. Скорее всего, это опечатка и функция должна завершаться следующими строчками кода: 

T += F;
return Args.MakeArgString(T);


Фрагмент N10, делитель равен нулю

typedef int32_t si_int;
typedef uint32_t su_int;

typedef union {
  du_int all;
  struct {
#if _YUGA_LITTLE_ENDIAN
    su_int low;
    su_int high;
#else
    su_int high;
    su_int low;
#endif // _YUGA_LITTLE_ENDIAN
  } s;
} udwords;

COMPILER_RT_ABI du_int __udivmoddi4(du_int a, du_int b, du_int *rem) {
  ....
  if (d.s.low == 0) {
    if (d.s.high == 0) {
      // K X
      // ---
      // 0 0
      if (rem)
        *rem = n.s.high % d.s.low;
      return n.s.high / d.s.low;
    }
  ....
}


Предупреждения PVS-Studio:

  • V609 Mod by zero. Denominator 'd.s.low' == 0. udivmoddi4.c 61
  • V609 Divide by zero. Denominator 'd.s.low' == 0. udivmoddi4.c 62


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

Фрагмент N11, Copy-Paste

bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly(....)
{
  ....
  StringRef FName = II->getName();
  ....
  if (FName == "postEvent" &&
      FD->getQualifiedNameAsString() == "QCoreApplication::postEvent") {
    return true;
  }

  if (FName == "postEvent" &&
      FD->getQualifiedNameAsString() == "QCoreApplication::postEvent") {
    return true;
  }
  ....
}


Предупреждение PVS-Studio: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 3108, 3113. MallocChecker.cpp 3113

Фрагмент кода был скопирован, но никак не изменён. Второй фрагмент следует либо удалить, либо модифицировать, чтобы он начал выполнять какую-то полезную проверку.

Заключение


04b0d5022b3a6cfadc4732dce7842da7.png


Напоминаю, что для проверки открытых проектов вы можете использовать вот этот бесплатный вариант лицензии. Кстати, есть и другие варианты бесплатного лицензирования PVS-Studio, в том числе даже для закрытых проектов. Они перечислены здесь: «Бесплатные варианты лицензирования PVS-Studio». Спасибо за внимание.

Другие наши статьи про проверку компиляторов


d51k2edrgxyrg5voswox_46pg3o.png


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking Clang 11 with PVS-Studio.

© Habrahabr.ru