From 0cd09774540a79392034007da76f9a63b95ca63f Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Fri, 23 Aug 2013 16:07:18 +0000 Subject: [PATCH] Fix memory corruption when trying to get "core show locks". Review https://reviewboard.asterisk.org/r/2580/ tried to fix the mismatch in memory pools but had a math error determining the buffer size and didn't address other similar memory pool mismatches. * Effectively reverted the previous patch to go in the same direction as trunk for the returned memory pool of ast_bt_get_symbols(). * Fixed memory leak in ast_bt_get_symbols() when BETTER_BACKTRACES is defined. * Fixed some formatting in ast_bt_get_symbols(). * Fixed sig_pri.c freeing memory allocated by libpri when MALLOC_DEBUG is enabled. * Fixed __dump_backtrace() freeing memory from ast_bt_get_symbols() when MALLOC_DEBUG is enabled. * Moved __dump_backtrace() because of compile issues with the utils directory. (closes issue ASTERISK-22221) Reported by: Matt Jordan Review: https://reviewboard.asterisk.org/r/2778/ ........ Merged revisions 397525 from http://svn.asterisk.org/svn/asterisk/branches/1.8 git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/11@397528 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- channels/sig_pri.c | 2 +- include/asterisk/astmm.h | 6 +++ include/asterisk/lock.h | 16 -------- include/asterisk/logger.h | 2 +- include/asterisk/utils.h | 16 +++----- main/astmm.c | 25 ++++++++++++ main/astobj2.c | 2 +- main/lock.c | 17 +++++++++ main/logger.c | 80 ++++++++++++++++----------------------- main/utils.c | 2 +- 10 files changed, 90 insertions(+), 78 deletions(-) diff --git a/channels/sig_pri.c b/channels/sig_pri.c index be8dfb4d33..e5fe18de63 100644 --- a/channels/sig_pri.c +++ b/channels/sig_pri.c @@ -9286,7 +9286,7 @@ void sig_pri_cli_show_span(int fd, int *dchannels, struct sig_pri_span *pri) info_str = pri_dump_info_str(pri->pri); if (info_str) { ast_cli(fd, "%s", info_str); - free(info_str); + ast_std_free(info_str); } #else pri_dump_info(pri->pri); diff --git a/include/asterisk/astmm.h b/include/asterisk/astmm.h index c2a717550e..1b008120a7 100644 --- a/include/asterisk/astmm.h +++ b/include/asterisk/astmm.h @@ -54,6 +54,12 @@ extern "C" { #undef vasprintf #undef free +void *ast_std_malloc(size_t size); +void *ast_std_calloc(size_t nmemb, size_t size); +void *ast_std_realloc(void *ptr, size_t size); +void ast_std_free(void *ptr); +void ast_free_ptr(void *ptr); + void *__ast_calloc(size_t nmemb, size_t size, const char *file, int lineno, const char *func); void *__ast_calloc_cache(size_t nmemb, size_t size, const char *file, int lineno, const char *func); void *__ast_malloc(size_t size, const char *file, int lineno, const char *func); diff --git a/include/asterisk/lock.h b/include/asterisk/lock.h index 677bfba6d0..081f8a8588 100644 --- a/include/asterisk/lock.h +++ b/include/asterisk/lock.h @@ -289,22 +289,6 @@ void ast_remove_lock_info(void *lock_addr); #endif /* HAVE_BKTR */ #endif /* !defined(LOW_MEMORY) */ -#ifdef HAVE_BKTR -static inline void __dump_backtrace(struct ast_bt *bt, int canlog) -{ - char **strings; - - ssize_t i; - - strings = backtrace_symbols(bt->addresses, bt->num_frames); - - for (i = 0; i < bt->num_frames; i++) - __ast_mutex_logger("%s\n", strings[i]); - - free(strings); -} -#endif - /*! * \brief log info for the current lock with ast_log(). * diff --git a/include/asterisk/logger.h b/include/asterisk/logger.h index fc70a815ac..6c2291dc55 100644 --- a/include/asterisk/logger.h +++ b/include/asterisk/logger.h @@ -411,7 +411,7 @@ void *ast_bt_destroy(struct ast_bt *bt); * \param addresses A list of addresses, such as the ->addresses structure element of struct ast_bt. * \param num_frames Number of addresses in the addresses list * \retval NULL Unable to allocate memory - * \return List of strings. Free the entire list with a single ast_free call. + * \return List of strings. Free the entire list with a single ast_std_free call. * \since 1.6.2.16 */ char **ast_bt_get_symbols(void **addresses, size_t num_frames); diff --git a/include/asterisk/utils.h b/include/asterisk/utils.h index 6c36eaa22c..b5ea160532 100644 --- a/include/asterisk/utils.h +++ b/include/asterisk/utils.h @@ -459,24 +459,20 @@ char *ast_process_quotes_and_slashes(char *start, char find, char replace_with); long int ast_random(void); +#ifndef __AST_DEBUG_MALLOC +#define ast_std_malloc malloc +#define ast_std_calloc calloc +#define ast_std_realloc realloc +#define ast_std_free free + /*! * \brief free() wrapper * * ast_free_ptr should be used when a function pointer for free() needs to be passed * as the argument to a function. Otherwise, astmm will cause seg faults. */ -#ifdef __AST_DEBUG_MALLOC -static void ast_free_ptr(void *ptr) attribute_unused; -static void ast_free_ptr(void *ptr) -{ - ast_free(ptr); -} -#else #define ast_free free #define ast_free_ptr ast_free -#endif - -#ifndef __AST_DEBUG_MALLOC #define MALLOC_FAILURE_MSG \ ast_log(LOG_ERROR, "Memory Allocation Failure in function %s at line %d of %s\n", func, lineno, file); diff --git a/main/astmm.c b/main/astmm.c index 9396d09873..f3e1b36290 100644 --- a/main/astmm.c +++ b/main/astmm.c @@ -157,6 +157,31 @@ AST_MUTEX_DEFINE_STATIC_NOTRACKING(reglock); } \ } while (0) +void *ast_std_malloc(size_t size) +{ + return malloc(size); +} + +void *ast_std_calloc(size_t nmemb, size_t size) +{ + return calloc(nmemb, size); +} + +void *ast_std_realloc(void *ptr, size_t size) +{ + return realloc(ptr, size); +} + +void ast_std_free(void *ptr) +{ + free(ptr); +} + +void ast_free_ptr(void *ptr) +{ + ast_free(ptr); +} + /*! * \internal * diff --git a/main/astobj2.c b/main/astobj2.c index 1ab5d3e1c8..761027679b 100644 --- a/main/astobj2.c +++ b/main/astobj2.c @@ -123,7 +123,7 @@ void ao2_bt(void) for(i = 0; i < c; i++) { ast_verbose("%d: %p %s\n", i, addresses[i], strings[i]); } - ast_free(strings); + ast_std_free(strings); } #endif diff --git a/main/lock.c b/main/lock.c index 902d631193..3036e5bc4b 100644 --- a/main/lock.c +++ b/main/lock.c @@ -29,6 +29,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$") +#include "asterisk/utils.h" #include "asterisk/lock.h" /* Allow direct use of pthread_mutex_* / pthread_cond_* */ @@ -45,6 +46,22 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$") #undef pthread_cond_wait #undef pthread_cond_timedwait +#if defined(DEBUG_THREADS) && defined(HAVE_BKTR) +static void __dump_backtrace(struct ast_bt *bt, int canlog) +{ + char **strings; + ssize_t i; + + strings = backtrace_symbols(bt->addresses, bt->num_frames); + + for (i = 0; i < bt->num_frames; i++) { + __ast_mutex_logger("%s\n", strings[i]); + } + + ast_std_free(strings); +} +#endif /* defined(DEBUG_THREADS) && defined(HAVE_BKTR) */ + int __ast_pthread_mutex_init(int tracking, const char *filename, int lineno, const char *func, const char *mutex_name, ast_mutex_t *t) { diff --git a/main/logger.c b/main/logger.c index 2011d250c9..1100f785d5 100644 --- a/main/logger.c +++ b/main/logger.c @@ -1593,7 +1593,7 @@ void *ast_bt_destroy(struct ast_bt *bt) char **ast_bt_get_symbols(void **addresses, size_t num_frames) { - char **strings = NULL; + char **strings; #if defined(BETTER_BACKTRACES) int stackfr; bfd *bfdobj; /* bfd.h */ @@ -1613,9 +1613,12 @@ char **ast_bt_get_symbols(void **addresses, size_t num_frames) #if defined(BETTER_BACKTRACES) strings_size = num_frames * sizeof(*strings); - eachlen = ast_calloc(num_frames, sizeof(*eachlen)); - if (!(strings = ast_calloc(num_frames, sizeof(*strings)))) { + eachlen = ast_calloc(num_frames, sizeof(*eachlen)); + strings = ast_std_calloc(num_frames, sizeof(*strings)); + if (!eachlen || !strings) { + ast_free(eachlen); + ast_std_free(strings); return NULL; } @@ -1630,6 +1633,7 @@ char **ast_bt_get_symbols(void **addresses, size_t num_frames) if (strcmp(dli.dli_fname, "asterisk") == 0) { char asteriskpath[256]; + if (!(dli.dli_fname = ast_utils_which("asterisk", asteriskpath, sizeof(asteriskpath)))) { /* This will fail to find symbols */ ast_debug(1, "Failed to find asterisk binary for debug symbols.\n"); @@ -1638,11 +1642,11 @@ char **ast_bt_get_symbols(void **addresses, size_t num_frames) } lastslash = strrchr(dli.dli_fname, '/'); - if ( (bfdobj = bfd_openr(dli.dli_fname, NULL)) && - bfd_check_format(bfdobj, bfd_object) && - (allocsize = bfd_get_symtab_upper_bound(bfdobj)) > 0 && - (syms = ast_malloc(allocsize)) && - (symbolcount = bfd_canonicalize_symtab(bfdobj, syms))) { + if ((bfdobj = bfd_openr(dli.dli_fname, NULL)) && + bfd_check_format(bfdobj, bfd_object) && + (allocsize = bfd_get_symtab_upper_bound(bfdobj)) > 0 && + (syms = ast_malloc(allocsize)) && + (symbolcount = bfd_canonicalize_symtab(bfdobj, syms))) { if (bfdobj->flags & DYNAMIC) { offset = addresses[stackfr] - dli.dli_fbase; @@ -1651,9 +1655,9 @@ char **ast_bt_get_symbols(void **addresses, size_t num_frames) } for (section = bfdobj->sections; section; section = section->next) { - if ( !bfd_get_section_flags(bfdobj, section) & SEC_ALLOC || - section->vma > offset || - section->size + section->vma < offset) { + if (!bfd_get_section_flags(bfdobj, section) & SEC_ALLOC || + section->vma > offset || + section->size + section->vma < offset) { continue; } @@ -1668,7 +1672,9 @@ char **ast_bt_get_symbols(void **addresses, size_t num_frames) found++; if ((lastslash = strrchr(file, '/'))) { const char *prevslash; - for (prevslash = lastslash - 1; *prevslash != '/' && prevslash >= file; prevslash--); + + for (prevslash = lastslash - 1; *prevslash != '/' && prevslash >= file; prevslash--) { + } if (prevslash >= file) { lastslash = prevslash; } @@ -1690,9 +1696,7 @@ char **ast_bt_get_symbols(void **addresses, size_t num_frames) } if (bfdobj) { bfd_close(bfdobj); - if (syms) { - ast_free(syms); - } + ast_free(syms); } /* Default output, if we cannot find the information within BFD */ @@ -1712,52 +1716,32 @@ char **ast_bt_get_symbols(void **addresses, size_t num_frames) if (!ast_strlen_zero(msg)) { char **tmp; - eachlen[stackfr] = strlen(msg); - if (!(tmp = ast_realloc(strings, strings_size + eachlen[stackfr] + 1))) { - ast_free(strings); + + eachlen[stackfr] = strlen(msg) + 1; + if (!(tmp = ast_std_realloc(strings, strings_size + eachlen[stackfr]))) { + ast_std_free(strings); strings = NULL; break; /* out of stack frame iteration */ } strings = tmp; strings[stackfr] = (char *) strings + strings_size; - ast_copy_string(strings[stackfr], msg, eachlen[stackfr] + 1); - strings_size += eachlen[stackfr] + 1; + strcpy(strings[stackfr], msg);/* Safe since we just allocated the room. */ + strings_size += eachlen[stackfr]; } } if (strings) { - /* Recalculate the offset pointers */ + /* Recalculate the offset pointers because of the reallocs. */ strings[0] = (char *) strings + num_frames * sizeof(*strings); for (stackfr = 1; stackfr < num_frames; stackfr++) { - strings[stackfr] = strings[stackfr - 1] + eachlen[stackfr - 1] + 1; + strings[stackfr] = strings[stackfr - 1] + eachlen[stackfr - 1]; } } + ast_free(eachlen); + #else /* !defined(BETTER_BACKTRACES) */ - if ((strings = backtrace_symbols(addresses, num_frames))) { - /* Re-do value into ast_alloc'ed memory */ - char **ast_strings; - char *p; - unsigned size = num_frames + sizeof(*strings); - int i; - for (i = 0; i < num_frames; ++i) { - size += strlen(strings[i]) + 1; - } -#undef free - if (!(ast_strings = ast_malloc(size))) { - free(strings); - ast_log(LOG_WARNING, "Unable to re-allocate space for backtrace structure\n"); - return NULL; - } - p = (char *) (ast_strings + num_frames); - for (i = 0; i < num_frames; ++i) { - unsigned len = strlen(strings[i]) + 1; - ast_strings[i] = p; - memcpy(p, strings[i], len); - p += len; - } - free(strings); - strings = ast_strings; - } + + strings = backtrace_symbols(addresses, num_frames); #endif /* defined(BETTER_BACKTRACES) */ return strings; } @@ -1782,7 +1766,7 @@ void ast_backtrace(void) ast_debug(1, "#%d: [%p] %s\n", i - 3, bt->addresses[i], strings[i]); } - ast_free(strings); + ast_std_free(strings); } else { ast_debug(1, "Could not allocate memory for backtrace\n"); } diff --git a/main/utils.c b/main/utils.c index e77215b5a9..07669a9d4b 100644 --- a/main/utils.c +++ b/main/utils.c @@ -869,7 +869,7 @@ static void append_backtrace_information(struct ast_str **str, struct ast_bt *bt ast_str_append(str, 0, "\t%s\n", symbols[frame_iterator]); } - ast_free(symbols); + ast_std_free(symbols); } else { ast_str_append(str, 0, "\tCouldn't retrieve backtrace symbols\n"); }