[Tarantool-patches] [PATCH] Fix make_scoped_guard usage

[Tarantool-patches] [PATCH] Fix make_scoped_guard usage

[Aleksandr Lyapunov]

make_scoped_guard usually captures scope by [=].
In some cases it could lead to unexpected results.
I found several problems and fixed them.

GH issue: https://github.com/tarantool/tarantool/issues/5154
GH branch: https://github.com/tarantool/tarantool/tree/alyapunov/gh-5154-wrong-scoped-guards

Aleksandr Lyapunov (1):
  Fix wrong make_scoped_guard usage

 src/box/alter.cc | 4 ++--
 src/main.cc      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.7.4

[Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage

[Aleksandr Lyapunov]

The common pitfall of using a lambda is wrong type of capture -
by value instead of by reference. The simple example is:
  struct sequence_def *new_def = NULL;
  auto def_guard = make_scoped_guard([=] { free(new_def); });
  // initialize new_def
The problem is that the lambda captures pointer by value, that
is NULL and will remain NULL in the lambda while new_def is
successfully initialized in function scope.

The patch fixes the problem above and a couple of similar mistakes.
---
 src/box/alter.cc | 4 ++--
 src/main.cc      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index bb42548..3109fb4 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3560,7 +3560,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 		 * definition to support upgrade script.
 		 */
 		struct func_def *old_def = NULL, *new_def = NULL;
-		auto guard = make_scoped_guard([=] {
+		auto guard = make_scoped_guard([&old_def, &new_def] {
 			free(old_def);
 			free(new_def);
 		});
@@ -4431,7 +4431,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 	struct tuple *new_tuple = stmt->new_tuple;
 
 	struct sequence_def *new_def = NULL;
-	auto def_guard = make_scoped_guard([=] { free(new_def); });
+	auto def_guard = make_scoped_guard([&new_def] { free(new_def); });
 
 	struct sequence *seq;
 	if (old_tuple == NULL && new_tuple != NULL) {		/* INSERT */
diff --git a/src/main.cc b/src/main.cc
index 2c96391..e622042 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -729,7 +729,7 @@ main(int argc, char **argv)
 	/* Lua interpeter options, e.g. -e and -l */
 	int optc = 0;
 	const char **optv = NULL;
-	auto guard = make_scoped_guard([=]{ if (optc) free(optv); });
+	auto guard = make_scoped_guard([&optc, &optv]{ if (optc) free(optv); });
 
 	static struct option longopts[] = {
 		{"help", no_argument, 0, 'h'},
-- 
2.7.4

Re: [Tarantool-patches] [PATCH] Fix make_scoped_guard usage

[Aleksandr Lyapunov]

+reviewers

On 08.07.2020 15:07, Aleksandr Lyapunov wrote:
> make_scoped_guard usually captures scope by [=].
> In some cases it could lead to unexpected results.
> I found several problems and fixed them.
>
> GH issue: https://github.com/tarantool/tarantool/issues/5154
> GH branch: https://github.com/tarantool/tarantool/tree/alyapunov/gh-5154-wrong-scoped-guards
>
> Aleksandr Lyapunov (1):
>    Fix wrong make_scoped_guard usage
>
>   src/box/alter.cc | 4 ++--
>   src/main.cc      | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
>

Re: [Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage

[Timur Safin]

Explicit capture list in lambda - is the correct way, making things clearer!

LGTM

Timur

: -----Original Message-----
: From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
: Behalf Of Aleksandr Lyapunov
: Sent: Wednesday, July 8, 2020 3:07 PM
: To: tarantool-patches@dev.tarantool.org
: Subject: [Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage
: 
: The common pitfall of using a lambda is wrong type of capture -
: by value instead of by reference. The simple example is:
:   struct sequence_def *new_def = NULL;
:   auto def_guard = make_scoped_guard([=] { free(new_def); });
:   // initialize new_def
: The problem is that the lambda captures pointer by value, that
: is NULL and will remain NULL in the lambda while new_def is
: successfully initialized in function scope.
: 
: The patch fixes the problem above and a couple of similar mistakes.
: ---
:  src/box/alter.cc | 4 ++--
:  src/main.cc      | 2 +-
:  2 files changed, 3 insertions(+), 3 deletions(-)
: 
: diff --git a/src/box/alter.cc b/src/box/alter.cc
: index bb42548..3109fb4 100644
: --- a/src/box/alter.cc
: +++ b/src/box/alter.cc
: @@ -3560,7 +3560,7 @@ on_replace_dd_func(struct trigger * /* trigger */,
: void *event)
:  		 * definition to support upgrade script.
:  		 */
:  		struct func_def *old_def = NULL, *new_def = NULL;
: -		auto guard = make_scoped_guard([=] {
: +		auto guard = make_scoped_guard([&old_def, &new_def] {
:  			free(old_def);
:  			free(new_def);
:  		});
: @@ -4431,7 +4431,7 @@ on_replace_dd_sequence(struct trigger * /* trigger
: */, void *event)
:  	struct tuple *new_tuple = stmt->new_tuple;
: 
:  	struct sequence_def *new_def = NULL;
: -	auto def_guard = make_scoped_guard([=] { free(new_def); });
: +	auto def_guard = make_scoped_guard([&new_def] { free(new_def); });
: 
:  	struct sequence *seq;
:  	if (old_tuple == NULL && new_tuple != NULL) {		/* INSERT */
: diff --git a/src/main.cc b/src/main.cc
: index 2c96391..e622042 100644
: --- a/src/main.cc
: +++ b/src/main.cc
: @@ -729,7 +729,7 @@ main(int argc, char **argv)
:  	/* Lua interpeter options, e.g. -e and -l */
:  	int optc = 0;
:  	const char **optv = NULL;
: -	auto guard = make_scoped_guard([=]{ if (optc) free(optv); });
: +	auto guard = make_scoped_guard([&optc, &optv]{ if (optc) free(optv);
: });
: 
:  	static struct option longopts[] = {
:  		{"help", no_argument, 0, 'h'},
: --
: 2.7.4

Re: [Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage

[Cyrill Gorcunov]

On Wed, Jul 08, 2020 at 03:07:07PM +0300, Aleksandr Lyapunov wrote:
> The common pitfall of using a lambda is wrong type of capture -
> by value instead of by reference. The simple example is:
>   struct sequence_def *new_def = NULL;
>   auto def_guard = make_scoped_guard([=] { free(new_def); });
>   // initialize new_def
> The problem is that the lambda captures pointer by value, that
> is NULL and will remain NULL in the lambda while new_def is
> successfully initialized in function scope.
> 
> The patch fixes the problem above and a couple of similar mistakes.

I think we could use shorter [&] form but this is just
a personal preference. Thank you, good catch!

Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH] Fix wrong make_scoped_guard usage

[Nikita Pettik]

On 08 Jul 15:07, Aleksandr Lyapunov wrote:
> The common pitfall of using a lambda is wrong type of capture -
> by value instead of by reference. The simple example is:
>   struct sequence_def *new_def = NULL;
>   auto def_guard = make_scoped_guard([=] { free(new_def); });
>   // initialize new_def
> The problem is that the lambda captures pointer by value, that
> is NULL and will remain NULL in the lambda while new_def is
> successfully initialized in function scope.
> 
> The patch fixes the problem above and a couple of similar mistakes.
> ---

Agree with Cyrill that we could simply use &. Anyway LGTM,
let's push this patch.

>  src/box/alter.cc | 4 ++--
>  src/main.cc      | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index bb42548..3109fb4 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3560,7 +3560,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
>  		 * definition to support upgrade script.
>  		 */
>  		struct func_def *old_def = NULL, *new_def = NULL;
> -		auto guard = make_scoped_guard([=] {
> +		auto guard = make_scoped_guard([&old_def, &new_def] {
>  			free(old_def);
>  			free(new_def);
>  		});
> @@ -4431,7 +4431,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
>  	struct tuple *new_tuple = stmt->new_tuple;
>  
>  	struct sequence_def *new_def = NULL;
> -	auto def_guard = make_scoped_guard([=] { free(new_def); });
> +	auto def_guard = make_scoped_guard([&new_def] { free(new_def); });
>  
>  	struct sequence *seq;
>  	if (old_tuple == NULL && new_tuple != NULL) {		/* INSERT */
> diff --git a/src/main.cc b/src/main.cc
> index 2c96391..e622042 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -729,7 +729,7 @@ main(int argc, char **argv)
>  	/* Lua interpeter options, e.g. -e and -l */
>  	int optc = 0;
>  	const char **optv = NULL;
> -	auto guard = make_scoped_guard([=]{ if (optc) free(optv); });
> +	auto guard = make_scoped_guard([&optc, &optv]{ if (optc) free(optv); });
>  
>  	static struct option longopts[] = {
>  		{"help", no_argument, 0, 'h'},
> -- 
> 2.7.4
> 

Re: [Tarantool-patches] [PATCH] Fix make_scoped_guard usage

[Kirill Yukhin]

Hello,

On 08 июл 15:07, Aleksandr Lyapunov wrote:
> make_scoped_guard usually captures scope by [=].
> In some cases it could lead to unexpected results.
> I found several problems and fixed them.
> 
> GH issue: https://github.com/tarantool/tarantool/issues/5154
> GH branch: https://github.com/tarantool/tarantool/tree/alyapunov/gh-5154-wrong-scoped-guards
> 
> Aleksandr Lyapunov (1):
>   Fix wrong make_scoped_guard usage

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

--
Regards, Kirill Yukhin