Asterisk: PVS-Studio is interested in telephony

Asterisk is a free open source computer telephony solution from Digium. The application runs on operating systems such as Linux, FreeBSD, OpenBSD, Solaris. In combination with the necessary equipment, Asterisk has all the features of a classic PBX, supports many VoIP protocols, and provides rich call management functions.

This article will look at the results of the Asterisk verification obtained with PVS-Studio 5.18.
')
The project, apparently, is checked by the Coverity analyzer, as evidenced by comments like:

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

However, I noticed some annoying typos. Let's try to understand them and in other suspicious places. The source code is taken from the SVN project repository.

Typo # 1

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

Here it was planned to check the status of channels with numbers 0 and 1, after which they are closed, but due to a typo the status of channel 0 is not checked. The code may work correctly for a long time, because in most cases both channels are involved.

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

At this point, a meaningless comparison of the pointer with zero is performed. Most likely, we wanted to check the timeout_override variable that the sscanf function returned.

Typo # 3

V568 It's odd that the 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 evaluates the type of expression and returns the size of this type, but the expression itself is not evaluated. Complex expressions are a sign of an error. Most often, these errors are associated with typos. As in this example: most likely, multiplication by two should be behind the bracket sizeof () operator.

Typo # 4

V653 A suspicious string 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++) { .... } .... } 

In the declaration of an array of string literals, the two strings are combined into one. The error may be due to a typo when a comma is missing between string literals.

This is how the elements of the token_equivs1 array actually look like:



Another such place:

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

Typo # 5

V501 There are identical sub-expressions 'strcasecmp (item-> u1.str, "endwhile") == 0' 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 cascade expressions of conditional statements is repeated. There is always the possibility that a typo occurred in some very important condition.

Identical comparisons

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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; } .... } 

Extremely suspicious: here either the string “HangupHandlerRun” is assigned to the variable 'event', or the function is exited.

Always a lie

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 variable 'i' is unsigned and will never be less than zero. The dn_expand () function returns the value -1 in case of failure, the type of the variable 'i' should not be 'unsigned'.

Insidious optimization

V597 The compiler couldn’t delete the memset function call, which is used to flush the 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 array 'buf' is no longer used after calling the function 'memset', the compiler can remove the function call for optimization, and the array will not be zeroed as planned.

Often the V597 warning remains misunderstood. I propose additional materials that reveal the essence of the problem:

• Overwrite memory - why?
• Zero and forget - caveats of zeroing memory in C
• Once you read about the volatile keyword ...

Pointers

V595 The 'object_wizard-> wizard' pointer has been 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, there is a random check of the pointer to zero. Usually such places say that a null pointer can still come to a function, therefore, it should be checked in all places before use.

Redundant code

I think the following two examples are not errors, but can be simplified.

V584 The '1' value is the operator. The expression is not correct. 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) { //<== .... } } 

In increasing the arguments by one on both sides of the equal sign, there is probably little practical meaning.

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

Not an error, but one pointer check can be explicitly removed.

Conclusion

Using static analysis regularly, you can save a lot of time on solving more useful tasks than catching silly mistakes and typos.

There is also an interesting article about typos: The effect of the last line .

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Asterisk: PVS-Studio Takes Up Telephony .