[tarantool-patches] [PATCH v2] box: fix long uri output in box.info()

[tarantool-patches] [PATCH v2] box: fix long uri output in box.info()

[Serge Petrenko]

lua_pushapplier() had an inexplicably small buffer for uri representation.
Enlarged the buffer. Also uri_format() didn't ensure that its return value
was not greater than the length of string passed to it. Also fixed and
added a test case.

Closes #3630
---
https://github.com/tarantool/tarantool/issues/3630
https://github.com/tarantool/tarantool/tree/sp/gh-3630-long-uri-fix

Changes in v2:
  - fix uri_format() return value.
  - add a unit test for uri_format()

 src/box/lua/info.c   |  2 +-
 src/uri.c            |  3 ++-
 test/unit/uri.c      | 29 ++++++++++++++++++++++++++++-
 test/unit/uri.result |  9 ++++++++-
 4 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index d6697df9c..970c80c99 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -95,7 +95,7 @@ lbox_pushapplier(lua_State *L, struct applier *applier)
 			       applier->last_row_time);
 		lua_settable(L, -3);
 
-		char name[FIBER_NAME_MAX];
+		char name[APPLIER_SOURCE_MAXLEN];
 		int total = uri_format(name, sizeof(name), &applier->uri, false);
 
 		lua_pushstring(L, "peer");
diff --git a/src/uri.c b/src/uri.c
index 941e7bab9..77a1c88f6 100644
--- a/src/uri.c
+++ b/src/uri.c
@@ -6303,6 +6303,7 @@ int
 uri_format(char *str, int len, const struct uri *uri, bool write_password)
 {
 	int total = 0;
+	int maxlen = len - 1;
 	if (uri->scheme_len > 0) {
 		SNPRINT(total, snprintf, str, len, "%.*s://",
 			 (int)uri->scheme_len, uri->scheme);
@@ -6337,7 +6338,7 @@ uri_format(char *str, int len, const struct uri *uri, bool write_password)
 		SNPRINT(total, snprintf, str, len, "#%.*s",
 			(int)uri->fragment_len, uri->fragment);
 	}
-	return total;
+	return MIN(total, maxlen);
 }
 
 /* vim: set ft=ragel: */
diff --git a/test/unit/uri.c b/test/unit/uri.c
index b69f92726..c3c20b176 100644
--- a/test/unit/uri.c
+++ b/test/unit/uri.c
@@ -59,10 +59,35 @@ test_invalid()
 	return check_plan();
 }
 
+int
+test_format()
+{
+	plan(5);
+
+	struct uri uri;
+
+	char str[] = "aurithatslongerthan16symbols:3303";
+	char ret_str[16];
+	char long_str[64];
+	is(uri_parse(&uri, str), 0, "parse successful");
+	is(uri_format(ret_str, sizeof(ret_str), &uri, false),
+	   sizeof(ret_str) - 1, "uri_format returns correct length when"\
+	   " string is too long");
+	is(strncmp(ret_str, str, sizeof(ret_str) - 1), 0, "string is correct");
+	/*
+	 * SNPRINT used in uri_format doesn't count the
+	 * terminating zero. So expected length is sizeof(str) - 1.
+	 */
+	is(uri_format(long_str, sizeof(long_str), &uri, false),
+	   sizeof(str) - 1, "uri_format returns correct length when string fits");
+	is(strcmp(long_str, str), 0, "string is correct");
+	return check_plan();
+}
+
 int
 main(void)
 {
-	plan(63);
+	plan(64);
 
 	/* General */
 	test("host", NULL, NULL, NULL, "host", NULL, NULL, NULL, NULL, 0);
@@ -234,5 +259,7 @@ main(void)
 
 	test_invalid();
 
+	test_format();
+
 	return check_plan();
 }
diff --git a/test/unit/uri.result b/test/unit/uri.result
index 804560b69..7ca9ba2da 100644
--- a/test/unit/uri.result
+++ b/test/unit/uri.result
@@ -1,4 +1,4 @@
-1..63
+1..64
     1..19
     ok 1 - host: parse
     ok 2 - host: scheme
@@ -1305,3 +1305,10 @@ ok 62 - subtests
     ok 1 - empty is invalid
     ok 2 - :// is invalid
 ok 63 - subtests
+    1..5
+    ok 1 - parse successful
+    ok 2 - uri_format returns correct length when string is too long
+    ok 3 - string is correct
+    ok 4 - uri_format returns correct length when string fits
+    ok 5 - string is correct
+ok 64 - subtests
-- 
2.15.2 (Apple Git-101.1)

Re: [tarantool-patches] [PATCH v2] box: fix long uri output in box.info()

[Vladimir Davydov]

On Tue, Aug 21, 2018 at 09:27:47AM +0300, Serge Petrenko wrote:
> diff --git a/src/uri.c b/src/uri.c
> index 941e7bab9..77a1c88f6 100644
> --- a/src/uri.c
> +++ b/src/uri.c
> @@ -6303,6 +6303,7 @@ int
>  uri_format(char *str, int len, const struct uri *uri, bool write_password)
>  {
>  	int total = 0;
> +	int maxlen = len - 1;
>  	if (uri->scheme_len > 0) {
>  		SNPRINT(total, snprintf, str, len, "%.*s://",
>  			 (int)uri->scheme_len, uri->scheme);
> @@ -6337,7 +6338,7 @@ uri_format(char *str, int len, const struct uri *uri, bool write_password)
>  		SNPRINT(total, snprintf, str, len, "#%.*s",
>  			(int)uri->fragment_len, uri->fragment);
>  	}
> -	return total;
> +	return MIN(total, maxlen);

This is incorrect.

uri_format() should always return the would-be length of the uri string,
even if there's not enough space in the buffer. This is consistent with
snprintf() and this allows the caller to estimate the target buffer size
by calling the function without a buffer:

	int buf_size = uri_format(NULL, 0, uri, false);
	char *buf = malloc(buf_size);

That said, you should fix lbox_pushapplier() instead.

Re: [tarantool-patches] [PATCH v2] box: fix long uri output in box.info()

[Serge Petrenko]

> 21 авг. 2018 г., в 16:45, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Tue, Aug 21, 2018 at 09:27:47AM +0300, Serge Petrenko wrote:
>> diff --git a/src/uri.c b/src/uri.c
>> index 941e7bab9..77a1c88f6 100644
>> --- a/src/uri.c
>> +++ b/src/uri.c
>> @@ -6303,6 +6303,7 @@ int
>> uri_format(char *str, int len, const struct uri *uri, bool write_password)
>> {
>> 	int total = 0;
>> +	int maxlen = len - 1;
>> 	if (uri->scheme_len > 0) {
>> 		SNPRINT(total, snprintf, str, len, "%.*s://",
>> 			 (int)uri->scheme_len, uri->scheme);
>> @@ -6337,7 +6338,7 @@ uri_format(char *str, int len, const struct uri *uri, bool write_password)
>> 		SNPRINT(total, snprintf, str, len, "#%.*s",
>> 			(int)uri->fragment_len, uri->fragment);
>> 	}
>> -	return total;
>> +	return MIN(total, maxlen);
> 
> This is incorrect.
> 
> uri_format() should always return the would-be length of the uri string,
> even if there's not enough space in the buffer. This is consistent with
> snprintf() and this allows the caller to estimate the target buffer size
> by calling the function without a buffer:
> 
> 	int buf_size = uri_format(NULL, 0, uri, false);
> 	char *buf = malloc(buf_size);
> 
> That said, you should fix lbox_pushapplier() instead.

Fixed.

---
 src/box/lua/info.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index d6697df9c..d9ea73a64 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -95,9 +95,14 @@ lbox_pushapplier(lua_State *L, struct applier *applier)
 			       applier->last_row_time);
 		lua_settable(L, -3);
 
-		char name[FIBER_NAME_MAX];
+		char name[APPLIER_SOURCE_MAXLEN];
 		int total = uri_format(name, sizeof(name), &applier->uri, false);
-
+		/*
+		 * total can be greater than sizeof(name) if
+		 * name has insufficient length. Terminating
+		 * zero is ignored by lua_pushlstring.
+		 */
+		total = MIN(total, (int)sizeof(name) - 1);
 		lua_pushstring(L, "peer");
 		lua_pushlstring(L, name, total);
 		lua_settable(L, -3);
-- 
2.15.2 (Apple Git-101.1)

Re: [tarantool-patches] [PATCH v2] box: fix long uri output in box.info()

[Vladimir Davydov]

Pushed to 1.9