# Asterisk: PVS\-Studio заинтересовался телефонией

Asterisk — свободное решение компьютерной телефонии с открытым исходным кодом от компании Digium\. Приложение работает на таких операционных системах, как Linux, FreeBSD, OpenBSD, Solaris\. Asterisk в комплексе с необходимым оборудованием обладает всеми возможностями классической АТС, поддерживает множество VoIP\-протоколов и предоставляет богатые функции управления звонками\.

В данной статье будут рассмотрены результаты проверки [Asterisk](https://www.asterisk.org/), полученные с помощью [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) 5\.18\.

Проект, по всей видимости, проверяется анализатором Coverity, о чём свидетельствуют комментарии вида:

/\* Ignore check\_return warning from Coverity for ast\_exists\_extension below \*/

Тем не менее, я заметил некоторые досадные опечатки\. Попробуем разобраться в них и в других подозрительных местах\. Исходный код взят из SVN репозитория проекта\.

![0276_asterisk_ru/image1.png](https://import.viva64.com/docx/blog/0276_asterisk_ru/image1.png)

## Опечатка \#1

[V581](https://pvs-studio.ru/ru/docs/warnings/v581/) The conditional expressions of the 'if' operators situated alongside each other are identical\. Check lines: 2513, 2516\. chan\_sip\.c 2516

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

Здесь планировалось проверять состояние каналов с номерами 0 и 1, после чего они закрываются, но из\-за опечатки состояние канала с номером 0 не проверяется\. Возможно, код корректно работает долгое время, потому что в большинстве случаев задействованы оба канала\.

## Опечатка \#2

[V503](https://pvs-studio.ru/ru/docs/warnings/v503/) This is a nonsensical comparison: pointer < 0\. parking\_manager\.c 520

```cpp
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;
  }
}
```

В этом месте выполняется бессмысленное сравнение указателя с нулём\. Скорее всего, хотели проверить переменную timeout\_override, которую вернула функция sscanf\.

## Опечатка \#3

[V568](https://pvs-studio.ru/ru/docs/warnings/v568/) It's odd that the argument of sizeof\(\) operator is the 'data\[0\] \* 2' expression\. channel\.c 8853

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

Оператор sizeof\(\) вычисляет тип выражения и возвращает размер этого типа, но само выражение не вычисляется\. Сложные выражения являются признаком наличия ошибки\. Чаще всего эти ошибки связаны с опечатками\. Как в данном примере: скорее всего, умножение на два должно быть за скобкой оператора sizeof\(\)\.

## Опечатка \#4

[V653](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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++) {
    ....
  }
  ....
}
```

В объявлении массива строковых литералов две строки объединяются в одну\. Ошибка может быть следствием опечатки, когда пропущена запятая между строковыми литералами\. 

Вот как на самом деле выглядят элементы массива token\_equivs1:

![0276_asterisk_ru/image2.png](https://import.viva64.com/docx/blog/0276_asterisk_ru/image2.png)

Ещё одно такое место:

* 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

## Опечатка \#5

[V501](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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
      || ....)
  {....}
}
```

Одно из выражений каскада условных операторов повторяется\. Всегда есть вероятность того, что опечатка произошла в каком\-нибудь очень важном условии\.

## Идентичные сравнения

[V517](https://pvs-studio.ru/ru/docs/warnings/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

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

Крайне подозрительное место: тут либо переменной 'event' присваивается строка "HangupHandlerRun", либо выполняется выход из функции\.

## Всегда ложь

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression is always false\. Unsigned type value is never < 0\. enum\.c 309

```cpp
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;
  }
}
```

Переменная 'i' имеет беззнаковый тип и никогда не будет меньше нуля\. Функция dn\_expand\(\) возвращает значение \-1 в случае неудачи, тип переменной 'i' не должен быть 'unsigned'\.

## Коварная оптимизация

[V597](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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));      // <=
  ....
}
```

Так как массив 'buf' больше не используется после вызова функции 'memset', то компилятор может удалить вызов функции для оптимизации, и массив не будет обнулён, как планировалось\.

Часто предупреждение V597 остаётся непонятым\. Предлагаю дополнительные материалы, раскрывающие суть проблемы:

* [Перезаписывать память \- зачем?](https://pvs-studio.ru/ru/blog/posts/cpp/k0041/)
* [Zero and forget \-\- caveats of zeroing memory in C](http://web.archive.org/web/20160304012102/http:/www.eliteraspberries.com/blog/2012/10/zero-and-forget--caveats-of-zeroing-memory-in-c.html)

## Указатели

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'object\_wizard\-\>wizard' pointer was utilized before it was verified against nullptr\. Check lines: 683, 686\. sorcery\.c 683

```cpp
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);                       // <=
}
```

Почему\-то здесь присутствует выборочная проверка указателя на ноль\. Обычно такие места говорят, что нулевой указатель всё же может прийти в функцию, следовательно, его нужно проверить во всех местах перед использованием\.

## Избыточный код

Думаю, следующие два примера не являются ошибками, но могут быть упрощены\.

[V584](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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) {  // <=
    ....
  }
}
```

В увеличении аргументов на единицу по обе стороны знака равенства наверняка мало практического смысла\.

[V571](https://pvs-studio.ru/ru/docs/warnings/v571/) Recurring check\. The 'wizard\-\>wizard\-\>retrieve\_fields' condition was already verified in line 1520\. sorcery\.c 1521

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

Не ошибка, но одну проверку указателя явно можно убрать\.

## Заключение

Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач, чем отлов глупых ошибок и опечаток\.

Также об опечатках есть интересная статья: [Эффект последней строки](https://pvs-studio.ru/ru/blog/posts/cpp/0260/)\.