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

[George Joseph]

George Joseph 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:

(6 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.

Oops, leftover.


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.

astmm can call ast_bt_get_symbols so I didn't want it to be 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.

Ooops.  Leftover.


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.

I didn't want to use the astmm versions because astmm can call ast_bt_get_symbols.


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.

Maybe but only the BFD functions need protecting and I didn't want to hold the lock for any longer than necessary.


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?

An extra letter.



-- 
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: 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 12:33:23 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181109/7134abba/attachment.html>