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

Haiku — свободная операционная система для персональных компьютеров, которая нацелена на двоичную совместимость с операционной системой BeOS. Haiku воплощает в себе основные идеи BeOS. Это модульная система, архитектурно решённая как гибридное ядро: микроядерная архитектура, способная динамически подгружать необходимые модули.

Проект проверялся по просьбе сообщества пользователей Haiku с помощью PVS-Studio 5.24.

Работа со строками

V527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *scratchPtr = '\0'. TextGapBuffer.cpp 228

const char*
TextGapBuffer::Text()
{
  const char* realText = RealText();

  if (fPasswordMode) {
    ....

    char* scratchPtr = fScratchBuffer;
    for (uint32 i = 0; i < numChars; i++) {
      memcpy(scratchPtr, B_UTF8_BULLET, bulletCharLen);
      scratchPtr += bulletCharLen;
    }
    scratchPtr = '\0';      //<==

    return fScratchBuffer;
  }

  return realText;
}

Скорее всего, после работы со строкой хотели записать терминальный ноль в конец строки, а не обнулить указатель. Правильный вариант: "*scratchPtr = '\0';".

V692 An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. PoorManWindow.cpp 254

void
PoorManWindow::MessageReceived(BMessage* message)
{
  ....
  if (inet_ntop(AF_INET, &sin_addr, addr, sizeof(addr)) != NULL){
    addr[strlen(addr)] = '\0';  //<==
    line << '(' << addr << ") ";
  }
  ....
}

Чтобы записать терминальный ноль в конец строки, тут воспользовались функций strlen(), но результат такого действия непредсказуем, ведь для работы функции strlen() строка уже должна заканчиваться терминальным нулём. Ноль будет записан как раз в ту ячейку, где был найден 0. При этом, функция strlen() может выйти далеко за пределы буфера, что приведет к неопределенному поведению программы. Чтобы исправить код, надо вычислить длину строки каким-то другим способом.

Плохие циклы

V529 Odd semicolon ';' after 'for' operator. ringqueue.cpp 39

int
compute_order(unsigned long size)
{
  int  order;
  unsigned long tmp;
  for (order = 0, tmp = size; tmp >>= 1; ++order); //<==
    if (size & ~(1 << order))
      ++order;
    return order;
}

Что-то не так с этой функцией: цикл без тела из-за точки с запятой в конце; форматирование кода показывает, что условие должно быть в теле цикла. С другой стороны, переменная 'tmp' всё равно нигде использоваться не будет.

Может хотели так сделать:

int
compute_order(unsigned long size)
{
  int  order;
  unsigned long tmp;
  for (order = 0, tmp = size; tmp >>= 1; ++order)
    if (tmp & ~(1 << order))
      ++order;
  return order;
}

Но изменять счётчик цикла for(;;) в теле не очень хороший стиль.

V535 The variable 'k' is being used for this loop and for the outer loop. Check lines: 3598, 3610. rules.c 3610

void
solver_get_unneeded(Solver *solv, Queue *unneededq, int filtered)
{
  ....
  if (dep_possible(solv, *dp, &installedm))
  {
    Queue iq;
    Id iqbuf[16];
    queue_init_buffer(&iq, iqbuf, sizeof(iqbuf)/sizeof(*iqbuf));
    dep_pkgcheck(solv, *dp, 0, &iq);
    for (k = 0; k < iq.count; k++)            //<==
      {
  Id p = iq.elements[k];
  Solvable *sp = pool->solvables + p;
  if (....)
    continue;
  for (j = 0; j < count; j++)
    if (p == unneededq->elements[j])
      break;
  /* now add edge from j + 1 to i + 1 */
  queue_insert(....);
  /* addapt following edge pointers */
  for (k = j + 2; k < count + 2; k++)         //<==
    edges.elements[k]++;
      }
    queue_free(&iq);
  }
  ....
}

Код так плохо отформатирован, что если тут есть ошибка, то она точно была допущена из-за этого. Плохой стиль использовать один счётчик во вложенных циклах for(;;).

Аналогичное место:

• V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2319, 2349. solver.c 2349

V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. RAW.cpp 1141

void
DCRaw::_WaveletDenoise()
{
  ....
  for (i = 0; i < (1 << dim * 2); i++) {  //<==
    if (fimg[i] < -fThreshold)
      fimg[i] += fThreshold;
    else if (fimg[i] > fThreshold)
      fimg[i] -= fThreshold;
    else
      fimg[i] = 0;
  }
  ....
}

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

Похожее место:

• V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. RAW.cpp 1099

V696 The 'continue' operator will terminate 'do {… } while (FALSE)' loop because the condition is always false. Check lines: 1939, 1945. Roster.cpp 1939

status_t
BRoster::_LaunchApp(....) const
{
  ....
  do {
    // find the app
    ....
    if (appType.InitCheck() == B_OK
      && appType.GetAppHint(&hintRef) == B_OK
      && appRef == hintRef) {
      appType.SetAppHint(NULL);
      // try again
      continue;
    }
    ...
  } while (false);
  ....
}

Оператор 'continue' в цикле «do {… } while(… )» выполняет переход к вычислению условия остановки цикла, а так оно всегда ложно — по сути это безусловный выход из цикла и комментарий «try again», получается, только вводит в заблуждение.

V706 Suspicious division: sizeof (kBaudrates) / sizeof (char *). Size of every element in 'kBaudrates' array does not equal to divisor. SerialWindow.cpp 162

const int SerialWindow::kBaudrates[] = { 50, 75, 110, .... };

SerialWindow::SerialWindow() : ....
{
  ....
  for(int i = sizeof(kBaudrates) / sizeof(char*); --i >= 0;)//<==
  {
    message = new BMessage(kMsgSettings);
    message->AddInt32("baudrate", kBaudrateConstants[i]);

    char buffer[7];
    sprintf(buffer, "%d", kBaudrates[i]);                   //<==
    BMenuItem* item = new BMenuItem(buffer, message);

    fBaudrateMenu->AddItem(item);
  }
  ....
}

Чтобы получить количество элементов в массиве 'kBaudrates', почему-то делят его размер на размер указателя, в итоге получится, что в 32-х битной системе будет индексироваться весь массив, а в 64-х битной системе — только половина.

Массивы

V548 Consider reviewing type casting. TYPE X[][] in not equivalent to TYPE **X. RAW.cpp 1668

void
DCRaw::_AdobeCoefficients(const char *make, const char *model)
{
  static const struct {
    const char *prefix;
    short black, trans[12];
  } table[] = {
    { "Canon EOS D2000", 0,
      { 24542,-10860,-3401,-1490,11370,-297,2858,-605,3225 }},
    { "Canon EOS D6000", 0,
      { 20482,-7172,-3125,-1033,10410,-285,2542,226,3136 }},
    ....
  };
  double cameraXYZ[4][3];

  for (uint32 i = 0; i < sizeof table / sizeof *table; i++) {
    if (!strncasecmp(model, table[i].prefix, strlen(....))) {
      if (table[i].black)
        fMeta.black = table[i].black;
      for (uint32 j = 0; j < 12; j++) {
        ((double**)cameraXYZ)[0][j] = table[i].trans[j] /10000.0;
      }
      _CameraXYZCoefficients(cameraXYZ);
      break;
    }
  }
}

Массив 'cameraXYZ', объявленный как «double cameraXYZ[4][3]», приводится к типу «double **». Это приведение типа, скорее всего, не имеет смысла и может повлечь ошибки.

Типы «type[a][b]» и «type **» представляют собой разные структуры данных. Type[a][b] это единый участок памяти с которым можно работать как с двумерным массивом. Type ** — это массив указателей на какие-то участки памяти.

V554 Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. DefaultCatalog.cpp 208

status_t
DefaultCatalog::ReadFromFile(const char *path)
{
  ....
  auto_ptr<char> buf(new(std::nothrow) char [sz]);
  ....
}

Анализатор обнаружил ситуацию, когда использование умного указателя может привести к неопределенному поведению. Класс 'auto_ptr' не предназначен для работы с массивами, он использует для освобождения памяти оператор 'delete', а если указать 'delete[]', то код не скомпилируется.

Правильный пример кода:

status_t
DefaultCatalog::ReadFromFile(const char *path)
{
  ....
  unique_ptr<char[]> buf(new(std::nothrow) char[sz]);
  ....
}

Ещё такое место:

• V554 Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. DefaultCatalog.cpp 249

V557 Array overrun is possible. The '8' index is pointing beyond array bound. floppy_ctrl.c 637
V557 Array overrun is possible. The '9' index is pointing beyond array bound. floppy_ctrl.c 638

typedef struct floppy {
  ....
  uint8 result[8]; /* status of the last finished command */
  ....
};

void
floppy_dump_reg(floppy_t *flp) {
  ....
  //uint8 result[10];           //<== This was correct!
  uint8 *result = flp->result;  //<== Bad fix! :)
  ....
  dprintf(FLO "gap=%d wg=%d eis=%d fifo=%d poll=%d thresh=%d pretrk=%d\n", 
    (result[7] & 0x02) >> 1, result[7] & 0x01,
    (result[8] & 0x40) >> 6, 
    (result[8] & 0x20) >> 5, (result[8] & 0x10) >> 4,
     result[8] & 0x0f, result[9]);
  ....
}

Два предупреждения анализатора указывают на выход за границу массива. Если судить по закомментированному коду, раньше массив 'result[]' состоял из 10 элементов, а после правки стал длиной 8 элементов. При этом в коде по-прежнему осуществляется доступ к десяти элементам массива с индексами от 0 до 9.

Имена переменных

V672 There is probably no need in creating the new 'path' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 348, 429. translate.cpp 429

status_t
Translator::FindPath(const translation_format *format,
  BPositionIO &stream, TypeList &typesSeen, TypeList &path, ....)
{
  ....
  TypeList path;
  double quality;
  if (FindPath(...) == B_OK) {
    if (bestQuality < quality * formatQuality) {
      bestQuality = quality * formatQuality;
      bestPath.SetTo(path);
      bestPath.Add(formats[j].type);
      status = B_OK;
    }
  }
  ....
}

Совпадение имени локальной переменной 'path' с параметром функции (тем более с ссылкой, как в данном случае) может приводить к потере локальных изменений в этой переменной и другим логических ошибкам.

V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. ipv4.cpp 514

static int
dump_ipv4_multicast(int argc, char** argv)
{
  MulticastState::Iterator it = sMulticastState->GetIterator();

  while (it.HasNext()) {
    ....
    int count = 0;
    IPv4GroupInterface::AddressSet::Iterator it
      = state->Sources().GetIterator();
    while (it.HasNext()) {
      ....
    }

    kprintf("}> sock %p\n", state->Parent()->Socket());
  }

  return 0;
}

В теле цикла обнаружено объявление переменной 'it', совпадающей с переменной, используемой для контроля цикла. Такой код может содержать логические ошибки, вплоть до получения вечного цикла.

Работа с памятью

V597 The compiler could delete the 'memset' function call, which is used to flush 'password' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. login.cpp 126

static status_t
login(const char* user, struct passwd** _passwd)
{
  ....
  bool ok = verify_password(passwd, spwd, password);
  memset(password, 0, sizeof(password));
  
  if (!ok)
    return B_PERMISSION_DENIED;

  *_passwd = passwd;
  return B_OK;
}

К сожалению, приведенный код может оставить пароль неочищенным. Обратите внимание, что массив 'password' очищается в конце и более не используется. Поэтому при сборке Release версии программы, компилятор, скорее всего, удалит вызов функции memset(). На это компилятор имеет полное право. Анализатор предлагает использовать функцию для Windows, но для этой операционной системы следует найти другой способ избежать оптимизации компилятора.

Ещё опасные места:

• V597 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha1.c 228
• V597 The compiler could delete the 'memset' function call, which is used to flush 'encoded_block' buffer. The RtlSecureZeroMemory() 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 'in_buff' buffer. The RtlSecureZeroMemory() 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 'repeatedPassword' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. passwd.cpp 171

V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors. PDFWriter.cpp 117

status_t
PDFWriter::PrintPage(int32  pageNumber, int32 pageCount)
{
  ....
  pictures =
    (BPicture **)malloc(pictureCount * sizeof(BPicture *));
  picRects =
    (BRect *)malloc(pictureCount * sizeof(BRect));    //<==
  picPoints =
    (BPoint *)malloc(pictureCount * sizeof(BPoint));  //<==
  picRegion = new BRegion();
  ....
}

При выделении памяти с помощью malloc под массив объектов какого-нибудь класса, не вызывается ни конструктор объекта при создании, ни деструктор при уничтожении. Подобный код может привести к работе с неинициализированными переменными и другим ошибкам.

V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 623

#define MEMSET_BZERO(p,l)  memset((p), 0, (l))

void solv_SHA256_Final(sha2_byte digest[], SHA256_CTX* context) {
  ....
  /* Clean up state data: */
  MEMSET_BZERO(context, sizeof(context));
  usedspace = 0;
}

Размер обнуляемой памяти равен не размеру структуры, а размеру указателя.

Ещё такие места:

• V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 644
• V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 953
• V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 973
• V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 1028
• V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 1048

Прочее

V591 Non-void function should return a value. pc.c 1031

ULONG
set_var(char *name, ULONG val)
{
  variable *v;

  v = lookup_var(name);
  if (v != NULL)
    v->value = val;
  else
    add_var(name, val);
}

Скорее всего, при вызове функции set_var(), возвращаемое значение никак не используется. Но если когда-нибудь им воспользуются, то получат неопределённое значение.

V671 It is possible that the 'swap' function interchanges the 'std::declval < _Alloc & > ()' variable with itself. alloc_traits.h 191

static constexpr bool _S_nothrow_swap()
{
  using std::swap;
  return !_S_propagate_on_swap()
    || noexcept(
         swap(std::declval<_Alloc&>(), std::declval<_Alloc&>()));
}

Непонятное использование функции swap(): одинаковые аргументы.

V519 The 'data->error' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 222, 223. repo_solv.c 223

static unsigned char *
data_read_idarray(.... , Repodata *data)
{
  ....
  data->error = pool_error(            //<==
    data->repo->pool, SOLV_ERROR_ID_RANGE,
    "data_read_idarray: id too large (%u/%u)", x, max);
  data->error = SOLV_ERROR_ID_RANGE;   //<==
  ....
}

Присваивание разных значений одной переменной подряд. Возможно, опечатка.

V568 It's odd that the argument of sizeof() operator is the 'sizeof (struct tlv_header_t)' expression. print-slow.c 255

void
slow_print(register const u_char *pptr, register u_int len) {
  ....
  if (vflag > 1)
    print_unknown_data(tptr+sizeof(sizeof(struct tlv_header_t)),
      "\n\t  ", tlv_len-sizeof(struct tlv_header_t));
  ....
}

Аргументом оператора sizeof() тоже является sizeof(). Этот оператор вычисляет тип выражения и возвращает размер этого типа, но само выражение не вычисляется, т.е. размер структуры здесь ни на что не влияет.

Таких мест много:

• V568 It's odd that the argument of sizeof() operator is the 'sizeof (struct lmp_object_header)' expression. print-lmp.c 872
• V568 It's odd that the argument of sizeof() operator is the 'sizeof (struct tlv_header_t)' expression. print-slow.c 182
• V568 It's odd that the argument of sizeof() operator is the 'sizeof (struct eigrp_tlv_header)' expression. print-eigrp.c 283
• V568 It's odd that the argument of sizeof() operator is the 'sizeof (struct eigrp_tlv_header)' expression. print-eigrp.c 471

Заключение

Haiku — большой и необычный проект. Было интересно его проверить и сделать маленький вклад в развитие. Несмотря на мой опыт работы с open-source проектами, тут я встретил много редких предупреждений анализатора. В статьи вошли самые подозрительные, на мой взгляд, примеры исходного кода. Всё остальное, что не вошло в статью или было мною пропущено, авторы проекта смогут найти в полном логе проверки.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analysis of Haiku Operating System (BeOS Family) by PVS-Studio. Part 2.