Парсер Хабра: Алло! Это FreeSWITCH? Тогда мы проверим вас

воскресенье, 11 октября 2015 г.

Алло! Это FreeSWITCH? Тогда мы проверим вас

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

Введение

FreeSWITCH — открытая телефонная платформа, распространяемая в исходных кодах, созданная для удовлетворения потребности в управляемых голосом или текстом системах, масштабируемых от софтфонa до софтсвичa. FreeSWITCH может быть использован в качестве коммутатора, АТС, медиа шлюза или медиа сервера для приложений IVR, использующих простые или XML скрипты для управления алгоритмом обработки звонка.

С помощью анализатора PVS-Studio 5.29 проект FreeSWITCH был с лёгкостью проверен в Visual Studio 2015.

If (bug) then find_copy_paste();

V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. switch_channel.c 493

typedef enum {
  SWITCH_STATUS_SUCCESS,
  SWITCH_STATUS_FALSE,
  SWITCH_STATUS_TIMEOUT,
  SWITCH_STATUS_RESTART,
  ....
} switch_status_t;


SWITCH_DECLARE(switch_status_t) switch_channel_queue_dtmf(....)
{
  ....
  switch_status_t status;
  ....
  if ((status = switch_core_session_recv_dtmf(channel->session,
                  dtmf) != SWITCH_STATUS_SUCCESS)) {
    goto done;
  }
  ....
}

Источником логических ошибок в программе может являться неправильно написанное условие. Например, в этом фрагменте кода приоритет оператора сравнения выше приоритета оператора присваивания. Таким образом, в 'status' сохраняется результат логической операции, а не результат функции switch_core_session_recv_dtmf(). В коде присутствуют оператор перехода goto, поэтому испорченное значение переменной 'status' в дальнейшем может использоваться где угодно.

К сожалению, таких мест много:

V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. switch_core_db.c 208
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. switch_core_db.c 211
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. switch_core_db.c 214
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. switch_core_db.c 217
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. switch_event.c 2986
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. switch_ivr.c 3905
V593 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. fsodbc.cpp 285
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. mod_db.c 653

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 141, 168. mod_easyroute.c 141

static switch_status_t load_config(void)
{
  ....
  if (globals.db_dsn) {                                     //<==
    ....
  } else if (globals.db_dsn) {                              //<==
    switch_log_printf(SWITCH_CHANNEL_LOG, SWITCH_LOG_CRIT,
      "Cannot Open ODBC Connection (did you enable it?!)\n");
  }
  ....
}

В каскаде условий проверяется одна и та же переменная «globals.db_dsn », поэтому сообщение о неудачном подключении к базе данных не будет залогировано.

V523 The 'then' statement is equivalent to the 'else' statement. sofia_glue.c 552

char *sofia_overcome_sip_uri_weakness(....)
{
  ....
  if (strchr(stripped, ';')) {
    if (params) {
      new_uri = switch_core_session_sprintf(session, "....",
        uri_only ? "" : "<", stripped, sofia_glue_transport2str(
        transport), params, uri_only ? "" : ">");
    } else {
      new_uri = switch_core_session_sprintf(session, "....",
        uri_only ? "" : "<", stripped, sofia_glue_transport2str(
        transport), uri_only ? "" : ">");
    }
  } else {
    if (params) {
      new_uri = switch_core_session_sprintf(session, "....",
        uri_only ? "" : "<", stripped, sofia_glue_transport2str(
        transport), params, uri_only ? "" : ">");
    } else {
      new_uri = switch_core_session_sprintf(session, "....",
        uri_only ? "" : "<", stripped, sofia_glue_transport2str(
        transport), uri_only ? "" : ">");
    }
  }
  ....
}

Большой фрагмент кода, содержащий много идентичного текста. В случае отсутствия ошибки этот фрагмент кода можно сократить в два раза, иначе здесь очередной забытый copy-paste.

V590 Consider inspecting the '* data == ' ' && * data != '\0'' expression. The expression is excessive or contains a misprint. mod_curl.c 306

static char *print_json(switch_memory_pool_t *pool, ....)
{
  ....
  while (*data == ' ' && *data != '\0') {
    data++;
  }
  ....
}

Здесь ошибки нет, но выражение избыточно, что может затруднять чтение кода. Проверка " *data != '\0' " не имеет смысла. Сокращенный вариант кода:

while (*data == ' ') {
  data++;

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. conference_api.c 1532

switch_status_t conference_api_sub_vid_logo_img(....)
{
  ....
  if (!strcasecmp(text, "allclear")) {
    switch_channel_set_variable(member->channel, "....", NULL);
    member->video_logo = NULL;
  } if (!strcasecmp(text, "clear")) {                       //<==
    member->video_logo = NULL;
  } else {
    member->video_logo = switch_core_strdup(member->pool, text);
  }
  ....
}

По коду видно, что планировалось написать «else if'. Но видимо случайно пропустили ключевое слово 'else' и это привело к изменению логики программы.

Что-бы понять суть ошибки давайте рассмотрим упрощенный вариант кода. В начале правильный вариант:

if (A == 1) {
  X();
} else if (A == 2) {
  Y();
} else {
  Z();
}

В зависти от значения переменной A будет вызвана одна из функций: X, Y или Z. Посмотрим, что будет если забыть 'else':

if (A == 1) {
  X();
} if (A == 2) {
  Y();
} else {
  Z();
}

Теперь если A равно единице, то вызовется не только функция X, но ещё и Z!

Использование типа SOCKET

V605 Consider verifying the expression: context->curlfd > — 1. An unsigned value is compared to the number -1. mod_shout.c 151

typedef SOCKET curl_socket_t;
curl_socket_t curlfd;

static inline void free_context(shout_context_t *context)
{
  ....
  if (context->curlfd > -1) {
    shutdown(context->curlfd, 2);
    context->curlfd = -1;
  }
  ....
}

Тип SOCKET является беззнаковым типом, поэтому сравнение с отрицательным числом недопустимо. В данном случае необходимо выполнять сравнение со специальными именованными константами для работы с типом SOCKET, например, SOCKET_ERROR и т.д.

V547 Expression is always false. Unsigned type value is never < 0. esl.c 690

typedef SOCKET ws_socket_t;

static ws_socket_t prepare_socket(ips_t *ips) 
{
  ws_socket_t sock = ws_sock_invalid;
  
  ....
  if ((sock = socket(family, SOCK_STREAM, IPPROTO_TCP)) < 0) {
    die("Socket Error!\n");
  }
  ....
}

Похожий пример неправильной работы с переменными типа SOCKET. Это беззнаковый тип, а для проверки статуса ошибки существуют специально определённые константы, например, SOCKET_ERROR.

Двойные присваивания

V570 The variable is assigned to itself. skypopen_protocol.c 1512

struct SkypopenHandles {
  HWND win32_hInit_MainWindowHandle;
  HWND win32_hGlobal_SkypeAPIWindowHandle;
  ....
};

LRESULT APIENTRY skypopen_present(...., WPARAM uiParam, ....)
{
 ....
 if (!tech_pvt->SkypopenHandles.currentuserhandle) {
   tech_pvt->SkypopenHandles.api_connected = 1;
   tech_pvt->SkypopenHandles.win32_hGlobal_SkypeAPIWindowHandle =
    (HWND) uiParam;
   tech_pvt->SkypopenHandles.win32_hGlobal_SkypeAPIWindowHandle =
    tech_pvt->SkypopenHandles.win32_hGlobal_SkypeAPIWindowHandle;
 }
 ....
}

Анализатор обнаружил, что переменная присваивается сама себе. Можно предположить, что во время написания кода, во втором присваивании было случайно выбрано не то поле структуры: „win32_hGlobal_SkypeAPIWindowHandle“ вместо „win32_hInit_MainWindowHandle“.

Возможно, код функции должен быть таким:

if (!tech_pvt->SkypopenHandles.currentuserhandle) {
  tech_pvt->SkypopenHandles.api_connected = 1;
  tech_pvt->SkypopenHandles.win32_hGlobal_SkypeAPIWindowHandle =
   (HWND) uiParam;
  tech_pvt->SkypopenHandles. win32_hInit_MainWindowHandle =
   tech_pvt->SkypopenHandles.win32_hGlobal_SkypeAPIWindowHandle;
}

V519 The 'status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 365, 368. fscoredb.cpp 368

JS_COREDB_FUNCTION_IMPL(BindInt)
{
  bool status;
  ....
  /* convert args */
  status = !info[0].IsEmpty() && info[0]->IsInt32() ? true:false;
  param_index = info[0]->Int32Value();

  status = !info[1].IsEmpty() && info[1]->IsInt32() ? true:false;
  param_value = info[1]->Int32Value();

  if (param_index < 1) {
    info.GetIsolate()->ThrowException(....);
    return;
  }
  ....
}

Анализатор обнаружил потенциально возможную ошибку, связанную с тем, что одной и той же переменной дважды подряд присваивается значение. Причем между этими присваиваниями сама переменная не используется. С помощью анализатора удалось найти пропущенную проверку: значение переменной 'status' нигде не используется.

Возможно, код должен быть таким:

....
param_index = status ? info[0]->Int32Value() : 0;
....
param_value = status ? info[1]->Int32Value() : 0;

V519 The 'status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1239, 1240. switch_core_io.c 1240

SWITCH_DECLARE(switch_status_t)
switch_core_session_write_frame(...., int stream_id)
{
  ....
  if (ptime_mismatch && status != SWITCH_STATUS_GENERR) {
    status = perform_write(session, frame, flags, stream_id);
    status = SWITCH_STATUS_SUCCESS;
    goto error;
  }
  ....
}

Почему здесь статус записи просто перезаписывают на успешный не понятно. Оставим изучение этого фрагмента авторам кода.

Ошибки в строках

V694 The condition (mode + 5) is only false if there is pointer overflow which is undefined behaviour anyway. mod_ilbc.c 51

static switch_status_t switch_ilbc_fmtp_parse(....)
{
  ....
  if (fmtp && (mode = strstr(fmtp, "mode=")) && (mode + 5)) {
      codec_ms = atoi(mode + 5);
    }
    if (!codec_ms) {
      /* default to 30 when no mode is defined for ilbc ONLY */
      codec_ms = 30;
    }
  ....
}

На первый взгляд здесь реализован простой алгоритм:

Найти подстроку „mode=“;
Убедиться, что после подстроки не стоит нулевой символ;
Конвертировать следующий символ в число.

Ошибка кроется в этапе N2: убедившись, что указатель 'mode' на подстроку ненулевой, в коде смещают указатель на 5 символов, но он так и останется ненулевым. В операции (mode + 5) пропущено разыменование смещённого указателя. Из-за такой ошибки стали возможны ситуации, когда символ завершения строки конвертируют в число и получают значение ноль. Благодаря проверке „if (!codec_ms) { codec_ms = 30;}“ нулевое значение всегда сбрасывается на значение по умолчанию.

V519 The '* e' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1438, 1439. switch_xml.c 1439

static int preprocess(....)
{
  ....
  if ((e = strstr(tcmd, "/>"))) {
    *e += 2;
    *e = '\0';
    if (fwrite(e, 1, (unsigned) strlen(e),
          write_fd) != (int) strlen(e)) {
      switch_log_printf(....);
    }
  }
  ....
}

А в этом месте возникает ошибка как в предыдущем примере, только наоборот. Здесь в случае нахождения подстроки планируют сместить указатель и записать символ конца строки, но в операции „*e += 2“ изменяется не указатель, а код символа, на который он указывает. Потом в этот символ просто записывается терминальный ноль.

Правильный код выглядит так:

if ((e = strstr(tcmd, "/>"))) {
    e += 2;
    *e = '\0';
    ....
  }

V600 Consider inspecting the condition. The 'name' pointer is always not equal to NULL. fsodbc.cpp 323

JS_ODBC_FUNCTION_IMPL(GetData)
{
  ....
  SQLCHAR name[1024] = "";                                  //<==
  SQLCHAR *data = _colbuf;
  SQLLEN pcbValue;
  
  SQLDescribeCol(_stmt, x, name, sizeof(name), ....);       //<==
  SQLGetData(_stmt, x, SQL_C_CHAR, _colbuf, _cblen, &pcbValue);

  if (name) {                                               //<==
    if (SQL_NULL_DATA == pcbValue) {
      arg->Set(String::NewFromUtf8(GetIsolate(),
        (const char *)name), Null(info.GetIsolate()));
    } else {
      arg->Set(String::NewFromUtf8(GetIsolate(),
        (const char *)name), String::NewFromUtf8(GetIsolate(),
        data ? (const char *)data : ""));
    }
  }
  ....
}

В этой функции выделяется память на стеке для массива символов „name“. В начало записывается нулевой символ и с массивом выполняются какие-то действия. В условии „if (name) {....} хотели проверить, осталась ли строка пустой (признаком является нулевой символ в начале строки). Но из-за пропущенного символа разыменования указателя, проверяют значение указателя, который всегда не равен нулю.

V595 The 'val' pointer was utilized before it was verified against nullptr. Check lines: 2496, 2499. switch_ivr.c 2496

static int
switch_ivr_set_xml_chan_var(...., const char *val, int off)
{
  char *data;
  switch_size_t dlen = strlen(val) * 3 + 1;            //<==
  switch_xml_t variable;

  if (!val) val = "";                                  //<==
  ....
}

В функцию может прийти нулевой указатель на массив символов “val», о чём свидетельствует наличие проверки. Но до этого такой указатель будет передан в функцию «strlen()» для вычисления длины строки, где выполнится разыменование нулевого указателя.

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

V713 The pointer codec->cur_frame was utilized in the logical expression before it was verified against nullptr in the same logical expression. mod_opus.c 631

static switch_status_t
switch_opus_decode(switch_codec_t *codec, ....)
{
  ....
  if (opus_packet_get_bandwidth(codec->cur_frame->data) !=  //<==
        OPUS_BANDWIDTH_FULLBAND && codec->cur_frame &&      //<==
        (jb = switch_core_session_get_jb(....))) {
    ....
  }
  ....
}

Было непросто, но анализатор нашёл потенциальное разыменование нулевого указателя. Проблема из-за неправильного порядка логических выражений в условии. Сначала используется переменная «codec->cur_frame->data», а потом выполняется проверка указателя «codec->cur_frame» на равенство нулю.

V595 The 'a_engine' pointer was utilized before it was verified against nullptr. Check lines: 6024, 6052. switch_core_media.c 6024

SWITCH_DECLARE(switch_status_t)
switch_core_media_activate_rtp(switch_core_session_t *session)
{
  ....
  switch_port_t remote_rtcp_port = a_engine->remote_rtcp_port;
  ....
  if (session && a_engine) {
    check_dtls_reinvite(session, a_engine);
  }
  ....
}

В отличии от диагностики V713, диагностика V595 ищет потенциальное разыменование нулевого указателя в пределах всех функции. Обратите внимание, как используется указатель «a_engine».

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

V595 The 'session' pointer was utilized before it was verified against nullptr. Check lines: 6027, 6052. switch_core_media.c 6027
V595 The 'session' pointer was utilized before it was verified against nullptr. Check lines: 6689, 6696. switch_core_media.c 6689
V595 The 'v_engine' pointer was utilized before it was verified against nullptr. Check lines: 6677, 6696. switch_core_media.c 6677
V595 The 'stream.data' pointer was utilized before it was verified against nullptr. Check lines: 2409, 2411. switch_event.c 2409
V595 The 'stack' pointer was utilized before it was verified against nullptr. Check lines: 461, 466. switch_ivr_menu.c 461
V595 The 'smin' pointer was utilized before it was verified against nullptr. Check lines: 3269, 3277. switch_utils.c 3269
V595 The 'key' pointer was utilized before it was verified against nullptr. Check lines: 111, 124. switch_xml.c 111

V547 Expression 'fftstate->Perm == ((void *) 0)' is always false. Pointer 'fftstate->Perm' != NULL. fft.c 339

typedef struct {
  unsigned int SpaceAlloced;
  unsigned int MaxPermAlloced;
  double Tmp0[MAXFFTSIZE];
  double Tmp1[MAXFFTSIZE];
  double Tmp2[MAXFFTSIZE];
  double Tmp3[MAXFFTSIZE];
  int Perm[MAXFFTSIZE];
  int factor [NFACTOR];

} FFTstr;

static int   FFTRADIX (...., FFTstr *fftstate)
{
  ....
  if (fftstate->Tmp0 == NULL || fftstate->Tmp1 == NULL ||
      fftstate->Tmp2 == NULL || fftstate->Tmp3 == NULL ||
      fftstate->Perm == NULL) {
    return -1;
  }
  ....
}

В коде присутствует большое, но бессмысленное условие, в котором проверяются адреса 5 массивов, являющихся членами класса FFTstr. При этом не важно, на стеке создан объект класса или в куче. Адреса массивов всегда будут отличаться он нуля. Даже если указатель 'fftstate' будет равен 0, всё равно проверки не имеют смысла, так члены Tmp0..Tmp3 имеют смещение относительно начала структуры.

Двойная защита

V530 The return value of function 'LoadLibraryExA' is required to be utilized. switch_dso.c 42

V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 41, 45. switch_dso.c 45

SWITCH_DECLARE(switch_dso_lib_t) switch_dso_open(....)
{
  HINSTANCE lib;

  lib = LoadLibraryEx(path, NULL, 0);

  if (!lib) {
    LoadLibraryEx(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
  }

  if (!lib) {
    DWORD error = GetLastError();
    *err = switch_mprintf("dll open error [%ul]\n", error);
  }

  return lib;
}

На этом фрагменте кода возникла интересная ситуация, когда в одном месте сработали 2 разные диагностики анализатора. В V530 говорится, что у функции «LoadLibraryEx()» не используется возвращаемое значение, а в V581 говорится, что присутствуют 2 проверки с одинаковым логическим выражением.

Первая проверка дескриптора «lib» проверяет загрузку модуля функцией «LoadLibraryEx()», если дескриптор нулевой, то надо попробовать загрузить модуль ещё раз. В этот момент забывают перезаписать дескриптор 'lib' новым значением, возвращаемым функцией и при второй проверке дескриптор всё равно остаётся нулевым.

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

lib = LoadLibraryEx(path, NULL, 0);
if (!lib) {
    lib = LoadLibraryEx(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
}

Про память

V597 The compiler could delete the 'memset' function call, which is used to flush 'corrSurfBuff' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pitch_estimator.c 158

void WebRtcIsac_InitializePitch(const double *in,
                                const double old_lag,
                                const double old_gain,
                                PitchAnalysisStruct *State,
                                double *lags)
{
  ....
  for(k = 0; k < 2*PITCH_BW+3; k++)
  {
    CorrSurf[k] = &corrSurfBuff[10 + k * (PITCH_LAG_SPAN2+4)];
  }
  /* reset CorrSurf matrix */
  memset(corrSurfBuff, 0, sizeof(double) * (10 + (2*PITCH_BW+3)
    * (PITCH_LAG_SPAN2+4)));
  ....
}

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

Примечание. Это не паранойя. Компилятор действительно удаляет такие вызовы функций. Познакомьтесь с описанием диагностики V597, чтобы понять всю глубину кроличьей дыры. Для неверующих даже приводится ассемблерный листинг. Это серьезная и к сожалению очень распространенная ошибка безопасности.

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'abuf' is lost. Consider assigning realloc() to a temporary pointer. switch_ivr_play_say.c 1535

SWITCH_DECLARE(switch_status_t) switch_ivr_play_file(....)
{
  ....
  if (buflen > write_frame.buflen) {
    abuf = realloc(abuf, buflen);
    write_frame.data = abuf;
    write_frame.buflen = buflen;
  }
  ....
}

Данный код является потенциально опасным: рекомендуется результат функции realloc() сохранять в другой переменной. Функция realloc() производит изменение размера некоторого блока памяти. В случае, если изменение размера блока памяти в данный момент невозможно, функция вернёт нулевой указатель. Главная проблема заключается в том, что при использовании конструкции вида «ptr = realloc(ptr, ...)» указатель ptr на этот блок данных может быть утерян.

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

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'buf' is lost. Consider assigning realloc() to a temporary pointer. switch_event.c 1556
V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'buf' is lost. Consider assigning realloc() to a temporary pointer. switch_event.c 1582

Остальные предупреждения

V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 802, 837. switch_utils.h 837

#ifdef _MSC_VER
#pragma warning(disable:6011)
#endif
static inline char *switch_lc_strdup(const char *it)
{
  ....
}


static inline char *switch_uc_strdup(const char *it)
{
  ....
}
#ifdef _MSC_VER
#pragma warning(default:6011)
#endif

Программисты часто считают, что после директивы «pragma warning(default: X)» опять начнут действовать предупреждения, отключенные ранее помощью «pragma warning(disable: X)». Это не так. Директива 'pragma warning(default: X)' устанавливает предупреждение с номером 'X' в состояние, которое действует ПО УМОЛЧАНИЮ. Это далеко не одно и то же.

Корректный вариант кода:

#pragma warning(push)
#pragma warning(disable: 6011)
....
// Correct code triggering the 6011 warning
....
#pragma warning(pop)

V555 The expression 'parser->maxlen — parser->minlen > 0' will work as 'parser->maxlen != parser->minlen'. switch_ivr.c 2342

typedef uintptr_t switch_size_t;

switch_size_t maxlen;
switch_size_t buflen;
switch_size_t minlen;

SWITCH_DECLARE(void *) switch_ivr_digit_stream_parser_feed(....)
{
  ....
  if (parser->maxlen - parser->minlen > 0 && ....) {
    len = 0;
  }
  ....
}

Разность беззнаковых чисел всегда больше нуля, если они не равны. Так здесь ошибка или имели ввиду проверку 'parser->maxlen != parser->minlen'?

V612 An unconditional 'goto' within a loop. mod_verto.c 112

static void json_cleanup(void)
{
  ....
top:

  for (hi = switch_core_hash_first_iter(....); hi;) {
    switch_core_hash_this(hi, &var, NULL, &val);
    json = (cJSON *) val;
    cJSON_Delete(json);
    switch_core_hash_delete(json_GLOBALS.store_hash, var);
    goto top;
  }
  switch_safe_free(hi);

  switch_mutex_unlock(json_GLOBALS.store_mutex);
}

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

Ещё несколько мест:

V612 An unconditional 'break' within a loop. mod_event_socket.c 1643
V612 An unconditional 'goto' within a loop. mod_verto.c 328
V612 An unconditional 'break' within a loop. mod_verto.c 1993

V652 The '!' operation is executed 3 or more times in succession. mod_verto.c 3032

static switch_bool_t verto__modify_func(....)
{
  ....
  switch_core_media_toggle_hold(session,
    !!!switch_channel_test_flag(tech_pvt->channel, ....));
  ....
}

Непонятное место с использованием целых трёх операторов отрицания, возможно, имеет место опечатка.

V567 Unspecified behavior. The order of argument evaluation is not defined for 'strtol' function. Consider inspecting the 'exp' variable. switch_utils.c 3759

SWITCH_DECLARE(int) switch_number_cmp(const char *exp, int val)
{
  for (;; ++exp) {
    int a = strtol(exp, (char **)&exp, 10);
    if (*exp != '-') {
      if (a == val)
        return 1;
    } else {
      int b = strtol(++exp, (char **)&exp, 10);        //<==
      ....
    }
    if (*exp != ',')
      return 0;
  }
}

Неизвестно, будет сначала изменён указатель 'exp' или взят его адрес. Соответственно, правильно работает выражение или нет, зависит от везения.

V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. switch_core.c 3014

SWITCH_DECLARE(int) switch_max_file_desc(void)
{
  int max = 0;                                   //<==

#ifndef WIN32
#if defined(HAVE_GETDTABLESIZE)
  max = getdtablesize();
#else
  max = sysconf(_SC_OPEN_MAX);
#endif
#endif

  return max;

}

SWITCH_DECLARE(void) switch_close_extra_files(....)
{
  int open_max = switch_max_file_desc();
  int i, j;

  for (i = 3; i < open_max; i++) {               //<==
    ....
    close(i);

  skip:

    continue;

  }
}

Неизвестно, ошибка это или нет, но анализатор нашёл код-заглушку для Windows в функции «switch_max_file_desc()». Если эта функция всегда возвращает ноль в Windows, то цикл после её вызова никогда не выполняется.

Заключение

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Hello, Is That FreeSWITCH? Then We're Coming to Check You!.

This entry passed through the Full-Text RSS service - if this is your content and you're reading it on someone else's site, please read the FAQ at http://ift.tt/jcXqJW.

Парсер Хабра

...

воскресенье, 11 октября 2015 г.

Алло! Это FreeSWITCH? Тогда мы проверим вас

Введение

If (bug) then find_copy_paste();

Использование типа SOCKET

Двойные присваивания

Ошибки в строках

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

Двойная защита

Про память

Остальные предупреждения

Заключение

Комментариев нет:

Отправить комментарий

...

воскресенье, 11 октября 2015 г.

Алло! Это FreeSWITCH? Тогда мы проверим вас

Введение

If (bug) then find_copy_paste();

Использование типа SOCKET

Двойные присваивания

Ошибки в строках

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

Двойная защита

Про память

Остальные предупреждения

Заключение

Комментариев нет:

Отправить комментарий

воскресенье, 11 октября 2015 г.