From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 10FE4469719 for ; Thu, 24 Sep 2020 02:16:10 +0300 (MSK) Date: Thu, 24 Sep 2020 02:16:19 +0300 From: Alexander Turenko Message-ID: <20200923231619.samgq6wp44dhjjdp@tkn_work_nb> References: <20200921174837.30681-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200921174837.30681-1-skaplun@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] cppcheck: src/lua/fiber.c null pointer dereference List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org On Mon, Sep 21, 2020 at 08:48:37PM +0300, Sergey Kaplun wrote: > Found and fixed possible null pointer dereference with cppcheck: > > [src/lua/fiber.c:245] -> [src/lua/fiber.c:217]: (warning) Either the condition 'if(func)' is redundant or there is possible null pointer dereference: func. Nit: I think there is nothing bad in carrying one-line non-prose text like an error message from some tool. It looks more pretty this way, IMHO. The first question that comes into my mind: whether the NULL dereference may occur before the patch or it is the false positive. It is nice, when it is investigated and described right in the commit message. In fact it influences how we take the patch: whether it is bugfix or refactoring. Bugfixes are usually included into future release notes (it is user visible change), but refactoring usually is not. > --- > > Branch: https://github.com/tarantool/tarantool/tree/skaplun/cppcheck-lua-fiber-possible-null-pointer-dereference > > src/lua/fiber.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/lua/fiber.c b/src/lua/fiber.c > index 45bc03787..bb6212b24 100644 > --- a/src/lua/fiber.c > +++ b/src/lua/fiber.c > @@ -214,7 +214,7 @@ fiber_backtrace_cb(int frameno, void *frameret, const char *func, size_t offset, > { > struct lua_fiber_tb_ctx *tb_ctx = (struct lua_fiber_tb_ctx *)cb_ctx; > struct lua_State *L = tb_ctx->L; > - if (strstr(func, "lj_BC_FUNCC") == func) { > + if (func && strstr(func, "lj_BC_FUNCC") == func) { Nit: We usually explicitly check against NULL (or 0 for an integer type). I was wonder whether 'func' actually can be NULL and found that there is no error handling around unw_get_proc_name(). We ignore its return value and just return content of our thread local buffer (not NULL). I imitated portage splitdebug feature: | objcopy --only-keep-debug --compress-debug-sections src/tarantool src/tarantool.debug | objcopy --add-gnu-debuglink=src/tarantool.debug src/tarantool The backtrace (shown in `require('fiber').info()`) looks so: | - C: '#0 0x555ad3e84f8c in _fini+144396948078589760' | - C: '#1 0x555ad3f41c8b in _fini+144396948078589760' | - C: '#2 0x555ad3f047ee in _fini+144396948078589760' | - C: '#3 0x555ad3e8b649 in _fini+144396948078589760' | - C: '#4 0x555ad3e84270 in _fini+144396948078589760' | - C: '#5 0x555ad3cc2c29 in _fini+144396948078589760' | - C: '#6 0x555ad3ead615 in _fini+144396948078589760' | - C: '#7 0x555ad40bc89a in _fini+144396948078589760' See: different addresses, but the same offset from the same label. Somebody lies... I handled unw_get_proc_name() error (see the patch below) to verify my guess. Got the following backtrace: | - C: '#0 0x565527509f9a in ?' | - C: '#1 0x5655275c707b in ?' | - C: '#2 0x565527589be4 in ?' | - C: '#3 0x565527510657 in ?' | - C: '#4 0x56552750927e in ?' | - C: '#5 0x565527347c29 in ?' | - C: '#6 0x565527532623 in ?' | - C: '#7 0x565527741c8a in ?' ---- The patch to look what is going on when unw_get_proc_name() fails (not self-reviewed, contains debug output as on the 'info' log level): diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc index 456ce9a4d..e063193bd 100644 --- a/src/lib/core/backtrace.cc +++ b/src/lib/core/backtrace.cc @@ -74,16 +74,65 @@ backtrace_proc_cache_clear(void) proc_cache = NULL; } +static void +say_unw_get_proc_name_status(int rc) +{ + /* + * from + * ${LIBUNWIND_SRC}/include/libunwind-common.h.in + */ + switch (-rc) { + case UNW_ESUCCESS: + /* No-op. */ + break; + case UNW_EUNSPEC: + say_info("DEBUG: unw_get_proc_name: UNW_EUNSPEC (%d)", rc); + break; + case UNW_ENOMEM: + say_info("DEBUG: unw_get_proc_name: UNW_ENOMEM (%d)", rc); + break; + case UNW_EBADREG: + say_info("DEBUG: unw_get_proc_name: UNW_EBADREG (%d)", rc); + break; + case UNW_EREADONLYREG: + say_info("DEBUG: unw_get_proc_name: UNW_EREADONLYREG (%d)", rc); + break; + case UNW_ESTOPUNWIND: + say_info("DEBUG: unw_get_proc_name: UNW_ESTOPUNWIND (%d)", rc); + break; + case UNW_EINVALIDIP: + say_info("DEBUG: unw_get_proc_name: UNW_EINVALIDIP (%d)", rc); + break; + case UNW_EBADFRAME: + say_info("DEBUG: unw_get_proc_name: UNW_EBADFRAME (%d)", rc); + break; + case UNW_EINVAL: + say_info("DEBUG: unw_get_proc_name: UNW_EINVAL (%d)", rc); + break; + case UNW_EBADVERSION: + say_info("DEBUG: unw_get_proc_name: UNW_EBADVERSION (%d)", rc); + break; + case UNW_ENOINFO: + say_info("DEBUG: unw_get_proc_name: UNW_ENOINFO (%d)", rc); + break; + default: + say_info("DEBUG: unw_get_proc_name: unknown retval: %d", rc); + } +} + const char * get_proc_name(unw_cursor_t *unw_cur, unw_word_t *offset, bool skip_cache) { static __thread char proc_name[BACKTRACE_NAME_MAX]; unw_word_t ip; unw_get_reg(unw_cur, UNW_REG_IP, &ip); + int rc = INT_MAX; if (skip_cache) { - unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name), - offset); + rc = unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name), + offset); + if (rc != UNW_ESUCCESS) + goto error; return proc_name; } @@ -95,9 +144,11 @@ get_proc_name(unw_cursor_t *unw_cur, unw_word_t *offset, bool skip_cache) region_create(&cache_region, &cord()->slabc); proc_cache = mh_i64ptr_new(); if (proc_cache == NULL) { - unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name), - offset); - goto error; + rc = unw_get_proc_name(unw_cur, proc_name, + sizeof(proc_name), offset); + if (rc != UNW_ESUCCESS) + goto error; + return proc_name; } } @@ -108,8 +159,10 @@ get_proc_name(unw_cursor_t *unw_cur, unw_word_t *offset, bool skip_cache) snprintf(proc_name, BACKTRACE_NAME_MAX, "%s", entry->name); *offset = entry->offset; } else { - unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name), - offset); + rc = unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name), + offset); + if (rc != UNW_ESUCCESS) + goto error; size_t size; entry = region_alloc_object(&cache_region, typeof(*entry), &size); @@ -126,8 +179,10 @@ get_proc_name(unw_cursor_t *unw_cur, unw_word_t *offset, bool skip_cache) goto error; } } -error: return proc_name; +error: + say_unw_get_proc_name_status(rc); + return NULL; } char * @@ -158,9 +213,11 @@ backtrace(void) p += snprintf(p, end - p, "#%-2d %p in ", frame_no, (void *)ip); if (p >= end) goto out; - p += snprintf(p, end - p, "%s+%lx", proc, (long)offset); - if (p >= end) - goto out; + if (proc != NULL) { + p += snprintf(p, end - p, "%s+%lx", proc, (long)offset); + if (p >= end) + goto out; + } p += snprintf(p, end - p, CRLF); if (p >= end) goto out; @@ -411,9 +468,11 @@ backtrace_foreach(backtrace_cb cb, coro_context *coro_ctx, void *cb_ctx) } proc = get_proc_name(&unw_cur, &offset, false); - char *cxxname = abi::__cxa_demangle(proc, demangle_buf, - &demangle_buf_len, - &demangle_status); + char *cxxname = NULL; + if (proc != NULL) + cxxname = abi::__cxa_demangle(proc, demangle_buf, + &demangle_buf_len, + &demangle_status); if (cxxname != NULL) demangle_buf = cxxname; if (frame_no > 0 && diff --git a/src/lua/fiber.c b/src/lua/fiber.c index 45bc03787..366bc9d53 100644 --- a/src/lua/fiber.c +++ b/src/lua/fiber.c @@ -214,7 +214,7 @@ fiber_backtrace_cb(int frameno, void *frameret, const char *func, size_t offset, { struct lua_fiber_tb_ctx *tb_ctx = (struct lua_fiber_tb_ctx *)cb_ctx; struct lua_State *L = tb_ctx->L; - if (strstr(func, "lj_BC_FUNCC") == func) { + if (func != NULL && strstr(func, "lj_BC_FUNCC") == func) { /* We are in the LUA vm. */ lua_Debug ar; while (tb_ctx->R && lua_getstack(tb_ctx->R, tb_ctx->lua_frame, &ar) > 0) { @@ -242,7 +242,7 @@ fiber_backtrace_cb(int frameno, void *frameret, const char *func, size_t offset, } char buf[512]; int l = snprintf(buf, sizeof(buf), "#%-2d %p in ", frameno, frameret); - if (func) + if (func != NULL) snprintf(buf + l, sizeof(buf) - l, "%s+%zu", func, offset); else snprintf(buf + l, sizeof(buf) - l, "?");