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

[Alexander Turenko <alexander.turenko@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)
+{
+	/*
+	 * <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 &&
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, "?");