Re: [Tarantool-patches] [PATCH] cppcheck: src/lua/fiber.c null pointer dereference

[Sergey Kaplun <skaplun@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)
> +{
> +	/*
> +	 * <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