[Tarantool-patches] [PATCH 1/1] app: handle concatenated argv name-value correctly

[Tarantool-patches] [PATCH 1/1] app: handle concatenated argv name-value correctly

[Vladislav Shpilevoy]

The server used to crash when any option argument was passed with
a value concatenated to it, like this: '-lvalue', '-evalue'
instead of '-l value' and '-e value'.

However this is a valid way of writing values, and it should not
have crashed regardless of its validity.

The bug was in usage of 'optind' global variable from getopt()
function family. It is not supposed to be used for getting an
option's value. It points to a next argv to parse. Next argv !=
value of current argv, like it was with '-lvalue' and '-evalue'.

For getting a current value there is a variable 'optarg'.

Closes #4775
---
Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4775-crash-on-l-e-opts
Issue: https://github.com/tarantool/tarantool/issues/4775

@ChangeLog
- Fixed crash at attempt to use -e and -l command line options
  concatenated with their values, like this: -eprint(100)
  (gh-4775).

 src/lua/init.c                           |  4 ++--
 src/lua/init.h                           |  2 +-
 src/main.cc                              | 13 +++++--------
 test/app/gh-4775-crash-args-l-e.result   | 15 +++++++++++++++
 test/app/gh-4775-crash-args-l-e.test.lua |  6 ++++++
 5 files changed, 29 insertions(+), 11 deletions(-)
 create mode 100644 test/app/gh-4775-crash-args-l-e.result
 create mode 100644 test/app/gh-4775-crash-args-l-e.test.lua

diff --git a/src/lua/init.c b/src/lua/init.c
index 097dd8495..28b6b2d62 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -557,7 +557,7 @@ run_script_f(va_list ap)
 	const char *path = va_arg(ap, const char *);
 	bool interactive = va_arg(ap, int);
 	int optc = va_arg(ap, int);
-	char **optv = va_arg(ap, char **);
+	const char **optv = va_arg(ap, const char **);
 	int argc = va_arg(ap, int);
 	char **argv = va_arg(ap, char **);
 	/*
@@ -660,7 +660,7 @@ error:
 
 int
 tarantool_lua_run_script(char *path, bool interactive,
-			 int optc, char **optv, int argc, char **argv)
+			 int optc, const char **optv, int argc, char **argv)
 {
 	const char *title = path ? basename(path) : "interactive";
 	/*
diff --git a/src/lua/init.h b/src/lua/init.h
index 507360738..7fc0b1a31 100644
--- a/src/lua/init.h
+++ b/src/lua/init.h
@@ -72,7 +72,7 @@ tarantool_lua_free();
  */
 int
 tarantool_lua_run_script(char *path, bool force_interactive,
-			 int optc, char **optv,
+			 int optc, const char **optv,
 			 int argc, char **argv);
 
 extern char *history;
diff --git a/src/main.cc b/src/main.cc
index e674d85b1..9d1450523 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -722,7 +722,7 @@ main(int argc, char **argv)
 	bool interactive = false;
 	/* Lua interpeter options, e.g. -e and -l */
 	int optc = 0;
-	char **optv = NULL;
+	const char **optv = NULL;
 	auto guard = make_scoped_guard([=]{ if (optc) free(optv); });
 
 	static struct option longopts[] = {
@@ -750,16 +750,13 @@ main(int argc, char **argv)
 		case 'e':
 			/* Save Lua interepter options to optv as is */
 			if (optc == 0) {
-				optv = (char **) calloc(argc, sizeof(char *));
+				optv = (const char **) calloc(argc,
+							      sizeof(optv[0]));
 				if (optv == NULL)
 					panic_syserror("No enough memory for arguments");
 			}
-			/*
-			 * The variable optind is the index of the next
-			 * element to be processed in argv.
-			 */
-			optv[optc++] = argv[optind - 2];
-			optv[optc++] = argv[optind - 1];
+			optv[optc++] = ch == 'l' ? "-l" : "-e";
+			optv[optc++] = optarg;
 			break;
 		default:
 			/* "invalid option" is printed by getopt */
diff --git a/test/app/gh-4775-crash-args-l-e.result b/test/app/gh-4775-crash-args-l-e.result
new file mode 100644
index 000000000..eff1ee763
--- /dev/null
+++ b/test/app/gh-4775-crash-args-l-e.result
@@ -0,0 +1,15 @@
+-- test-run result file version 2
+--
+-- gh-4775: crash on option concatenated with value.
+--
+child = io.popen('tarantool -e"print(100) os.exit()"')
+ | ---
+ | ...
+child:read()
+ | ---
+ | - '100'
+ | ...
+child:close()
+ | ---
+ | - true
+ | ...
diff --git a/test/app/gh-4775-crash-args-l-e.test.lua b/test/app/gh-4775-crash-args-l-e.test.lua
new file mode 100644
index 000000000..1cccb78a4
--- /dev/null
+++ b/test/app/gh-4775-crash-args-l-e.test.lua
@@ -0,0 +1,6 @@
+--
+-- gh-4775: crash on option concatenated with value.
+--
+child = io.popen('tarantool -e"print(100) os.exit()"')
+child:read()
+child:close()
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/1] app: handle concatenated argv name-value correctly

[Igor Munkin]

Vlad,

Thanks for the patch it LGTM.

However, the Travis job failed on the newly added test[1]. It looks like
the wait underneath the pclose has failed. I can't reproduce the problem
on my machine, but I guess the strace output for the failed child:close
might shed some light on the issue. Nevertheless, what do you think
regarding omitting the close call since the resourses will be
automatically released considering the doc[2]?

On 19.02.20, Vladislav Shpilevoy wrote:
> The server used to crash when any option argument was passed with
> a value concatenated to it, like this: '-lvalue', '-evalue'
> instead of '-l value' and '-e value'.
> 
> However this is a valid way of writing values, and it should not
> have crashed regardless of its validity.
> 
> The bug was in usage of 'optind' global variable from getopt()
> function family. It is not supposed to be used for getting an
> option's value. It points to a next argv to parse. Next argv !=
> value of current argv, like it was with '-lvalue' and '-evalue'.
> 
> For getting a current value there is a variable 'optarg'.
> 
> Closes #4775
> ---
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4775-crash-on-l-e-opts
> Issue: https://github.com/tarantool/tarantool/issues/4775
> 
> @ChangeLog
> - Fixed crash at attempt to use -e and -l command line options
>   concatenated with their values, like this: -eprint(100)
>   (gh-4775).
> 
>  src/lua/init.c                           |  4 ++--
>  src/lua/init.h                           |  2 +-
>  src/main.cc                              | 13 +++++--------
>  test/app/gh-4775-crash-args-l-e.result   | 15 +++++++++++++++
>  test/app/gh-4775-crash-args-l-e.test.lua |  6 ++++++
>  5 files changed, 29 insertions(+), 11 deletions(-)
>  create mode 100644 test/app/gh-4775-crash-args-l-e.result
>  create mode 100644 test/app/gh-4775-crash-args-l-e.test.lua
> 
> diff --git a/test/app/gh-4775-crash-args-l-e.result b/test/app/gh-4775-crash-args-l-e.result
> new file mode 100644
> index 000000000..eff1ee763
> --- /dev/null
> +++ b/test/app/gh-4775-crash-args-l-e.result
> @@ -0,0 +1,15 @@
> +-- test-run result file version 2
> +--
> +-- gh-4775: crash on option concatenated with value.
> +--
> +child = io.popen('tarantool -e"print(100) os.exit()"')
> + | ---
> + | ...
> +child:read()
> + | ---
> + | - '100'
> + | ...
> +child:close()
> + | ---
> + | - true
> + | ...
> diff --git a/test/app/gh-4775-crash-args-l-e.test.lua b/test/app/gh-4775-crash-args-l-e.test.lua
> new file mode 100644
> index 000000000..1cccb78a4
> --- /dev/null
> +++ b/test/app/gh-4775-crash-args-l-e.test.lua
> @@ -0,0 +1,6 @@
> +--
> +-- gh-4775: crash on option concatenated with value.
> +--
> +child = io.popen('tarantool -e"print(100) os.exit()"')
> +child:read()
> +child:close()
> -- 
> 2.21.1 (Apple Git-122.3)
> 

[1]: https://travis-ci.org/tarantool/tarantool/jobs/652226987#L4873
[2]: https://www.lua.org/manual/5.1/manual.html#pdf-file:close

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 1/1] app: handle concatenated argv name-value correctly

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 19/02/2020 09:54, Igor Munkin wrote:
> Vlad,
> 
> Thanks for the patch it LGTM.
> 
> However, the Travis job failed on the newly added test[1]. It looks like
> the wait underneath the pclose has failed. I can't reproduce the problem
> on my machine, but I guess the strace output for the failed child:close
> might shed some light on the issue. Nevertheless, what do you think
> regarding omitting the close call since the resourses will be
> automatically released considering the doc[2]?

It is good that you noticed. The problem has nothing to do with Lua as
I found. It is because of libev, which automatically calls wait() every
time when a SIGCHILD is received. We compile with EV_CHILD_ENABLE 1,
which activates that behaviour, and breaks Lua popen. I am not sure
whether it is a bug. But yeah, we can omit :close() I suppose. Then it
will work regardless of EV_CHILD_ENABLE. We probably will need to
disable or properly handle that flag for our own popen.

I changed the test:

================================================================================

diff --git a/test/app/gh-4775-crash-args-l-e.result b/test/app/gh-4775-crash-args-l-e.result
index eff1ee763..88169f700 100644
--- a/test/app/gh-4775-crash-args-l-e.result
+++ b/test/app/gh-4775-crash-args-l-e.result
@@ -9,7 +9,6 @@ child:read()
  | ---
  | - '100'
  | ...
-child:close()
- | ---
- | - true
- | ...
+-- :close() is omitted, because SIGCHILD may be handled by
+-- libev instead of Lua. In that case :close() with fail with
+-- ECHILD, but it does not matter for this test.
diff --git a/test/app/gh-4775-crash-args-l-e.test.lua b/test/app/gh-4775-crash-args-l-e.test.lua
index 1cccb78a4..7dff8e894 100644
--- a/test/app/gh-4775-crash-args-l-e.test.lua
+++ b/test/app/gh-4775-crash-args-l-e.test.lua
@@ -3,4 +3,6 @@
 --
 child = io.popen('tarantool -e"print(100) os.exit()"')
 child:read()
-child:close()
+-- :close() is omitted, because SIGCHILD may be handled by
+-- libev instead of Lua. In that case :close() with fail with
+-- ECHILD, but it does not matter for this test.

Re: [Tarantool-patches] [PATCH 1/1] app: handle concatenated argv name-value correctly

[Kirill Yukhin]

Hello,

On 19 фев 00:08, Vladislav Shpilevoy wrote:
> The server used to crash when any option argument was passed with
> a value concatenated to it, like this: '-lvalue', '-evalue'
> instead of '-l value' and '-e value'.
> 
> However this is a valid way of writing values, and it should not
> have crashed regardless of its validity.
> 
> The bug was in usage of 'optind' global variable from getopt()
> function family. It is not supposed to be used for getting an
> option's value. It points to a next argv to parse. Next argv !=
> value of current argv, like it was with '-lvalue' and '-evalue'.
> 
> For getting a current value there is a variable 'optarg'.
> 
> Closes #4775
> ---
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4775-crash-on-l-e-opts
> Issue: https://github.com/tarantool/tarantool/issues/4775

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

--
Regards, Kirill Yukhin