To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (an extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Hello, Is That FreeSWITCH? Then We're C…

Hello, Is That FreeSWITCH? Then We're Coming to Check You!

Oct 11 2015

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.

0351_FreeSWITCH/image1.png

Introduction

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.

If (bug) then find_copy_paste();

0351_FreeSWITCH/image2.png

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:

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

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!

Using the SOCKET type

0351_FreeSWITCH/image3.png

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.

Double assignments

0351_FreeSWITCH/image4.png

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.

Errors in strings

0351_FreeSWITCH/image5.png

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:

  • Find the "mode=" substring;
  • Make sure there is no null character after the substring;
  • Convert the next character into a number.

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.

Dangerous pointers

0351_FreeSWITCH/image6.png

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:

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

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.

Double defense

0351_FreeSWITCH/image7.png

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

Memory-related issues

0351_FreeSWITCH/image8.png

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:

  • 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

Miscellaneous

0351_FreeSWITCH/image9.png

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

Many 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:

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

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.

Conclusion

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.

Popular related articles
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept