Sviatoslav Razmyslov

Aug 26 2014

Tags:

#Cpp

Asterisk: PVS-Studio Takes Up Telephony

• Typo #1
• Typo #2
• Typo #3
• Typo #4
• Typo #5
• Identical comparisons
• Always false
• Insidious optimization
• Pointers
• Excessive code
• Conclusion

Asterisk is a software implementation of a telephone private branch exchange (PBX); it was created in 1999 by Mark Spencer of Digium. Like any PBX, it allows attached telephones to make calls to one another, and to connect to other telephone services, such as the public switched telephone network (PSTN) and Voice over Internet Protocol (VoIP) services. Its name comes from the asterisk symbol, *.

Asterisk is released under a dual license model, using the GNU General Public License (GPL) as a free software license and a proprietary software license to permit licensees to distribute proprietary, unpublished system components.

In this article, we are going to discuss the results of the check of the Asterisk project by PVS-Studio 5.18.

The project seems to be regularly checked by the Coverity analyzer, which is indicated by comments like this one:

/* Ignore check_return warning from Coverity for ast_exists_extension below */

However, I still found some disappointing typos in the code. Let's try to figure them out as well as other potential issues. The source code was downloaded from the project's SVN repository.

Typo #1

V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 2513, 2516. chan_sip.c 2516

static void sip_threadinfo_destructor(void *obj)
{
  struct sip_threadinfo *th = obj;
  struct tcptls_packet *packet;

  if (th->alert_pipe[1] > -1) {            // <=
    close(th->alert_pipe[0]);
  }
  if (th->alert_pipe[1] > -1) {
    close(th->alert_pipe[1]);
  }
  th->alert_pipe[0] = th->alert_pipe[1] = -1;
  ....
}

In this code, the programmer intended to check the states of pipes 0 and 1, after which they should be closed, but the typo prevents the state of pipe 0 from being checked. The reason why the code runs well for a long time is probably that both pipes are used in most cases.

Typo #2

V503 This is a nonsensical comparison: pointer < 0. parking_manager.c 520

static int manager_park(....)
{
  ....
  const char *timeout = astman_get_header(m, "Timeout");
  ....
  int timeout_override = -1;
  ....
  if (sscanf(timeout, "%30d", &timeout_override) != 1 ||
    timeout < 0) {                                          // <=
      astman_send_error(s, m, "Invalid Timeout value.");
      return 0;
  }
}

In this fragment, a pointer is meaninglessly compared to zero. I guess the programmer wanted to check the timeout_override variable returned by the sscanf function.

Typo #3

V568 It's odd that the argument of sizeof() operator is the 'data[0] * 2' expression. channel.c 8853

static int redirecting_reason_build_data(....)
{
  ....
  if (datalen < pos + sizeof(data[0] * 2) + length) {       // <=
    ast_log(LOG_WARNING, "No space left for %s string\n", label);
    return -1;
  }
  ....
}

The sizeof() operator calculates the expression type and returns the size of this type while the expression itself fails to be calculated. Complex expressions usually indicate that the code contains an error, which errors are most often caused by typos. This is just the case in the example above: multiplication by two was most likely meant to be outside the parentheses of the sizeof() operator.

Typo #4

V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: "KW_INCLUDES" "KW_JUMP". ael.y 736

static char *token_equivs1[] =
{
  ....
  "KW_IF",
  "KW_IGNOREPAT",
  "KW_INCLUDES"          // <=
  "KW_JUMP",
  "KW_MACRO",
  "KW_PATTERN",
  ....
};

static char *ael_token_subst(const char *mess)
{
  ....
  int token_equivs_entries = sizeof(token_equivs1)/sizeof(char*);
  ....
  for (i=0; i<token_equivs_entries; i++) {
    ....
  }
  ....
}

When declaring an array of string literals, two strings get united into one. This error may be a consequence of a typo when a comma is missing between the string literals.

This is what the items of the token_equivs1 array actually look like:

Another issue of that kind:

• V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: "includes" "jump". ael.y 776

Typo #5

V501 There are identical sub-expressions 'strcasecmp(item->u1.str, "endwhile") == 0' to the left and to the right of the '||' operator. pval.c 2513

void check_pval_item(pval *item, ....)
{
  ....
  if (strcasecmp(item->u1.str,"GotoIf") == 0
      || strcasecmp(item->u1.str,"GotoIfTime") == 0
      || strcasecmp(item->u1.str,"while") == 0
      || strcasecmp(item->u1.str,"endwhile") == 0           // <=
      || strcasecmp(item->u1.str,"random") == 0
      || strcasecmp(item->u1.str,"gosub") == 0
      || strcasecmp(item->u1.str,"gosubif") == 0
      || strcasecmp(item->u1.str,"continuewhile") == 0
      || strcasecmp(item->u1.str,"endwhile") == 0           // <=
      || strcasecmp(item->u1.str,"execif") == 0
      || ....)
  {....}
}

One of the expressions in the cascade of conditional operators is repeated twice. One can never guarantee that a typo hasn't affected some very important condition.

Identical comparisons

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

static void channel_hangup_handler_cb(....)
{
  const char *event;
  ....
  if (!strcmp(action, "type")) {
    event = "HangupHandlerRun";
  } else if (!strcmp(action, "type")) {
    event = "HangupHandlerPop";
  } else if (!strcmp(action, "type")) {
    event = "HangupHandlerPush";
  } else {
    return;
  }
  ....
}

This is a highly suspicious fragment: what is done here is either assigning the "HangupHandlerRun" string to the 'event' variable or leaving the function.

Always false

V547 Expression is always false. Unsigned type value is never < 0. enum.c 309

static int ebl_callback(....)
{
  unsigned int i;
  ....
  if ((i = dn_expand((unsigned char *)fullanswer,
     (unsigned char *)answer + len,
     (unsigned char *)answer, c->apex, sizeof(c->apex) - 1)) < 0)
  {
    ast_log(LOG_WARNING, "Failed to expand hostname\n");
    return 0;
  }
}

The 'i' variable is unsigned and will never be less than zero. The dn_expand() function returns value -1 in case of failure, so the 'i' variable cannot be 'unsigned'.

Insidious optimization

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

static int silence_generator_generate(....)
{
  short buf[samples];

  struct ast_frame frame = {
    .frametype = AST_FRAME_VOICE,
    .data.ptr = buf,
    .samples = samples,
    .datalen = sizeof(buf),
  };
  frame.subclass.format = ast_format_slin;
  
  memset(buf, 0, sizeof(buf));      // <=
  ....
}

Since the 'buf' array is not used anywhere after calling the 'memset' function, the compiler may remove the call for optimization's sake, and the array will not be cleared, as planned by the programmer.

Users tend to misunderstand the V597 warning. Here you are some references to figure out what the issue this diagnostic points out is all about:

• Overwriting memory - why?
• Zero and forget -- caveats of zeroing memory in C

Pointers

V595 The 'object_wizard->wizard' pointer was utilized before it was verified against nullptr. Check lines: 683, 686. sorcery.c 683

static void sorcery_object_wizard_destructor(void *obj)
{
  struct ast_sorcery_object_wizard *object_wizard = obj;

  if (object_wizard->data) {
    object_wizard->wizard->close(object_wizard->data);      // <=
  }

  if (object_wizard->wizard) {                              // <=
    ast_module_unref(object_wizard->wizard->module);
  }

  ao2_cleanup(object_wizard->wizard);                       // <=
}

For some reason, this code selectively checks the pointer for being null. Places like this usually indicate that there is some probability that a null pointer may get into the function, so it should be checked in all the related places before using it.

Excessive code

I don't think the next two samples are errors, but they can be simplified.

V584 The '1' value is present on both sides of the '==' operator. The expression is incorrect or it can be simplified. chan_unistim.c 1095

static void check_send_queue(struct unistimsession *pte)
{
  if (pte->last_buf_available == 1) {
    ....
  }
  else if (pte->last_seq_ack + 1 == pte->seq_server + 1) {  // <=
    ....
  }
}

Incrementing the arguments by one to both sides of the equal sign doesn't seem to make much sense.

V571 Recurring check. The 'wizard->wizard->retrieve_fields' condition was already verified in line 1520. sorcery.c 1521

void *ast_sorcery_retrieve_by_fields(....)
{
  ....
  if ((flags & AST_RETRIEVE_FLAG_MULTIPLE)) {
  ....
  } else if (fields && wizard->wizard->retrieve_fields) {  // <=
      if (wizard->wizard->retrieve_fields) {               // <=
        object = wizard->wizard->retrieve_fields(....);
      }
  }
}

It's not an error, but one of the pointer checks can surely be removed.

Conclusion

Using static analysis regularly will help you save quite a lot of time you could spend on solving more useful tasks than catching silly mistakes and typos.

Also, see the interesting article The Last Line Effect about typos.

#Cpp

0

0

0

0

SHARE

Tags:

#Cpp

We can email you a selection of our best articles once a month