From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 9019A469719 for ; Thu, 24 Sep 2020 12:23:11 +0300 (MSK) Date: Thu, 24 Sep 2020 12:22:50 +0300 From: Sergey Kaplun Message-ID: <20200924092250.GA7363@root> References: <20200921174837.30681-1-skaplun@tarantool.org> <20200923231619.samgq6wp44dhjjdp@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200923231619.samgq6wp44dhjjdp@tkn_work_nb> 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: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review and investigation! On 24.09.20, Alexander Turenko wrote: > 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. I've updated commit message as follows: ``` fiber: src/lua/fiber.c null pointer dereference [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. ``` > > 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've updated corresponding branch as follows: diff --git a/src/lua/fiber.c b/src/lua/fiber.c index bb6212b24..1c7d9cdb6 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 (func && 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) { > > 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 && Thanks again for this investigation! I suppose that we should add a separate ticket for that. -- Best regards, Sergey Kaplun