[Asterisk-code-review] backtrace: Refactor ast bt get symbols so it doesn't crash (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/10602 )

Change subject: backtrace:  Refactor ast_bt_get_symbols so it doesn't crash
......................................................................


Patch Set 5: Code-Review-1

(10 comments)

https://gerrit.asterisk.org/#/c/10602/5/include/asterisk/utils.h
File include/asterisk/utils.h:

https://gerrit.asterisk.org/#/c/10602/5/include/asterisk/utils.h@517
PS5, Line 517: #define ast_std_strdup strdup
You don't appear to be using this at all.


https://gerrit.asterisk.org/#/c/10602/5/include/asterisk/vector.h
File include/asterisk/vector.h:

https://gerrit.asterisk.org/#/c/10602/5/include/asterisk/vector.h@172
PS5, Line 172: #define AST_VECTOR_STD_PTR_FREE(vec) do { \
You shouldn't need to have this since the vector is Asterisk managed.


https://gerrit.asterisk.org/#/c/10602/5/main/astmm.c
File main/astmm.c:

https://gerrit.asterisk.org/#/c/10602/5/main/astmm.c@178
PS5, Line 178: char *ast_std_strdup(const char *ptr)
You don't appear to be using this at all.


https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c
File main/backtrace.c:

https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@36
PS5, Line 36: #undef malloc
            : #undef calloc
            : #undef strdup
            : #undef free
            : #undef ast_free
            : #undef ast_calloc
            : #define ast_free(x) free(x)
            : #define ast_calloc(n, x) calloc(n, x)
I do not think you need to do this.  Since you are using Asterisk vector code you should let Asterisk handle memory for the vector itself.  In fact since you either create or duplicate the strings put into the vector yourself you don't even need to use the ast_std_xxx() functions for anything put in the vector either.

You only need to use standard allocation memory when dealing with the backtrace library functions themselves.  You have completely encapsulated the access to those functions in this file so the rest of Asterisk can use the normal Asterisk allocation functions.


https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@155
PS5, Line 155: 		AST_VECTOR_APPEND(data->return_strings, strdup(data->msg));
The APPEND can fail since inlined functions can add extra backtrace lines than initially allocated.


https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@174
PS5, Line 174: 	AST_VECTOR_INIT(return_strings, num_frames);
The INIT can fail since you are passing in nonzero num_frames.


https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@198
PS5, Line 198: 		pthread_mutex_lock(&bfd_mutex);
It may be better/safer to lock around the for loop instead.


https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@224
PS5, Line 224: 			data.syms = malloc(allocsize);
Allocation can fail.


https://gerrit.asterisk.org/#/c/10602/5/main/backtrace.c@266
PS5, Line 266: 	strings = backtrace_symbols(addresses, num_frames);
Need to check if strings is NULL before duping the strings in the array.


https://gerrit.asterisk.org/#/c/10602/5/tests/test_pbx.c
File tests/test_pbx.c:

https://gerrit.asterisk.org/#/c/10602/5/tests/test_pbx.c@354
PS5, Line 354: ast_asert()K!!!
What is K?



-- 
To view, visit https://gerrit.asterisk.org/10602
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I79d02862ddaa2423a0809caa4b3b85c128131621
Gerrit-Change-Number: 10602
Gerrit-PatchSet: 5
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 09 Nov 2018 01:14:32 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181108/172696f8/attachment-0001.html>