Following our readers' requests, we have scanned the open-source project FreeSWITCH with PVS-Studio. This project was initially founded by the developers of the Asterisk project, which we already analyzed some time ago. The FreeSWITCH project is actively developing and has a handful of interesting issues, which we will discuss in this article.
FreeSWITCH is a scalable open source cross-platform telephony platform designed to route and interconnect popular communication protocols using audio, video, text or any other form of media. It was created in 2006 to fill the void left by proprietary commercial solutions. FreeSWITCH also provides a stable telephony platform on which many applications can be developed using a wide range of free tools.
The FreeSWITCH project was smoothly analyzed with the PVS-Studio 5.29 analyzer in 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;
}
....
}The source of logical errors in a program may be in an incorrectly written condition. In this code fragment, for example, the comparison operation's precedence is higher than that of the assignment operation. So what is saved into the 'status' variable is the result of a logical operation, not of the switch_core_session_recv_dtmf() function. The code also contains the goto statement, so the spoiled value of the 'status' variable may be then used anywhere in the code.
Unfortunately, the code is abundant in bugs like that:
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");
}
....
}In a cascade of conditions, one and the same variable, "globals.db_dsn", is checked, so the message about a database connection failure won't 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 ? "" : ">");
}
}
....
}That's a large bulk of code with lots of identical text. If there appears to be no error here, this fragment can be abridged twice. Otherwise, it's another unfixed 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++;
}
....
}No error here, but the expression is redundant, which may make the code difficult to read. The "*data != '\0' " check makes no sense. The correct, abridged, version of this code should look as follows:
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);
}
....
}As seen from the code, the programmer intended to write "else if" but probably missed the 'else' keyword, which resulted in changing the program's logic.
To understand what this error is about, let's examine a simplified version of this code. Here's the correct version first:
if (A == 1) {
X();
} else if (A == 2) {
Y();
} else {
Z();
}Depending on the A variable's value, one of the functions X, Y, or Z will be called. Let's see now what will happen if we "forget" 'else':
if (A == 1) {
X();
} if (A == 2) {
Y();
} else {
Z();
}Now, if A equals one, not only will the X function be called, but the Z function as well!
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 unsigned, which means it's not valid to compare it with a negative number. In cases like this, the comparison should be done against special named constants, when handling the SOCKET type – for example SOCKET_ERROR and the like.
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 handling of SOCKET-type variables. This is an unsigned type, and one should use special constants to check for the error status – for example 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;
}
....
}The analyzer has detected a variable being assigned to itself. I guess the programmer picked a wrong structure field for the second assignment operation: "win32_hGlobal_SkypeAPIWindowHandle" instead of "win32_hInit_MainWindowHandle".
The function's code should have probably looked 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 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;
}
....
}The analyzer has detected a potential error that has to do with one and the same variable being assigned values twice on end, the variable itself not being used in any way between the two assignment operations. The analyzer has helped to find a missing check: the value of the 'status' variable is not used anywhere.
The code should probably look as follows:
....
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;
}
....
}It's not clear why the writing status is simply redefined as successful. Let's leave it to the code's authors to sort it out.
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;
}
....
}At first sight, we seem to have a simple algorithm in this code:
The bug is lurking in step 2: after checking that the 'mode' pointer, pointing to the substring, is not null, it is shifted by 5 characters, but it will still remain non-null. In the (mode + 5) expression, dereferencing of the shifted pointer is missing. This error opens the way for issues when a null character is converted into a number, resulting in the value zero. Thanks to the "if (!codec_ms) { codec_ms = 30;}" check, the value zero is always cast back 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(....);
}
}
....
}Here we've got a similar error as in the previous example save that it is opposite in meaning. On finding the substring, the programmer wants the pointer to be shifted and the null character written. But in the "*e += 2" expression, it is the code of the character the pointer refers to that is changed instead of the pointer itself. After that, it's just the null terminator to be written into this character.
The correct version of this code should look as follows:
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 : ""));
}
}
....
}In this function, memory is allocated on the stack for the character array "name". A null character is written into the beginning of the array, the latter being then handled somehow. In the "if (name) {....}" condition, the programmer wanted to check if the string had remained empty (which is indicated by a null character in the beginning of the string), but because of the missing pointer-dereferencing character, they check a pointer that is never null.
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 = ""; // <=
....
}The function may receive a null pointer to the character array "val", which is indicated by the presence of the corresponding check. But before that, this null pointer will be passed into the "strlen()" function to evaluate the string length, where it will be dereferenced.
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(....))) {
....
}
....
}It was tricky, but the analyzer has managed to find a potential null-pointer-dereferencing issue caused by an incorrect order of logical expressions inside a condition. In that condition, the "codec->cur_frame->data" variable is used first and then the "codec->cur_frame" pointer is checked for null.
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);
}
....
}Unlike the V713 diagnostic, diagnostic V595 searches for potential null-pointer-dereferencing errors through the entire function. Notice the way the "a_engine" pointer is used.
Here's a list of other dangerous issues with pointers:
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;
}
....
}There is a large yet meaningless condition checking the addresses of 5 arrays belonging to the FFTstr class, and it doesn't matter whether the class object is created on the stack or the heap. The arrays' addresses will always be different from zero. Even if the 'fftstate' pointer equals 0, the checks don't make sense anyway because the Tmp0..Tmp3 members are offset from the structure's beginning.
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;
}What's interesting about this fragment is that it triggered two different diagnostics at once. Diagnostic V530 tells us that the return value of the "LoadLibraryEx()" function is not used, while diagnostic V581, that the code contains two checks with identical logical expressions.
The first check of the "lib" descriptor checks if the module has been loaded by the "LoadLibraryEx()" function; if the descriptor is null, the program will attempt to load the module once again. It is at this point that the programmer forgot to rewrite the value in the 'lib' descriptor with a new value returned by the function, so the descriptor will still remain null at the second check.
The correct version of this code:
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)));
....
}The code above may leave the matrix uncleared. Notice that the "corrSurfBuff" array is cleared at the end and is not used anymore afterwards. Because of that, the compiler will almost surely delete the call of the memset() function when building the Release version of the program, and it does have an absolute right to do so. The analyzer suggests using the RtlSecureZeroMemory() function for Windows instead, but since the project is cross-platform, the authors need to find another way to avoid optimizations by other compilers.
Note. We are not being paranoid. The compiler does delete function calls like that. Look for the V597 diagnostic rule's description to see how deep the rabbit hole goes. For those who don't trust me, there is even an assembly listing included. This is a serious, and unfortunately very common, security issue.
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;
}
....
}This code is potentially dangerous: we recommend that the realloc() function's result be saved in a different variable. The realloc() function is used to change the size of a certain memory block. If it is impossible at the moment, the function will return a null pointer. The biggest problem here is that in "ptr = realloc(ptr, ...)"-like constructs, the ptr pointer to this data block may get lost.
Two other similar issues:
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)
#endifMany programmers believe that after the "pragma warning(default : X)" directive, warnings previously disabled through the "pragma warning(disable: X)" directive will start working again. They are wrong. The 'pragma warning(default : X)' directive sets the warning with number 'X' to its DEFAULT state, which is quite a different thing.
The correct version of this code:
#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;
}
....
}A difference of unsigned numbers is always larger than zero unless they are equal. So is there an error here or did the programmer actually mean the 'parser->maxlen != parser->minlen' check?
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);
}Also, the project's authors use unconditional jump statements at some points in the code, which makes it more difficult to read and maintain, especially where loops are involved.
A few other issues of this kind:
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, ....));
....
}A strange fragment with as many as three negation operators used at once. There is probably a typo somewhere.
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;
}
}It's unknown whether first the 'exp' pointer will be changed or its address obtained. Therefore, whether or not the expression works right depends on chance.
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;
}
}I don't know if it's an error or not, but the analyzer has found a stub for the Windows version inside the "switch_max_file_desc()" function. If this function always returns zero on Windows, the loop following it is never executed.
In this article, I've told you about the most suspicious (to my mind) code fragments of the FreeSWITCH project detected by the PVS-Studio static analyzer. It's just another project dealing with computer telephony: I once scanned a similar project Asterisk. The FreeSWITCH project is pretty large, and the analyzer output plenty of interesting messages, although the libraries it uses triggered way more warnings, but it's just a different story. Before this article was published, we had informed the project's authors about the analysis and sent them a detailed analysis report. So some of the issues discussed here may be already fixed by now.