> On April 1, 2015, 1:21 a.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 ? > > rmudgett wrote: > 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.
Ok Agreed. > On April 1, 2015, 1:21 a.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 > > rmudgett wrote: > 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(). Wrote a little tester.c #include <malloc.h> #include <stdio.h> #include <strings.h> int main() { int *test1 = malloc(0); int *test2 = __builtin_alloca(0); printf("%p / %p\n", test1, test2); free(test1); } And without any compiler flags, clang and gcc seem to be happy and running this without a problem. Will have to figure out which compiler flags the Makefile compounds to see if that makes any difference. - Diederik ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review15000 ----------------------------------------------------------- On April 1, 2015, 4:47 a.m., Diederik de Groot wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4555/ > ----------------------------------------------------------- > > (Updated April 1, 2015, 4:47 a.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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
