Geekly Articles each Day
📜 ⬆️ ⬇️

Hello! Is it FreeSWITCH? Then we will check you


At the request of our readers, the open source project FreeSWITCH was taken for verification using PVS-Studio. It was originally founded by the developers of the project, Asterisk, which we have already tested. The FreeSWITCH project is being actively developed and contains many interesting analyzer warnings, which will be described in the article.

Introduction

FreeSWITCH is an open source telephony platform designed to meet the need for voice or text-driven systems that scale from softphone to softswitch. FreeSWITCH can be used as a switch, PBX, media gateway or media server for IVR applications using simple or XML scripts to control the call processing algorithm.

Using the PVS-Studio 5.29 analyzer, the FreeSWITCH project was easily tested in Visual Studio 2015.

If (bug) then find_copy_paste ();



')
V593 Consider reviewing the A = B! = C 'kind. The expression is calculated as the 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; } .... }

The source of logical errors in the program may be an incorrectly written condition. For example, in this code fragment, the priority of the comparison operator is higher than the priority of the assignment operator. Thus, the result of a logical operation is stored in 'status', and not the result of the function switch_core_session_recv_dtmf (). The code contains the goto transition operator, so the corrupted value of the 'status' variable can later be used anywhere.

Unfortunately, there are many such places:
V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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"); } .... }

In the conditions cascade, the same variable “globals.db_dsn” is checked, therefore the message about unsuccessful connection to the database will not be logged.

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 ? "" : ">"); } } .... }

A large piece of code containing a lot of identical text. If there is no error, this code fragment can be cut in half, otherwise here is another forgotten copy-paste.

V590 Consider inspecting the '* data ==' '&& * data! =' \ 0 '' expression. The expression is misprint. mod_curl.c 306
 static char *print_json(switch_memory_pool_t *pool, ....) { .... while (*data == ' ' && *data != '\0') { data++; } .... }

There is no error, but the expression is redundant, which can make it difficult to read the code. Checking "* data! = '\ 0'" does not make sense. Abbreviated code:
 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); } .... }

From the code you can see that it was planned to write “else if '. But apparently accidentally missed the keyword 'else' and this led to a change in the logic of the program.

To understand the essence of the error, let's consider a simplified version of the code. At the beginning of the correct option:
 if (A == 1) { X(); } else if (A == 2) { Y(); } else { Z(); }

Depending on the value of the variable A, one of the functions will be called: X, Y, or Z. Let's see what happens if we forget the 'else':
 if (A == 1) { X(); } if (A == 2) { Y(); } else { Z(); }

Now, if A is equal to one, then not only the function X, but also Z will be called!

Use of type 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; } .... }

The SOCKET type is an unsigned type, so a comparison with a negative number is not allowed . In this case, it is necessary to perform a comparison with special named constants for working with the SOCKET type, for example, SOCKET_ERROR, etc.

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"); } .... }

A similar example of incorrect work with variables of type SOCKET. This is an unsigned type, and there are specially defined constants to check the error status, for example, SOCKET_ERROR.

Double assignments




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; } .... }

The analyzer has detected that a variable is assigned to itself. It can be assumed that during the writing of the code, the wrong structure field was randomly selected in the second assignment: „win32_hGlobal_SkypeAPIWindowHandle“ instead of „win32_hInit_MainWindowHandle“.

Perhaps the function code should be like this:
 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 twice 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; } .... }

The analyzer detected a potential error related to the fact that the same variable is assigned a value twice in a row. And between these assignments the variable itself is not used. Using the analyzer, it was possible to find the missed check: the value of the 'status' variable is not used anywhere.

Perhaps the code should be like this:
 .... param_index = status ? info[0]->Int32Value() : 0; .... param_value = status ? info[1]->Int32Value() : 0;

V519 The 'status' variable is assigned twice 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; } .... }

Why here the status of the record is simply overwritten on the successful is not clear. Let's leave the study of this fragment to the authors of the code.

Errors in strings




V694 The condition (mode + 5) is only false if there is a pointer overflow which is undefined behavior 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; } .... }

At first glance, a simple algorithm is implemented here:
  1. Find the substring "mode =";
  2. Make sure that there is no null character after the substring;
  3. Convert the next character to a number.
The error lies in the stage N2: making sure that the 'mode' pointer to the substring is non-zero, the code shifts the pointer to 5 characters, but it will remain non-zero. In operation (mode + 5) the dereferencing of the shifted pointer is missing. Because of such an error, situations are possible when the terminator is converted to a number and get a value of zero. Thanks to the “if (! Codec_ms) {codec_ms = 30;}“ check, the zero value is always reset to the default value.

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(....); } } .... }

And in this place an error occurs as in the previous example, just the opposite. Here, if the substring is found, they plan to move the pointer and write the end-of-line character, but in the “* e + = 2” operation, it is not the pointer that changes, but the character code to which it points. Then the terminal zero is simply written to this character.

The correct code looks like this:
 if ((e = strstr(tcmd, "/>"))) { e += 2; *e = '\0'; .... }

V600 Consider inspecting the condition. The 'name' pointer is not always 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 : "")); } } .... }

This function allocates memory on the stack for the character array "name". The zero character is written to the beginning and some actions are performed with the array. In the condition „if (name) {....} they wanted to check whether the string remained empty (the sign is the zero character at the beginning of the string). But because of the missed pointer dereference character, the value of the pointer, which is always non-zero, is checked.

V595 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 = ""; //<== .... }

A zero pointer to the “val” character array can come into the function, which is indicated by the presence of a check. But before that, such a pointer will be passed to the “strlen ()” function to calculate the length of the string where the null pointer dereference will be performed.

Dangerous Signs




V713 The pointer codec-> cur_frame was used 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(....))) { .... } .... }

It was not easy, but the analyzer found a potential dereference of the null pointer. Problem due to incorrect order of logical expressions in condition. First, the “codec-> cur_frame-> data” variable is used, and then the “codec-> cur_frame” pointer is checked for equality to zero.

V595 The 'a_engine' pointer was used 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); } .... }

Unlike the V713 diagnostic, the V595 diagnostic looks for a potential null pointer dereference within all functions. Notice how the "a_engine" pointer is used.

Another list of dangerous places:
V547 Expression 'fftstate-> Perm == ((void *) 0)' is always false. Pointer 'fftstate-> Perm'! = NULL. fft.c
 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; } .... }

There is a large, but meaningless condition in the code in which the addresses of 5 arrays that are members of the FFTstr class are checked. It does not matter if a class object or heap was created on the stack. The addresses of arrays will always be different from zero. Even if the 'fftstate' pointer is 0, the checks are still meaningless, as the Tmp0..Tmp3 members are offset from the beginning of the structure.

Double protection




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

V581 The conditional expressions of the 'if' are located 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; }

An interesting situation arose on this code fragment, when 2 different diagnostics of the analyzer worked in one place. In V530, it says that the “LoadLibraryEx ()” function does not use a return value, and in V581 it says that there are 2 checks with the same logical expression.

The first check of the “lib” descriptor checks the loading of the module by the “LoadLibraryEx ()” function; if the descriptor is zero, then you should try to load the module again. At this point, they forget to overwrite the 'lib' descriptor with the new value returned by the function, and during the second check the descriptor still remains zero.

The correct code snippet is:
 lib = LoadLibraryEx(path, NULL, 0); if (!lib) { lib = LoadLibraryEx(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH); }

About memory




V597 The compiler couldn’t need the 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))); .... }

The code above may leave the matrix uncleaned. Note that the “corrSurfBuff” array is cleared at the end and is no longer used. Therefore, when building the Release version of a program, the compiler will most likely remove the memset () function call. The compiler has every right to do this. The analyzer suggests using the RtlSecureZeroMemory () function for Windows, but since the project is cross-platform, you should find another way to avoid optimizing other compilers.

Note. This is not paranoia. The compiler really removes such function calls. Read the description of the V597 diagnostic to understand the depth of the rabbit hole. For non-believers, even an assembler listing is provided. This is a serious and unfortunately very common security error.

V701 realloc () possible leak: when realloc () fails in allocating memory, the 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; } .... }

This code is potentially dangerous: it is recommended to save the result of the realloc () function in another variable. The realloc () function changes the size of a block of memory. If it is not possible to change the size of the memory block at the moment, the function will return a null pointer. The main problem is that when using a construction like "ptr = realloc (ptr, ...)" the ptr pointer to this data block can be lost.

More places like this:

The remaining warnings




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

Programmers often believe that after the “pragma warning (default: X)” directive, the warnings will again take effect, previously disabled using the “pragma warning (disable: X)”. This is not true. The 'pragma warning (default: X)' directive sets the warning with the number 'X' to the state that is valid DEFAULT. This is not the same thing.

The correct code is:
 #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; } .... }

The difference between unsigned numbers is always greater than zero if they are not equal. So here is the error or meant checking '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); }

In the project source code, unconditional jump operators are used in several places, which makes it difficult to read and maintain the code, especially when used in loops.

A few more places:
V652 The '!' 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, ....)); .... }

An incomprehensible place with the use of as many as three negation operators, perhaps there is a typo.

V567 Unspecified behavior. Evaluation order 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; } }

It is not known whether the 'exp' pointer will be changed first or its address will be taken. Accordingly, the expression works correctly or not, depends on luck.

V621 Consider inspecting the 'for' operator. It is possible that you will not 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; } }

It is not known whether this is an error or not, but the analyzer found a stub code for Windows in the “switch_max_file_desc ()” function. If this function always returns zero in Windows, the loop after its call is never executed.

Conclusion

In the article, I talked about the most suspicious parts of the FreeSWITCH project code, in my opinion, found using the PVS-Studio static analyzer. This is another project related to computer telephony, as I once tested a similar Asterisk project. The FreeSWITCH project is quite large and many interesting warnings were issued by the analyzer, although there were even more warnings to the used libraries in this project, but this is another story. Prior to the publication of the article, the developers were already notified of the verification of their project and received a full report of the analyzer. Therefore, some parts of the code can be corrected.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Hello, Is That FreeSWITCH? Then We're Coming to Check You! .



Read the article and have a question?
Often our articles are asked the same questions. We collected the answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/268603/

More articles:


All Articles