По просьбам наших читателей для проверки с помощью PVS-Studio был взят проект с открытым исходным кодом FreeSWITCH. Первоначально он был основан разработчиками проекта, Asterisk, который мы уже проверяли. Проект FreeSWITCH активно разрабатывается и содержит много интересных предупреждений анализатора, которые будут описаны в статье.
FreeSWITCH – масштабируемое кроссплатформенное программное обеспечение для телефонии с открытым кодом, предназначенное для маршрутизации и объединения популярных протоколов связи, использующих аудио-, видео-, текстовую и прочие виды информации. Платформа была разработана в 2006 году с целью восполнить дефицит подобных решений, образовавшийся с появлением закрытых коммерческими приложений. Кроме того, FreeSWITCH предоставляет разработчикам стабильную телефонную платформу, на основе которой они могут создавать собственные разнообразные приложения с помощью большого набора бесплатных инструментов.
С помощью анализатора PVS-Studio 5.29 проект FreeSWITCH был с лёгкостью проверен в Visual Studio 2015.
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' в дальнейшем может использоваться где угодно.
К сожалению, таких мест много:
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!
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;
}
....
}На первый взгляд здесь реализован простой алгоритм:
Ошибка кроется в этапе 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".
Ещё список опасных мест:
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 на этот блок данных может быть утерян.
Ещё такие места:
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);
}В исходном коде проекта в нескольких местах используют операторы безусловного перехода, это затрудняет читаемость и поддержку кода, особенно при использовании в циклах.
Ещё несколько мест:
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 довольно больший и анализатором было выдано много интересных предупреждений, хотя предупреждений на используемые библиотеки в этом проекте было ещё больше, но это уже совсем другая история. До публикации статьи разработчики уже были уведомлены о проверке их проекта и получили полный отчёт анализатора. Поэтому какие-то участки кода могут быть исправлены.