[asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang

Tue Mar 31 21:14:47 CDT 2015

> On March 31, 2015, 6:21 p.m., rmudgett wrote:
> > /branches/13/main/utils.c, lines 492-493
> > <https://reviewboard.asterisk.org/r/4555/diff/5/?file=73346#file73346line492>
> >
> >     Revert this.  This change could affect the callers of the function since you are changing the API.  However, no callers currently care about the return value.
> >     
> >     You changed this because clang had a problem in test_semi1() in tests/test_strings.c.  There is a better way.
> 
> Diederik de Groot wrote:
>     Actually as i tried to mention in the description i changed this according to the tests being run. I was not sure which one is supposed to be leading, the test or the source. 
>     
>     The test says it expects:
>     test_semi(";", "", 0)
>     To be legal. FOr it to be legal ast_alloca(0) must be allowed.
>     
>     Am i really changing the API here ?

You are changing what is returned if buflen is zero.  Before it would return outbuf while the change makes it return NULL.  It is just fortunate that nothing cares about the return value.

The test code is in error here because for a zero length it writes beyond the buffer boundary.

> On March 31, 2015, 6:21 p.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, lines 395-396
> > <https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line395>
> >
> >     Is clang returning a NULL pointer when passed a zero length?  If so this could be a problem througout the codebase since the code assumes that the function can never fail.
> 
> Diederik de Groot wrote:
>     I am not sure what happens, the compiler actually segfaults when it encountered this one. I was a little shocked about it as well. I guess the __builtins are a little more scary. If you wanna find out, give it a try. I think this should at least be reported to the llvm/clang team.
>     
>     I am not sure how to interpret alloca(0) either... What is the developer trying to achieve here. And what should it return. It can't allocate space of 0 length and return a pointer to it. 
>     
>     A simple but ugly temp-fix would be to change 
>     
>     #define ast_alloca(size) __builtin_alloca(size)
>     
>     to:
>     
>     #if defined(__clang)
>     #define ast_alloca(size) __builtin_alloca(size ? size : 1);
>     #else
>     #define ast_alloca(size) __builtin_alloca(size)
>     #endif

That could be an undefined area.  What is the return value of malloc(0)?  Some systems return NULL while other systems return a pointer that you cannot dereference but can later pass to free().

The problem with the original test_semi() code is that with a zero length we dereference any returned pointer to put a '\0' in it.  For a zero length buffer that is beyond the bounds of the buffer and will corrupt the stack.  The code also reads beyond the boundary in strcmp().

- rmudgett

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4555/#review15000
-----------------------------------------------------------

On March 31, 2015, 8:30 p.m., Diederik de Groot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> -----------------------------------------------------------
> 
> (Updated March 31, 2015, 8:30 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24917
>     https://issues.asterisk.org/jira/browse/ASTERISK-24917
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. 
> 
> fixes for tests to be compiled using clang
> 
> 
> Diffs
> -----
> 
>   /branches/13/tests/test_strings.c 433444 
>   /branches/13/tests/test_stringfields.c 433444 
>   /branches/13/tests/test_sched.c 433444 
>   /branches/13/tests/test_acl.c 433444 
> 
> Diff: https://reviewboard.asterisk.org/r/4555/diff/
> 
> 
> Testing
> -------
> 
> executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) -> 
> test show results failed:
> 
> 
> =================== /main/message/ ====== 
> FAIL   test_message_queue_handler_nom /main/message/             31036ms
> [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message
> 
> Not sure if this is actually a fail or just a timeout. WIP
> 
> 
> =================== /main/strings/ ====== 
> FAIL   escape_semicolons              /main/strings/             1ms     
> [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL && outbuf != NULL (0)
> -> explainable by the change made to the source. ast_alloca(0) is not being executed -> test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes.
> 
> (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though)
> 
> 
> =================== /main/stdtime ====== 
> "test execute all" fails, caused by the /main/stdtime/ test. 
> START  /main/stdtime/ - timezone_watch 
> [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test...
> j62747*CLI> 
> CLI becomes unresponsive / no further command completion for example.
> Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct.
> 
> Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP
> 
> 
> File Attachments
> ----------------
> 
> tests results xml (except /main/stdtime)
>   https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150401/720dcfac/attachment-0001.html>