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

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

[Sergey Kaplun]

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.
---

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) {
 		/* We are in the LUA vm. */
 		lua_Debug ar;
 		while (tb_ctx->R && lua_getstack(tb_ctx->R, tb_ctx->lua_frame, &ar) > 0) {
-- 
2.28.0

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

[Sergey Kaplun]

On 21.09.20, 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.
> ---
> 
> 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) {
>  		/* We are in the LUA vm. */
>  		lua_Debug ar;
>  		while (tb_ctx->R && lua_getstack(tb_ctx->R, tb_ctx->lua_frame, &ar) > 0) {
> -- 
> 2.28.0
> 
Since we don't have 'cppcheck' subsystem, I've changed it to 'fiber'.

I've updated commit message to:
```
fiber: src/lua/fiber.c null pointer dereference

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.
```
and pushed it to the branch.


-- 
Best regards,
Sergey Kaplun

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

[Alexander Turenko]

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, "?");

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

[Sergey Kaplun]

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

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

[Alexander Turenko]

On Thu, Sep 24, 2020 at 02:16:20AM +0300, 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.
> 
> 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.

Aside of this point (and the nit below), the patch is okay. LGTM except
those points.

> 
> > ---
> > 
> > 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).

This nit, I meant.

> 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).

Moved to https://github.com/tarantool/tarantool/issues/5326

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

[Sergey Kaplun]

On 24.09.20, Alexander Turenko wrote:
> On Thu, Sep 24, 2020 at 02:16:20AM +0300, Alexander Turenko wrote:
<snipped>
> 
> Moved to https://github.com/tarantool/tarantool/issues/5326

I've added comment about this behaviour:

```
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 1c7d9cdb6..efb0a4921 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -214,6 +214,11 @@ 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;
+        /*
+         * There is impossible to get func == NULL until
+         * https://github.com/tarantool/tarantool/issues/5326
+         * will not resolved, but is possible afterwards.
+         */
         if (func != NULL && strstr(func, "lj_BC_FUNCC") == func) {
                 /* We are in the LUA vm. */
                 lua_Debug ar;
```

You can see other fixes in [1]

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-September/019636.html

-- 
Best regards,
Sergey Kaplun

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

[Kirill Yukhin]

Hello,

On 21 сен 20:48, 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.
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/skaplun/cppcheck-lua-fiber-possible-null-pointer-dereference

I've checked your patch into 1.10, 2.4, 2.5 and master.

--
Regards, Kirill Yukhin