Re: [Tarantool-patches] [PATCH v1] asan: fix leak in AccessDeniedError

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 3 comments below.

On 24.08.2020 10:35, Alexander V. Tikhonov wrote:
> In asan/lsan check found common leaks after strdup() function,
> because of its internal allocations in AccessDeniedError class
> for m_object_name, m_object_type, m_access_type buffers:
> 
>   Indirect leak of 24 byte(s) in 4 object(s) allocated from:
>     #0 0x50b550 in __interceptor_strdup (/tnt/src/tarantool+0x50b550)
>     #1 0xd71a98 in AccessDeniedError::AccessDeniedError(char const*, unsigned int, char const*, char const*, char const*, char const*, bool) /tarantool/src/box/error.cc:309:18
>     #2 0xd71c5b in BuildAccessDeniedError /tarantool/src/box/error.cc:319:14
>     #3 0x567864 in access_check_space /tarantool/src/box/space.c:91:5
>     #4 0x55e58b in check_index(unsigned int, unsigned int, space**, index**) /tarantool/src/box/index.cc:172:6
>     #5 0x55e58b in box_index_max /tarantool/src/box/index.cc:296
>     #6 0x2abfea88  (<unknown module>)
> 
> To fix the found issues better to use local memory allocation in stack
> for these buffers. In the same situation in a common CustomError class
> m_custom_type buffer was locally allocated with 64 size. So the buffers
> were changed from strdup() function internal allocation to local setup
> with the same size.
> 
> Suppresion "leak:AccessDeniedError::AccessDeniedError" removed from

1. Suppresion -> Suppression.

> asan suppressions file.
> 
> Part of #4360
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/asan-access-fix
> Issue: https://github.com/tarantool/tarantool/issues/4360
> 
>  asan/lsan.supp   |  6 ------
>  src/box/error.cc | 10 +++++++---
>  src/box/error.h  |  9 +++------
>  3 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/asan/lsan.supp b/asan/lsan.supp
> index 1e297d999..1275b7d0e 100644
> --- a/asan/lsan.supp
> +++ b/asan/lsan.supp
> @@ -30,12 +30,6 @@ leak:gconv_init
>  # source: third_party/luajit
>  leak:lj_BC_FUNCC
>  
> -# test: box/access.test.lua
> -# test: box/access_bin.test.lua
> -# test: box/access_misc.test.lua
> -# source: src/box/error.cc
> -leak:AccessDeniedError::AccessDeniedError
> -
>  # test: box/bitset.test.lua
>  # source: src/lib/bitset/iterator.c
>  leak:tt_bitset_iterator_init
> diff --git a/src/box/error.cc b/src/box/error.cc
> index c3c2af3ab..4e112cc50 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -304,9 +304,13 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
>  	 */
>  	if (run_trigers)
>  		trigger_run(&on_access_denied, (void *) &ctx);
> -	m_object_type = strdup(object_type);
> -	m_access_type = strdup(access_type);
> -	m_object_name = strdup(object_name);
> +       strncpy(m_object_type, object_type, sizeof(m_object_type) - 1);
> +       m_object_type[sizeof(m_object_type) - 1] = '\0';
> +       strncpy(m_access_type, access_type, sizeof(m_access_type) - 1);
> +       m_access_type[sizeof(m_access_type) - 1] = '\0';
> +       strncpy(m_object_name, object_name, sizeof(m_object_name) - 1);
> +       m_object_name[sizeof(m_object_name) - 1] = '\0';

2. Please, use tabs, not whitespaces.

> +
>  }
>  
>  struct error *
> diff --git a/src/box/error.h b/src/box/error.h
> index 988b98255..4c61ed74d 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -246,9 +246,6 @@ public:
>  
>  	~AccessDeniedError()
>  	{
> -		free(m_object_name);
> -		free(m_object_type);
> -		free(m_access_type);
>  	}
>  
>  	const char *
> @@ -271,11 +268,11 @@ public:
>  
>  private:
>  	/** Type of object the required access was denied to */
> -	char *m_object_type;
> +       char m_object_type[64];
>  	/** Name of object the required access was denied to */
> -	char *m_object_name;
> +       char m_object_name[64];
>  	/** Type of declined access */
> -	char *m_access_type;
> +       char m_access_type[64];

3. It does not look like a good idea. CustomError uses fixed size buffer for
error message, which is considered not as important as error object attributes.
We can't allocate everything inside the error object structure. What will
happen when CustomError gets payload? It definitely won't fit into a fixed buffer.

It is necessary to investigate deeper, why the destructor wasn't called. Any why
ASAN does not consider the AccessDeniedError object leaked itself, because it is
also on the heap.

>  };
>  
>  /**
>