Проверка Firebird 3.0

• Введение
• Опечатки
• Опасное использование memcmp
• Лишние проверки
• Опасное сравнение беззнаковой переменной
• Разыменовывание нулевых указателей
• Проверка на nullptr после new
• Опасное использование realloc
• Неиспользованные значения enum в switch
• Переполнение буфера
• Сдвиг отрицательных чисел
• Переопределение переменной
• Заключение

Не так давно вышла новая версия СУБД Firebird. Релиз стал одним из масштабных в истории проекта: была сильно переработана архитектура, добавлена поддержка многопоточности, улучшена производительность. Такое значительное обновление и послужило поводом для повторной проверки Firebird с помощью статического анализатора кода PVS-Studio.

Введение

Firebird - это кроссплатформенная свободная система управлениями базами данных. Проект написан на C++ и работает на Microsoft Windows, Linux, macOS X и многих Unix-like операционных системах. СУБД полностью бесплатна для использования и распространения. Подробнее о Firebird можно узнать на официальном сайте.

Ранее Firebird уже проверялся анализатором. С отчётом о предыдущей проверке можно ознакомиться в статье "Побочный результат: проверяем Firebird с помощью PVS-Studio". Для проверки был взят код с GitHub из ветви master. Сборка подробно описана в соответствующей статье на сайте проекта. Анализ исходных файлов производился в PVS-Studio Standalone версии 6.03 с помощью перехвата вызова компиляторов (Compiler Monitoring). Данная технология позволяет проверять проекты без интеграции в сборочную систему. Полученный отчёт можно просмотреть как в Standalone версии, так и в Visual Studio.

Опечатки

void advance_to_start()
{
  ....
  if (!isalpha(c) && c != '_' && c != '.' && c != '_')
    syntax_error(lineno, line, cptr);
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'c != '_'' to the left and to the right of the '&&' operator. reader.c 1203

Анализатор обнаружил, что в логической операции присутствуют два одинаковых подвыражения c != '_'. В последнем условии допущена опечатка и переменная c должна сравниваться с другим символом. В других функциях используется проверка с символом '$', возможно здесь нужно использовать его:

if (!isalpha(c) && c != '_' && c != '.' && c != '$')

Ещё один пример ошибки от невнимательности:

int put_message(....)
{
  if (newlen <= MAX_UCHAR)
    {
    put(tdgbl, attribute);
    put(tdgbl, (UCHAR) newlen);
  }
  else if (newlen <= MAX_USHORT)
  {
    if (!attribute2)
      BURP_error(314, "");
    ....
  }
  else
    BURP_error(315, "");
  ....
}

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

• V601 The string literal is implicitly cast to the bool type. Inspect the second argument. backup.cpp 6113
• V601 The string literal is implicitly cast to the bool type. Inspect the second argument. backup.cpp 6120

Здесь неверно используется функция BURP_error. Она объявлена следующим образом:

void BURP_error(USHORT errcode, bool abort,
     const MsgFormat::SafeArg& arg = MsgFormat::SafeArg());

void BURP_error(USHORT errcode, bool abort, const char* str);

Второй аргумент имеет логический тип, а строка стоит третьим аргументом. В результате строковый литерал приводится к true. Вызов функции должен быть переписан как BURP_error(315, true, "") или BURP_error(315, false, "").

Однако, есть случаи, когда распознать ошибку может только разработчик проекта.

void IDX_create_index(....)
{
  ....
  index_fast_load ifl_data;
  ....
  if (!ifl_data.ifl_duplicates)
    scb->sort(tdbb);

  if (!ifl_data.ifl_duplicates)
    BTR_create(tdbb, creation, selectivity);

  ....
}

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

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

В следующем примере рассмотрим ситуацию, связанную с указателями.

static void string_to_datetime(....)
{
  ....

  const char* p = NULL;
  const char* const end = p + length;

  ....

  while (p < end)
  {
    if (*p != ' ' && *p != '\t' && p != 0)
    {
      CVT_conversion_error(desc, err);
      return;
    }
    ++p;
  }

  ....
}

Предупреждение PVS-Studio: V713 The pointer p was utilized in the logical expression before it was verified against nullptr in the same logical expression. cvt.cpp 702

В условии переменная p проверяется на nullptr сразу же после разыменовывания. Это может означать, что на месте проверки должно было быть другое условие либо то, что проверка лишняя.

Если посмотреть выше по коду, то можно найти аналогичный фрагмент:

while (++p < end)
{
  if (*p != ' ' && *p != '\t' && *p != 0)
    CVT_conversion_error(desc, err);
}

Для того, чтобы избежать подобной ошибки, для сравнения с нулём лучше использовать соответствующие литералы: '\0' для типа char, 0 для чисел и nullptr для указателей. Если взять это за правило, можно избежать множества подобных глупых ошибок.

Опасное использование memcmp

SSHORT TextType::compare(ULONG len1, const UCHAR* str1,
                         ULONG len2, const UCHAR* str2)
{
  ....

  SSHORT cmp = memcmp(str1, str2, MIN(len1, len2));

  if (cmp == 0)
    cmp = (len1 < len2 ? -1 : (len1 > len2 ? 1 : 0));

  return cmp;
}

Предупреждение PVS-Studio: V642 Saving the 'memcmp' function result inside the 'short' type variable is inappropriate. The significant bits could be lost breaking the program's logic. texttype.cpp 3

Функция memcmp возвращает следующие значения:

• < 0, если str1 меньше str2
• 0, если str1 равен str2
• > 0, если str1 больше str2

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

Лишние проверки

void Trigger::compile(thread_db* tdbb)
{
  SET_TDBB(tdbb);

  Database* dbb = tdbb->getDatabase();
  Jrd::Attachment* const att = tdbb->getAttachment();

  if (extTrigger)
    return;

  if (!statement /*&& !compile_in_progress*/)
  {
    if (statement)
      return;

    ....
  }
}

Предупреждение PVS-Studio: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 778, 780. jrd.cpp 778

Анализатор нашёл проверку двух противоположных условий. Скорее всего, второе условие здесь стало не нужным в результате изменения первого и его можно удалить, но решение тут стоит принимать автору.

Ещё одним примером странного ветвления может послужить следующий фрагмент кода.

static void asgn_from( ref* reference, int column)
{
  TEXT variable[MAX_REF_SIZE];
  TEXT temp[MAX_REF_SIZE];

  for (; reference; reference = reference->ref_next)
  {
    const gpre_fld* field = reference->ref_field;
    ....

    if (!field || field->fld_dtype == dtype_text)
      ....
    else if (!field || field->fld_dtype == dtype_cstring)
      ....
    else
      ....
  }
}

Предупреждение PVS-Studio: V560 A part of conditional expression is always false: !field. int_cxx.cpp 217

Если field не нулевой указатель, то код никогда не достигнет условия в else if. Либо данная проверка лишняя, либо на её месте должно было быть другое сравнение. Неизвестно, противоречит ли данное условие логике приложения.

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

bool XnetServerEndPoint::server_init(USHORT flag)
{
  ....

  xnet_connect_mutex = CreateMutex(ISC_get_security_desc(),
                          FALSE, name_buffer);
  if (!xnet_connect_mutex ||
          (xnet_connect_mutex && ERRNO == ERROR_ALREADY_EXISTS))
  {
    system_error::raise(ERR_STR("CreateMutex"));
  }

  ....
}

Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!xnet_connect_mutex' and 'xnet_connect_mutex'. xnet.cpp 2231

Проверку if (!xnet_connect_mutex || (xnet_connect_mutex && ERRNO == ERROR_ALREADY_EXISTS)) можно упростить до if (!xnet_connect_mutex || ERRNO == ERROR_ALREADY_EXISTS). Это можно легко доказать с помощью таблицы истинности.

Опасное сравнение беззнаковой переменной

static bool write_page(thread_db* tdbb, BufferDesc* bdb, ....)
{
  ....
  if (bdb->bdb_page.getPageNum() >= 0)
  ....
}

Предупреждение PVS-Studio: V547 Expression 'bdb->bdb_page.getPageNum() >= 0' is always true. Unsigned type value is always >= 0. cch.cpp 4827

Условие bdb->bdb_page.getPageNum() >= 0 всегда окажется верным, так как функция возвращает значение беззнакового типа. Возможно, программист неверно проверил значение. Принимая во внимания аналогичные сравнения в проекте, можно предположить, что код должен выглядеть следующим образом:

if (bdb->bdb_page.getPageNum() != 0)

Разыменовывание нулевых указателей

static bool initializeFastMutex(FAST_MUTEX* lpMutex, 
  LPSECURITY_ATTRIBUTES lpAttributes, BOOL bInitialState,
  LPCSTR lpName)
{
  if (pid == 0)
    pid = GetCurrentProcessId();
  
  LPCSTR name = lpName;

  if (strlen(lpName) + strlen(FAST_MUTEX_EVT_NAME) - 2
                                                   >= MAXPATHLEN)
  {
    SetLastError(ERROR_FILENAME_EXCED_RANGE);
    return false;
  }

  setupMutex(lpMutex);

  char sz[MAXPATHLEN]; 
  if (lpName)
  ....
}

Предупреждение PVS-Studio: V595 The 'lpName' pointer was utilized before it was verified against nullptr. Check lines: 2814, 2824. isc_sync.cpp 2814

Предупреждение V595 является самым часто встречаемым в проектах, проверенных PVS-Studio, и Firebird не стал исключением. Всего обнаружено более 30 мест с данным предупреждением.

В этом примере вызов strlen(lpName) идёт перед проверкой указателя на nullptr. Это приведёт к неопределённому поведению при попытке передать в функцию нулевой указатель. Разыменовывание указателя здесь спрятано в вызове strlen, что затрудняет обнаружение ошибки, если не пользоваться статическим анализатором.

Проверка на nullptr после new

rem_port* XnetServerEndPoint::get_server_port(....)
{
  ....
  XCC xcc = FB_NEW struct xcc(this);

  try {

    ....
  }
  catch (const Exception&)
  {
    if (port)
      cleanup_port(port);
    else if (xcc)
      cleanup_comm(xcc);

    throw;
  }

  return port;
}

Предупреждение PVS-Studio: V668 There is no sense in testing the 'xcc' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. xnet.cpp 2533

Анализатор предупреждает, что оператор new не может вернуть nullptr - для проверки необходимо использовать блок try-catch или new (std::nothrow). Однако, в данном примере всё несколько сложнее. Для выделения памяти используется макрос FB_NEW. Он объявлен в файле alloc.h:

#ifdef USE_SYSTEM_NEW
#define OOM_EXCEPTION std::bad_alloc
#else
#define OOM_EXCEPTION Firebird::BadAlloc
#endif

#define FB_NEW new(__FILE__, __LINE__)

inline void* operator new(size_t s ALLOC_PARAMS)
throw (OOM_EXCEPTION)
{
  return MemoryPool::globalAlloc(s ALLOC_PASS_ARGS);
}

Сложно сказать, является ли этот конкретный пример ошибкой или нет, так как используется нестандартный аллокатор. Но из-за того, что в определении оператора указан throw (std::bad_alloc), подобная проверка выглядит подозрительно.

Опасное использование realloc

int mputchar(struct mstring *s, int ch)
{
  if (!s || !s->base) return ch;
  if (s->ptr == s->end) {
    int len = s->end - s->base;
    if ((s->base = realloc(s->base, len+len+TAIL))) {
      s->ptr = s->base + len;
      s->end = s->base + len+len+TAIL; }
    else {
      s->ptr = s->end = 0;
      return ch; } }
  *s->ptr++ = ch;
  return ch;
}

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 's->base' is lost. Consider assigning realloc() to a temporary pointer. mstring.c 42

Выражение вида ptr = realloc(ptr, size) плохо тем, что в случае, когда realloc вернёт nullptr, указатель на память будет потерян. Чтобы этого избежать, следует сохранить результат realloc в временную переменную и после проверки на nullptr присвоить ptr её значение.

temp_ptr = realloc(ptr, new_size);
if (temp_ptr == nullptr) {
  //handle exception
} else {
  ptr = temp_ptr;
}

Неиспользованные значения enum в switch

template <typename CharType>
LikeEvaluator<CharType>::LikeEvaluator(....)
{
  ....
  PatternItem *item = patternItems.begin();
  ....
  switch (item->type)
  {
  case piSkipFixed:
  case piSkipMore:
    patternItems.grow(patternItems.getCount() + 1);
    item = patternItems.end() - 1;
    // Note: fall into
    case piNone:
      item->type = piEscapedString;
      item->str.data = const_cast<CharType*>
                        (pattern_str + pattern_pos - 2);
      item->str.length = 1;
      break;
    case piSearch:
      item->type = piEscapedString;
      // Note: fall into
    case piEscapedString:
      item->str.length++;
      break;
  }
  ....
}

Предупреждение PVS-Studio: V719 The switch statement does not cover all values of the 'PatternItemType' enum: piDirectMatch. evl_string.h 324

В конструкции switch были использованы не все значения enum, и отсутствует блок default. Возможно, что здесь забыли добавить обработку piDirectMatch. Список мест с аналогичным предупреждением:

• V719 The switch statement does not cover all values of the 'PatternItemType' enum: piDirectMatch, piSkipMore. evl_string.h 351
• V719 The switch statement does not cover all values of the 'PatternItemType' enum: piDirectMatch. evl_string.h 368
• V719 The switch statement does not cover all values of the 'PatternItemType' enum: piDirectMatch. evl_string.h 387

Переполнение буфера

const int GDS_NAME_LEN = 32;
....
bool get_function(BurpGlobals* tdgbl)
{
  ....
  struct isc_844_struct {
    ....
    short isc_870; /* gds__null_flag */
    ....
    char  isc_874 [125]; /* RDB$PACKAGE_NAME */
    ....
  } isc_844;
 
  att_type attribute;
  TEXT    temp[GDS_NAME_LEN * 2];
  ....
  SSHORT prefixLen = 0;
  if (!/*X.RDB$PACKAGE_NAME.NULL*/
       isc_844.isc_870)
  {
    prefixLen = static_cast<SSHORT>(strlen(/*X.RDB$PACKAGE_NAME*/
                                           isc_844.isc_874));
    memcpy(temp, /*X.RDB$PACKAGE_NAME*/
                 isc_844.isc_874, prefixLen);
    temp[prefixLen++] = '.';
  }
  ....

}

Предупреждение PVS-Studio: V557 Array overrun is possible. The value of 'prefixLen ++' index could reach 124. restore.cpp 10040

Размер буфера isc_844.isc_874 равен 125, соответственно, максимальное возможное значение strlen(isc_844.isc_874) - 124. Размер temp - 64, что меньше этого значения. Запись по такому индексу может привести к переполнению буфера. Безопасней будет выделить больший объём памяти для переменной temp.

Сдвиг отрицательных чисел

static ISC_STATUS stuff_literal(gen_t* gen, SLONG literal)
{
  ....

  if (literal >= -32768 && literal <= 32767)
    return stuff_args(gen, 3, isc_sdl_short_integer, literal, 
                      literal >> 8);

  ....

}

Предупреждение PVS-Studio: V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('literal' = [-32768..32767]). array.cpp 848

Код содержит сдвиг вправо отрицательного числа. В стандарте C++ указано, что такое действие имеет неуточняемое поведение, значит на разных компиляторах и платформах может выдавать разный результат. Лучше переписать его так:

if (literal >= -32768 && literal <= 32767)
  return stuff_args(gen, 3, isc_sdl_short_integer, literal, 
                    (ULONG)literal >> 8);

Ещё одно место, где сработало предупреждение:

V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('i64value' = [-2147483648..2147483647]). exprnodes.cpp 6382

Переопределение переменной

THREAD_ENTRY_DECLARE Service::run(THREAD_ENTRY_PARAM arg)
{
  int exit_code = -1;
  try
  {
    Service* svc = (Service*)arg;
    RefPtr<SvcMutex> ref(svc->svc_existence);
    int exit_code = svc->svc_service_run->serv_thd(svc);

    svc->started();
    svc->svc_sem_full.release();
    svc->finish(SVC_finished);
  }
  catch (const Exception& ex)
  {
    // Not much we can do here
    iscLogException("Exception in Service::run():", ex);
  }

  return (THREAD_ENTRY_RETURN)(IPTR) exit_code;
}

Предупреждение PVS-Studio: V561 It's probably better to assign value to 'exit_code' variable than to declare it anew. Previous declaration: svc.cpp, line 1893. svc.cpp 1898

В этом примере вместо присвоения переменная exit_code переопределяется. Это скрывает из области видимости предыдущую переменную, и в итоге функция возвращает неверное значение, всегда равное -1.

Корректный код:

THREAD_ENTRY_DECLARE Service::run(THREAD_ENTRY_PARAM arg)
{
  int exit_code = -1;
  try
  {
    Service* svc = (Service*)arg;
    RefPtr<SvcMutex> ref(svc->svc_existence);
    exit_code = svc->svc_service_run->serv_thd(svc);

    svc->started();
    svc->svc_sem_full.release();
    svc->finish(SVC_finished);
  }
  catch (const Exception& ex)
  {
    // Not much we can do here
    iscLogException("Exception in Service::run():", ex);
  }

  return (THREAD_ENTRY_RETURN)(IPTR) exit_code;
}

Заключение

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