[Tarantool-patches] [PATCH] cppcheck: src/lua/fiber.c null pointer dereference
Sergey Kaplun
skaplun at tarantool.org
Thu Sep 24 12:22:50 MSK 2020
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)
> +{
> + /*
> + * <unw_error_t> 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 &&
<snipped>
Thanks again for this investigation! I suppose that we should add a
separate ticket for that.
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list