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

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

Введение

Герои нашей истории — операционная система Haiku с открытым исходным кодом и статический анализатор PVS-Studio для языков C, C++, C# и Java. Когда 4.5 года назад мы взялись за анализ проекта, то пришлось работать только со скомпилированным исполняемым файлом анализатора. Вся инфраструктура для парсинга параметров компиляции, запуска препроцессора, распараллеливания анализа и т.д. была взята из утилиты Compiler Monitoring UI на языке C#, которая частями была портирована на платформу Mono, чтобы запускаться в Linux. Сам проект Haiku собирается с помощью кросс-компилятора под разными операционными системами, кроме Windows. Очередной раз хочу отметить удобство и полноту документации по сборке Haiku, а также поблагодарить разработчиков Haiku за помощь в сборке проекта.

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

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. Это может сильно упростить некоторым компаниям применение методики статического анализа проектов.

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

V614 Uninitialized variable 'rval' used. fetch.c 1727

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 Uninitialized pointer 'res' used. commands.c 2873

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 Pointer to local variable 'normalized' is stored outside the scope of this variable. Such a pointer will become invalid. TextView.cpp 5596

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 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

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 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

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 The 'then' statement is equivalent to the 'else' statement. subr_gtaskqueue.c 191

#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);
}

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

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 со следующим содержимым:

/* 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 The null pointer is passed into 'free' function. Inspect the first argument. setmime.cpp 727

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 The null pointer is passed into 'free' function. Inspect the first argument. driver_settings.cpp 461

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 The null pointer is passed into 'free' function. Inspect the first argument. PackageFileHeapWriter.cpp 166

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 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

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 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

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 реализован следующим образом:

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 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

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 Calling a 'delete' operator for a void pointer will cause undefined behavior. Hashtable.cpp 207

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 Non-void function should return a value. Referenceable.h 228

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 Non-void function should return a value. main.c 1010

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 The object was created but it is not being used. The 'throw' keyword could be missing: throw ParseException(FOO); Response.cpp 659

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 An exception was thrown by pointer. Consider throwing it by value instead. gensyscallinfos.cpp 316

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 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

#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: 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 Expression 'remaining < 0' is always false. Unsigned type value is never < 0. DwarfFile.cpp 1947

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

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

V547 Expression 'sleep((unsigned) secs) < 0' is always false. Unsigned type value is never < 0. misc.cpp 56

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:

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

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

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

V774 The 'device' pointer was used after the memory was released. xhci.cpp 1572

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 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

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 The function was exited without releasing the 'inputFileFile' pointer. A memory leak is possible. command_recompress.cpp 119

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 умеет выявлять утечки памяти. Здесь не освобождается память для inputFileFile в случае какой-то ошибки. Кто-то считает, что в случае ошибок можно не заморачиваться с освобождением памяти — программа всё равно завершится. Но это не всегда так. Корректно обрабатывать ошибки и продолжать работу — требование к очень многим программам.

V595 The 'fReply' pointer was utilized before it was verified against nullptr. Check lines: 49, 52. ReplyBuilder.cpp 49

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 практически всегда лидер по количеству предупреждений в проекте. В этом фрагменте кода опасное использование указателя fReply.

V595 The 'mq' pointer was utilized before it was verified against nullptr. Check lines: 782, 786. oce_queue.c 782

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 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

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 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

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 Consider inspecting the expression. The argument of sizeof() is the macro which expands to a number. device.c 72

#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 It's odd to compare a bool type value with a value of 18: 0x12 == IsProfessionalSpdif(). CEchoGals_mixer.cpp 533

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, часть 2).

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

Хотите попробовать Haiku и у вас возникли вопросы? Разработчики Haiku приглашают вас в telegram-канал.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. How to shoot yourself in the foot in C and C++. Haiku OS Cookbook