# Как выстрелить себе в ногу в C и C\+\+\. Сборник рецептов Haiku OS

История встречи статического анализатора PVS\-Studio с кодом операционной системы Haiku уходит в далёкий 2015\-й год\. Это был интереснейший эксперимент и полезный опыт для команд обоих проектов\. Почему эксперимент? Анализатора для Linux тогда не было и не будет ещё полтора года\. Но труды энтузиастов нашей команды были вознаграждены: мы познакомились с разработчиками Haiku и повысили качество кода, пополнили базу редкими ошибками программистов и доработали анализатор\. Сейчас проверить код Haiku на наличие ошибок можно легко и быстро\.

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

## Введение

Герои нашей истории \- операционная система [Haiku](https://www.haiku-os.org/) с открытым исходным кодом и статический анализатор [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/) для языков C, C\+\+, C\# и Java\. Когда 4\.5 года назад мы взялись за анализ проекта, то пришлось работать только со скомпилированным исполняемым файлом анализатора\. Вся инфраструктура для парсинга параметров компиляции, запуска препроцессора, распараллеливания анализа и т\.д\. была взята из утилиты [Compiler Monitoring UI](https://pvs-studio.ru/ru/docs/manual/0031/) на языке C\#, которая частями была портирована на платформу Mono, чтобы запускаться в Linux\. Сам проект Haiku собирается с помощью кросс\-компилятора под разными операционными системами, кроме Windows\. Очередной раз хочу отметить удобство и полноту документации по сборке Haiku, а также поблагодарить разработчиков Haiku за помощь в сборке проекта\.

Сейчас выполнить анализ можно гораздо проще\. Список всех команд для сборки и анализа проекта выглядит так:

```cpp
cd /opt
git clone https://review.haiku-os.org/buildtools
git clone https://review.haiku-os.org/haiku
cd ./haiku
mkdir generated.x86_64; cd generated.x86_64
../configure --distro-compatibility official -j12 \
  --build-cross-tools x86_64 ../../buildtools
cd ../../buildtools/jam
make all
cd /opt/haiku/generated.x86_64
pvs-studio-analyzer trace -- /opt/buildtools/jam/bin.linuxx86/jam \
  -q -j1 @nightly-anyboot
pvs-studio-analyzer analyze -l /mnt/svn/PVS-Studio.lic -r /opt/haiku \
   -C x86_64-unknown-haiku-gcc -o /opt/haiku/haiku.log -j12
```

Кстати, анализ проекта выполнялся в Docker\-контейнере\. Недавно мы подготовили новую документацию по этой теме: [Запуск PVS\-Studio в Docker](https://pvs-studio.ru/ru/docs/manual/0047/)\. Это может сильно упростить некоторым компаниям применение методики статического анализа проектов\.

## Неинициализированные переменные

[V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized variable 'rval' used\. fetch\.c 1727

```cpp
int
auto_fetch(int argc, char *argv[])
{
  volatile int  argpos;
  int    rval;                  // <=
  argpos = 0;

  if (sigsetjmp(toplevel, 1)) {
    if (connected)
      disconnect(0, NULL);
    if (rval > 0)               // <=
      rval = argpos + 1;
    return (rval);
  }
  ....
}
```

Переменная _rval_ не была инициализирована при объявлении, поэтому её сравнение с нулевым значением приведёт к неопределённому результату\. При неудачном стечении обстоятельств неопределённое значение переменной _rval_ может стать возвращаемым значением функции _auto\_fetch_\.

[V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized pointer 'res' used\. commands\.c 2873

```cpp
struct addrinfo {
 int ai_flags;
 int ai_family;
 int ai_socktype;
 int ai_protocol;
 socklen_t ai_addrlen;
 char *ai_canonname;
 struct sockaddr *ai_addr;
 struct addrinfo *ai_next;
};

static int
sourceroute(struct addrinfo *ai, char *arg, char **cpp,
            int *lenp, int *protop, int *optp)
{
  static char buf[1024 + ALIGNBYTES];
  char *cp, *cp2, *lsrp, *ep;
  struct sockaddr_in *_sin;
#ifdef INET6
  struct sockaddr_in6 *sin6;
  struct ip6_rthdr *rth;
#endif
  struct addrinfo hints, *res;     // <=
  int error;
  char c;

  if (cpp == NULL || lenp == NULL)
    return -1;
  if (*cpp != NULL) {
    switch (res->ai_family) {      // <=
    case AF_INET:
      if (*lenp < 7)
        return -1;
      break;
      ....
    }
  }
  ....
}
```

Похожий случай использования неинициализированной переменной, только здесь имеет место неинициализированный указатель _res_\.

[V506](https://pvs-studio.ru/ru/docs/warnings/v506/) Pointer to local variable 'normalized' is stored outside the scope of this variable\. Such a pointer will become invalid\. TextView\.cpp 5596

```cpp
void
BTextView::_ApplyStyleRange(...., const BFont* font, ....)
{
  if (font != NULL) {
    BFont normalized = *font;
    _NormalizeFont(&normalized);
    font = &normalized;
  }
  ....
  fStyles->SetStyleRange(fromOffset, toOffset, fText->Length(), mode,
    font, color);
}
```

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

[V603](https://pvs-studio.ru/ru/docs/warnings/v603/) The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 27

```cpp
int8
BUnicodeChar::Type(uint32 c)
{
  BUnicodeChar();
  return u_charType(c);
}
```

Очень распространённая ошибка среди C\+\+ программистов \- использовать вызов конструктора якобы для инициализации/обнуления полей класса\. В этом случае не происходит модификации полей класса, а создаётся новый неименованный объект этого класса, который тут же уничтожается\. К сожалению, таких мест найдено очень много:

* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 37
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 49
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 58
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 67
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 77
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 89
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 103
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 115
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 126
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 142
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 152
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 163
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 186
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 196
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 206
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 214
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 222
* V603 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>BUnicodeChar::BUnicodeChar\(\.\.\.\.\)' should be used\. UnicodeChar\.cpp 230

[V670](https://pvs-studio.ru/ru/docs/warnings/v670/) The uninitialized class member 'fPatternHandler' is used to initialize the 'fInternal' member\. Remember that members are initialized in the order of their declarations inside a class\. Painter\.cpp 184

```cpp
Painter::Painter()
  :
  fInternal(fPatternHandler),
  ....
  fPatternHandler(),
  ....
{
  ....
};

class Painter {
  ....
private:
  mutable PainterAggInterface fInternal; // line 336

  bool fSubpixelPrecise : 1;
  bool fValidClipping : 1;
  bool fDrawingText : 1;
  bool fAttached : 1;
  bool fIdentityTransform : 1;

  Transformable fTransform;
  float fPenSize;
  const BRegion* fClippingRegion;
  drawing_mode fDrawingMode;
  source_alpha fAlphaSrcMode;
  alpha_function fAlphaFncMode;
  cap_mode fLineCapMode;
  join_mode fLineJoinMode;
  float fMiterLimit;

  PatternHandler fPatternHandler;        // line 355
  mutable AGGTextRenderer fTextRenderer;
};
```

Другой пример неправильной инициализации\. Поля класса инициализируются в порядке их объявления в самом классе\. В этом примере первым будет инициализировано поле _fInternal_ с использованием неинициализированного значения _fPatternHandler_\.

## Подозрительный \#define

[V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. subr\_gtaskqueue\.c 191

```cpp
#define  TQ_LOCK(tq)              \
  do {                \
    if ((tq)->tq_spin)          \
      mtx_lock_spin(&(tq)->tq_mutex);      \
    else              \
      mtx_lock(&(tq)->tq_mutex);      \
  } while (0)
#define  TQ_ASSERT_LOCKED(tq)  mtx_assert(&(tq)->tq_mutex, MA_OWNED)

#define  TQ_UNLOCK(tq)              \
  do {                \
    if ((tq)->tq_spin)          \
      mtx_unlock_spin(&(tq)->tq_mutex);    \
    else              \
      mtx_unlock(&(tq)->tq_mutex);      \
  } while (0)

void
grouptask_block(struct grouptask *grouptask)
{
  ....
  TQ_LOCK(queue);
  gtask->ta_flags |= TASK_NOENQUEUE;
  gtaskqueue_drain_locked(queue, gtask);
  TQ_UNLOCK(queue);
}
```

Фрагмент кода не выглядит подозрительным, пока не взглянешь на результат работы препроцессора:

```cpp
void
grouptask_block(struct grouptask *grouptask)
{
  ....
  do { if ((queue)->tq_spin) mtx_lock(&(queue)->tq_mutex);
       else mtx_lock(&(queue)->tq_mutex); } while (0);
  gtask->ta_flags |= 0x4;
  gtaskqueue_drain_locked(queue, gtask);
   do { if ((queue)->tq_spin) mtx_unlock(&(queue)->tq_mutex);
        else mtx_unlock(&(queue)->tq_mutex); } while (0);
}
```

Анализатор действительно прав \- ветви _if_ и _else_ идентичны\. Но куда делись функции _mtx\_lock\_spin_ и _mtx\_unlock\_spin_? Макросы _TQ\_LOCK_, _TQ\_UNLOCK_ и функция _grouptask\_block_ объявлены в одном файле и практически рядом, тем не менее, где\-то произошла подмена\.

Поиск по содержимому файлов нашёл только _mutex\.h_ со следующим содержимым:

```cpp
/* on FreeBSD these are different functions */
#define mtx_lock_spin(x)   mtx_lock(x)
#define mtx_unlock_spin(x) mtx_unlock(x)
```

Правильная ли такая подмена или нет \- стоит проверить разработчикам проекта\. Я проверял проект в Linux, и такая подмена показалась мне подозрительной\.

## Ошибки с функцией free

[V575](https://pvs-studio.ru/ru/docs/warnings/v575/) The null pointer is passed into 'free' function\. Inspect the first argument\. setmime\.cpp 727

```cpp
void
MimeType::_PurgeProperties()
{
  fShort.Truncate(0);
  fLong.Truncate(0);
  fPrefApp.Truncate(0);
  fPrefAppSig.Truncate(0);
  fSniffRule.Truncate(0);

  delete fSmallIcon;
  fSmallIcon = NULL;

  delete fBigIcon;
  fBigIcon = NULL;

  fVectorIcon = NULL;            // <=
  free(fVectorIcon);             // <=

  fExtensions.clear();
  fAttributes.clear();
}
```

В функцию _free_ передавать нулевой указатель можно, но такая запись явно вызывает подозрение\. Так, анализатор нашёл перепутанные строки кода\. Сначала надо было освободить память по указателю _fVectorIcon_, а только потом присвоить _NULL_\.

[V575](https://pvs-studio.ru/ru/docs/warnings/v575/) The null pointer is passed into 'free' function\. Inspect the first argument\. driver\_settings\.cpp 461

```cpp
static settings_handle *
load_driver_settings_from_file(int file, const char *driverName)
{
  ....
  handle = new_settings(text, driverName);
  if (handle != NULL) {
    // everything went fine!
    return handle;
  }

  free(handle);           // <=
  ....
}
```

Ещё один пример явной передачи нулевого указателя в функцию _free_\. Эту строку можно удалить, т\.к\. при успешном получении указателя выполняется выход из функции\.

[V575](https://pvs-studio.ru/ru/docs/warnings/v575/) The null pointer is passed into 'free' function\. Inspect the first argument\. PackageFileHeapWriter\.cpp 166

```cpp
void* _GetBuffer()
{
  ....
  void* buffer = malloc(fBufferSize);
  if (buffer == NULL && !fBuffers.AddItem(buffer)) {
    free(buffer);
    throw std::bad_alloc();
  }
  return buffer;
}
```

Вот тут допущена ошибка\. Вместо оператора && должен использоваться оператор \|\|\. Только в этом случае будет брошено исключение _std::bad\_alloc\(\)_, если не удалось выделить память с помощью функции _malloc_\.

## Ошибки с оператором delete

[V611](https://pvs-studio.ru/ru/docs/warnings/v611/) The memory was allocated using 'new T\[\]' operator but was released using the 'delete' operator\. Consider inspecting this code\. It's probably better to use 'delete \[\] fMsg;'\. Err\.cpp 65

```cpp
class Err {
public:
 ....
private:
 char *fMsg;
 ssize_t fPos;
};

void
Err::Unset() {
 delete fMsg;                                   // <=
 fMsg = __null;
 fPos = -1;
}

void
Err::SetMsg(const char *msg) {
 if (fMsg) {
  delete fMsg;                                  // <=
  fMsg = __null;
 }
 if (msg) {
  fMsg = new(std::nothrow) char[strlen(msg)+1]; // <=
  if (fMsg)
   strcpy(fMsg, msg);
 }
}
```

Указатель _fMsg_ используется для аллокации памяти под хранение массива символов, при этом для освобождения памяти используется оператор _delete_, вместо _delete\[\]_\.

[V611](https://pvs-studio.ru/ru/docs/warnings/v611/) The memory was allocated using 'new' operator but was released using the 'free' function\. Consider inspecting operation logics behind the 'wrapperPool' variable\. vm\_page\.cpp 3080

```cpp
status_t
vm_page_write_modified_page_range(....)
{
  ....
  PageWriteWrapper* wrapperPool
    = new(malloc_flags(allocationFlags)) PageWriteWrapper[maxPages + 1];
  PageWriteWrapper** wrappers
    = new(malloc_flags(allocationFlags)) PageWriteWrapper*[maxPages];
  if (wrapperPool == NULL || wrappers == NULL) {
    free(wrapperPool);                              // <=
    free(wrappers);                                 // <=
    wrapperPool = stackWrappersPool;
    wrappers = stackWrappers;
    maxPages = 1;
  }
  ....
}
```

Здесь _malloc\_flags_ – функция, которая делает _malloc_\. И затем уже _placement\-new_ конструирует здесь объект\. А так как класс _PageWriteWrapper_ реализован следующим образом:

```cpp
class PageWriteWrapper {
public:
 PageWriteWrapper();
 ~PageWriteWrapper();
 void SetTo(vm_page* page);
 bool Done(status_t result);

private:
 vm_page* fPage;
 struct VMCache* fCache;
 bool fIsActive;
};

PageWriteWrapper::PageWriteWrapper()
 :
 fIsActive(false)
{
}

PageWriteWrapper::~PageWriteWrapper()
{
 if (fIsActive)
  panic("page write wrapper going out of scope but isn't completed");
}
```

то деструкторы объектов этого класса не будут вызваны из\-за использования функции _free_ для освобождения памяти\.

[V611](https://pvs-studio.ru/ru/docs/warnings/v611/) The memory was allocated using 'new T\[\]' operator but was released using the 'delete' operator\. Consider inspecting this code\. It's probably better to use 'delete \[\] fOutBuffer;'\. Check lines: 26, 45\. PCL6Rasterizer\.h 26

```cpp
class PCL6Rasterizer : public Rasterizer
{
public:
  ....
  ~PCL6Rasterizer()
  {
    delete fOutBuffer;
    fOutBuffer = NULL;
  }
  ....
  virtual void InitializeBuffer()
  {
    fOutBuffer = new uchar[fOutBufferSize];
  }
private:
  uchar* fOutBuffer;
  int    fOutBufferSize;
};
```

Использование оператора _delete_ вместо _delete\[\]_ \- очень распространённая ошибка\. Легче всего допустить ошибку при написании класса, т\.к\. код деструктора часто находится далеко от мест аллокации памяти\. Здесь программист неправильно освобождает память в деструкторе по указателю _fOutBuffer_\.

[V772](https://pvs-studio.ru/ru/docs/warnings/v772/) Calling a 'delete' operator for a void pointer will cause undefined behavior\. Hashtable\.cpp 207

```cpp
void
Hashtable::MakeEmpty(int8 keyMode,int8 valueMode)
{
  ....
  for (entry = fTable[index]; entry; entry = next) {
    switch (keyMode) {
      case HASH_EMPTY_DELETE:
        // TODO: destructors are not called!
        delete (void*)entry->key;
        break;
      case HASH_EMPTY_FREE:
        free((void*)entry->key);
        break;
    }
    switch (valueMode) {
      case HASH_EMPTY_DELETE:
        // TODO: destructors are not called!
        delete entry->value;
        break;
      case HASH_EMPTY_FREE:
        free(entry->value);
        break;
    }
    next = entry->next;
    delete entry;
  }
  ....
}
```

Кроме неправильного выбора между _delete_/_delete\[\]_ и _free_, можно добавить в программу неопределённое поведение ещё одним способом \- попытаться очистить память по указателю на неопределённый тип _\(void\*\)_\.

## Функции без возвращаемого значения

[V591](https://pvs-studio.ru/ru/docs/warnings/v591/) Non\-void function should return a value\. Referenceable\.h 228

```cpp
BReference& operator=(const BReference<const Type>& other)
{
  fReference = other.fReference;
}
```

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

Похожие проблемы в других фрагментах кода этого класса:

* V591 Non\-void function should return a value\. Referenceable\.h 233
* V591 Non\-void function should return a value\. Referenceable\.h 239

[V591](https://pvs-studio.ru/ru/docs/warnings/v591/) Non\-void function should return a value\. main\.c 1010

```cpp
void errx(int, const char *, ...) ;

char *
getoptionvalue(const char *name)
{
  struct option *c;

  if (name == NULL)
    errx(1, "getoptionvalue() invoked with NULL name");
  c = getoption(name);
  if (c != NULL)
    return (c->value);
  errx(1, "getoptionvalue() invoked with unknown option '%s'", name);
  /* NOTREACHED */
}
```

Пользовательский комментарий NOTREACHED здесь ничего не значит\. Чтобы правильно написать код для таких сценариев, необходимо размечать функции как noreturn\. Для этого существуют noreturn\-атрибуты: стандартные и специфичные для компилятора\. В первую очередь, такие атрибуты учитываются компиляторами для правильной кодогенерации или уведомления о некоторых типах ошибок с помощью варнингов\. Различные инструменты статического анализа тоже учитывают атрибуты для повышения качества анализа\.

## Работа с исключениями

[V596](https://pvs-studio.ru/ru/docs/warnings/v596/) The object was created but it is not being used\. The 'throw' keyword could be missing: throw ParseException\(FOO\); Response\.cpp 659

```cpp
size_t
Response::ExtractNumber(BDataIO& stream)
{
  BString string = ExtractString(stream);

  const char* end;
  size_t number = strtoul(string.String(), (char**)&end, 10);
  if (end == NULL || end[0] != '\0')
    ParseException("Invalid number!");

  return number;
}
```

Здесь случайно забыто ключевое слово _throw_\. Таким образом, исключение _ParseException_ не будет сгенерировано, а объект этого класса будет просто уничтожен при выходе из области видимости\. После чего функция продолжит свою работу как ни в чём не бывало, как будто введено корректное число\.

[V1022](https://pvs-studio.ru/ru/docs/warnings/v1022/) An exception was thrown by pointer\. Consider throwing it by value instead\. gensyscallinfos\.cpp 316

```cpp
int
main(int argc, char** argv)
{
  try {
    return Main().Run(argc, argv);
  } catch (Exception& exception) {                                         // <=
    fprintf(stderr, "%s\n", exception.what());
    return 1;
  }
}

int Run(int argc, char** argv)
{
  ....
  _ParseSyscalls(argv[1]);
  ....
}

void _ParseSyscalls(const char* filename)
{
  ifstream file(filename, ifstream::in);
  if (!file.is_open())
    throw new IOException(string("Failed to open '") + filename + "'.");   // <=
  ....
}
```

Анализатор обнаружил исключение _IOException_, брошенное по указателю\. Бросание указателя приводит к тому, что исключение не будет поймано, так исключение в итоге перехватывается по ссылке\. Также использование указателя вынуждает перехватывающую сторону вызвать оператор _delete_ для уничтожения созданного объекта, что тоже не сделано\.

Ещё два проблемных участка кода:

* V1022 An exception was thrown by pointer\. Consider throwing it by value instead\. gensyscallinfos\.cpp 347
* V1022 An exception was thrown by pointer\. Consider throwing it by value instead\. gensyscallinfos\.cpp 413

## Формальная безопасноcть

[V597](https://pvs-studio.ru/ru/docs/warnings/v597/) The compiler could delete the 'memset' function call, which is used to flush 'f\_key' object\. The memset\_s\(\) function should be used to erase the private data\. dst\_api\.c 1018

```cpp
#ifndef SAFE_FREE
#define SAFE_FREE(a) \
do{if(a != NULL){memset(a,0, sizeof(*a)); free(a); a=NULL;}} while (0)
....
#endif

DST_KEY *
dst_free_key(DST_KEY *f_key)
{
  if (f_key == NULL)
    return (f_key);
  if (f_key->dk_func && f_key->dk_func->destroy)
    f_key->dk_KEY_struct =
      f_key->dk_func->destroy(f_key->dk_KEY_struct);
  else {
    EREPORT(("dst_free_key(): Unknown key alg %d\n",
       f_key->dk_alg));
  }
  if (f_key->dk_KEY_struct) {
    free(f_key->dk_KEY_struct);
    f_key->dk_KEY_struct = NULL;
  }
  if (f_key->dk_key_name)
    SAFE_FREE(f_key->dk_key_name);
  SAFE_FREE(f_key);
  return (NULL);
}
```

Анализатор обнаружил подозрительный код, предназначенный для безопасной очистки приватных данных\. К сожалению, макрос _SAFE\_FREE_, который раскрывается в вызовы _memset_, _free_ и присваивание _NULL_, не делает код безопаснее, т\.к\. всё это удаляется компилятором уже при оптимизации _O2_\.

Кстати, это не что иное, как [CWE\-14](https://cwe.mitre.org/data/definitions/14.html): Compiler Removal of Code to Clear Buffers\.

Весь список мест, где очистка буферов по факту не выполняется:

* V597 The compiler could delete the 'memset' function call, which is used to flush 'encoded\_block' buffer\. The memset\_s\(\) function should be used to erase the private data\. dst\_api\.c 446
* V597 The compiler could delete the 'memset' function call, which is used to flush 'key\_st' object\. The memset\_s\(\) function should be used to erase the private data\. dst\_api\.c 685
* V597 The compiler could delete the 'memset' function call, which is used to flush 'in\_buff' buffer\. The memset\_s\(\) function should be used to erase the private data\. dst\_api\.c 916
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ce' object\. The memset\_s\(\) function should be used to erase the private data\. fs\_cache\.c 1078

## Сравнения с беззнаковыми переменными

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'remaining < 0' is always false\. Unsigned type value is never < 0\. DwarfFile\.cpp 1947

```cpp
status_t
DwarfFile::_UnwindCallFrame(....)
{
  ....
  uint64 remaining = lengthOffset + length - dataReader.Offset();
  if (remaining < 0)
    return B_BAD_DATA;
  ....
}
```

Анализатор обнаружил явное сравнение беззнаковой переменной с отрицательными значениями\. Возможно, следует сравнить переменную _remaining_ только с нулём или реализовать проверку на переполнение\.

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'sleep\(\(unsigned\) secs\) < 0' is always false\. Unsigned type value is never < 0\. misc\.cpp 56

```cpp
status_t
snooze(bigtime_t amount)
{
  if (amount <= 0)
    return B_OK;

  int64 secs = amount / 1000000LL;
  int64 usecs = amount % 1000000LL;
  if (secs > 0) {
    if (sleep((unsigned)secs) < 0)     // <=
      return errno;
  }

  if (usecs > 0) {
    if (usleep((useconds_t)usecs) < 0)
      return errno;
  }

  return B_OK;
}
```

Чтобы понять, в чём ошибка, обратимся к сигнатурам функций _sleep_ и _usleep_:

```cpp
extern unsigned int sleep (unsigned int __seconds);
extern int usleep (__useconds_t __useconds);
```

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

## Опасные указатели

[V774](https://pvs-studio.ru/ru/docs/warnings/v774/) The 'device' pointer was used after the memory was released\. xhci\.cpp 1572

```cpp
void
XHCI::FreeDevice(Device *device)
{
  uint8 slot = fPortSlots[device->HubPort()];
  TRACE("FreeDevice() port %d slot %d\n", device->HubPort(), slot);

  // Delete the device first, so it cleans up its pipes and tells us
  // what we need to destroy before we tear down our internal state.
  delete device;

  DisableSlot(slot);
  fDcba->baseAddress[slot] = 0;
  fPortSlots[device->HubPort()] = 0;            // <=
  delete_area(fDevices[slot].trb_area);
  delete_area(fDevices[slot].input_ctx_area);
  delete_area(fDevices[slot].device_ctx_area);

  memset(&fDevices[slot], 0, sizeof(xhci_device));
  fDevices[slot].state = XHCI_STATE_DISABLED;
}
```

Некий объект _device_ очищается оператором _delete_\. Это логичное действие для функции с названием _FreeDevice_\. Но, по какой\-то причине для освобождения других ресурсов в коде снова есть обращение к уже удалённому объекту\.

Такой код является крайне опасным и встречается ещё в нескольких местах:

* V774 The 'self' pointer was used after the memory was released\. TranslatorRoster\.cpp 884
* V774 The 'string' pointer was used after the memory was released\. RemoteView\.cpp 1269
* V774 The 'bs' pointer was used after the memory was released\. mkntfs\.c 4291
* V774 The 'bs' pointer was used after the memory was released\. mkntfs\.c 4308
* V774 The 'al' pointer was used after the memory was reallocated\. inode\.c 1155

[V522](https://pvs-studio.ru/ru/docs/warnings/v522/) Dereferencing of the null pointer 'data' might take place\. The null pointer is passed into 'malo\_hal\_send\_helper' function\. Inspect the third argument\. Check lines: 350, 394\. if\_malohal\.c 350

```cpp
static int
malo_hal_fwload_helper(struct malo_hal *mh, char *helper)
{
  ....
  /* tell the card we're done and... */
  error = malo_hal_send_helper(mh, 0, NULL, 0, MALO_NOWAIT); // <= NULL
  ....
}

static int
malo_hal_send_helper(struct malo_hal *mh, int bsize,
    const void *data, size_t dsize, int waitfor)
{
  mh->mh_cmdbuf[0] = htole16(MALO_HOSTCMD_CODE_DNLD);
  mh->mh_cmdbuf[1] = htole16(bsize);
  memcpy(&mh->mh_cmdbuf[4], data , dsize);                   // <= data
  ....
}
```

Межпроцедурный анализ позволил выявить ситуацию, когда в функцию передаётся _NULL_ и указатель _data_ с таким значением впоследствии разыменовывается в функции _memcpy_\.

[V773](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without releasing the 'inputFileFile' pointer\. A memory leak is possible\. command\_recompress\.cpp 119

```cpp
int
command_recompress(int argc, const char* const* argv)
{
  ....
  BFile* inputFileFile = new BFile;
  error = inputFileFile->SetTo(inputPackageFileName, O_RDONLY);
  if (error != B_OK) {
    fprintf(stderr, "Error: Failed to open input file \"%s\": %s\n",
      inputPackageFileName, strerror(error));
    return 1;
  }
  inputFile = inputFileFile;
  ....
}
```

[PVS\-Studio умеет выявлять утечки памяти](https://pvs-studio.ru/ru/blog/posts/cpp/0543/)\. Здесь не освобождается память для _inputFileFile_ в случае какой\-то ошибки\. Кто\-то считает, что в случае ошибок можно не заморачиваться с освобождением памяти \- программа всё равно завершится\. Но это не всегда так\. Корректно обрабатывать ошибки и продолжать работу \- требование к очень многим программам\.

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'fReply' pointer was utilized before it was verified against nullptr\. Check lines: 49, 52\. ReplyBuilder\.cpp 49

```cpp
RPC::CallbackReply*
ReplyBuilder::Reply()
{
  fReply->Stream().InsertUInt(fStatusPosition, _HaikuErrorToNFS4(fStatus));
  fReply->Stream().InsertUInt(fOpCountPosition, fOpCount);

  if (fReply == NULL || fReply->Stream().Error() == B_OK)
    return fReply;
  else
    return NULL;
}
```

Как же часто разработчики разыменовывают указатели перед их проверкой\. Диагностика [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) практически всегда лидер по количеству предупреждений в проекте\. В этом фрагменте кода опасное использование указателя _fReply_\.

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'mq' pointer was utilized before it was verified against nullptr\. Check lines: 782, 786\. oce\_queue\.c 782

```cpp
static void
oce_mq_free(struct oce_mq *mq)
{
  POCE_SOFTC sc = (POCE_SOFTC) mq->parent;
  struct oce_mbx mbx;
  struct mbx_destroy_common_mq *fwcmd;

  if (!mq)
    return;
  ....
}
```

Похожий пример\. Указатель _mg_ разыменовывается на несколько строк ранее, чем проверяется на нулевое значение\. Похожих мест в проекте много\. В некоторых местах использование и проверка указателей значительно удалены друг от друга, поэтому в статью вошло два примера\. Остальные смогут посмотреть разработчики в полном отчёте анализатора\.

## Разные ошибки

[V645](https://pvs-studio.ru/ru/docs/warnings/v645/) The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 101

```cpp
static void
dump_acpi_namespace(acpi_ns_device_info *device, char *root, int indenting)
{
  char output[320];
  char tabs[255] = "";
  ....
  strlcat(tabs, "|--- ", sizeof(tabs));
  ....
  while (....) {
    uint32 type = device->acpi->get_object_type(result);
    snprintf(output, sizeof(output), "%s%s", tabs, result + depth);
    switch(type) {
      case ACPI_TYPE_INTEGER:
        strncat(output, "     INTEGER", sizeof(output));
        break;
      case ACPI_TYPE_STRING:
        strncat(output, "     STRING", sizeof(output));
        break;
      ....
    }
    ....
  }
  ....
}
```

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

Весь список опасных мест со строками:

* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 104
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 107
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 110
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 113
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 118
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 119
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 120
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 123
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 126
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 129
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 132
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 135
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 138
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 141
* V645 The 'strncat' function call could lead to the 'output' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. NamespaceDump\.cpp 144
* V645 The 'strncat' function call could lead to the 'features\_string' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. VirtioDevice\.cpp 283
* V645 The 'strncat' function call could lead to the 'features\_string' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. VirtioDevice\.cpp 284
* V645 The 'strncat' function call could lead to the 'features\_string' buffer overflow\. The bounds should not contain the size of the buffer, but a number of characters it can hold\. VirtioDevice\.cpp 285

[V792](https://pvs-studio.ru/ru/docs/warnings/v792/) The 'SetDecoratorSettings' function located to the right of the operator '\|' will be called regardless of the value of the left operand\. Perhaps, it is better to use '\|\|'\. DesktopListener\.cpp 324

```cpp
class DesktopListener : public DoublyLinkedListLinkImpl<DesktopListener> {
public:
 ....
 virtual bool SetDecoratorSettings(Window* window,
         const BMessage& settings) = 0;
 ....
};

bool
DesktopObservable::SetDecoratorSettings(Window* window,
  const BMessage& settings)
{
  if (fWeAreInvoking)
    return false;
  InvokeGuard invokeGuard(fWeAreInvoking);

  bool changed = false;
  for (DesktopListener* listener = fDesktopListenerList.First();
    listener != NULL; listener = fDesktopListenerList.GetNext(listener))
    changed = changed | listener->SetDecoratorSettings(window, settings);

  return changed;
}
```

Скорее всего, в коде перепутали операторы '\|' и '\|\|'\. Такая ошибка приводит к лишним вызовам функции _SetDecoratorSettings_\.

[V627](https://pvs-studio.ru/ru/docs/warnings/v627/) Consider inspecting the expression\. The argument of sizeof\(\) is the macro which expands to a number\. device\.c 72

```cpp
#define PCI_line_size 0x0c /* (1 byte) cache line size in 32 bit words */

static status_t
wb840_open(const char* name, uint32 flags, void** cookie)
{
  ....
  data->wb_cachesize = gPci->read_pci_config(data->pciInfo->bus,
    data->pciInfo->device, data->pciInfo->function, PCI_line_size,
    sizeof(PCI_line_size)) & 0xff;
  ....
}
```

Передача в оператор _sizeof_ значения _0x0c_ выглядит подозрительно\. Возможно, следовало посчитать размер какого\-нибудь объекта, например, _data_\.

[V562](https://pvs-studio.ru/ru/docs/warnings/v562/) It's odd to compare a bool type value with a value of 18: 0x12 \=\= IsProfessionalSpdif\(\)\. CEchoGals\_mixer\.cpp 533

```cpp
typedef bool BOOL;

virtual BOOL IsProfessionalSpdif() { ... }

#define ECHOSTATUS_DSP_DEAD 0x12

ECHOSTATUS CEchoGals::ProcessMixerFunction(....)
{
  ....
  if ( ECHOSTATUS_DSP_DEAD == IsProfessionalSpdif() ) // <=
  {
    Status = ECHOSTATUS_DSP_DEAD;
  }
  else
  {
    pMixerFunction->Data.bProfSpdif = IsProfessionalSpdif();
  }
  ....
}
```

Функция _IsProfessionalSpdif_ возвращает значение типа _bool_, при этом в условии результат функции сравнивается с числом _0x12_\.

## Заключение

Мы пропустили выпуск первой беты Haiku прошлой осенью, т\.к\. были заняты выпуском PVS\-Studio для языка Java\. Но природа программистских ошибок такова, что они никуда не деваются, если их не искать и вообще не уделять внимание качеству кода\. Разработчики пробовали использовать Coverity Scan, но последний запуск анализа был почти два года назад\. Это должно огорчать пользователей Haiku\. Хотя анализ с помощью Coverity был настроен в 2014 году, нам это не помешало в 2015 году написать две больших статьи с обзорами ошибок \([часть 1](https://pvs-studio.ru/ru/blog/posts/cpp/0317/), [часть 2](https://pvs-studio.ru/ru/blog/posts/cpp/0318/)\)\.

Тех, кто дочитал до конца, ждёт ещё один обзор кода Haiku не меньшего объёма с новыми типами ошибок\. Полный отчёт анализатора будет отправлен разработчикам до публикации обзоров кода, поэтому некоторые ошибки могут быть исправлены\. Чтобы скоротать время между публикациями, предлагаю скачать и попробовать [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/) на своём проекте\.

Хотите попробовать Haiku и у вас возникли вопросы? Разработчики Haiku приглашают вас в [telegram\-канал](https://t.me/HaikuOS_RU_chat)\.