[Tarantool-patches] [PATCH] lua: handle uri.format empty input properly

[Tarantool-patches] [PATCH] lua: handle uri.format empty input properly

[olegrok]

From: Oleg Babin <babinoleg@mail.ru>

After 7fd6c8096343ca254fec10c2ce8a8233ebe47028
(buffer: port static allocator to Lua) uri started to use
static_allocator - cyclic buffer that also is used in
several modules.

However situation then uri.format output is zero-length
string was not handled properly and ffi.string could
return data that was previously written in static buffer
because use as string terminator the first zero byte.

To prevent such situation let's pass result length explicitly.

Closes #4779
---
Issue: https://github.com/tarantool/tarantool/issues/4779
Branch: https://github.com/tarantool/tarantool/tree/olegrok/4779-uri-format-junk-output
 src/lua/uri.lua           |  4 ++--
 test/app-tap/uri.test.lua | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/lua/uri.lua b/src/lua/uri.lua
index 5967c8bf2..e08e20675 100644
--- a/src/lua/uri.lua
+++ b/src/lua/uri.lua
@@ -77,8 +77,8 @@ local function format(uri, write_password)
     uribuf.fragment = uri.fragment
     uribuf.fragment_len = string.len(uri.fragment or '')
     local str = static_alloc('char', 1024)
-    builtin.uri_format(str, 1024, uribuf, write_password and 1 or 0)
-    return ffi.string(str)
+    local len = builtin.uri_format(str, 1024, uribuf, write_password and 1 or 0)
+    return ffi.string(str, len)
 end
 
 return {
diff --git a/test/app-tap/uri.test.lua b/test/app-tap/uri.test.lua
index 5405b0873..27efe6bc6 100755
--- a/test/app-tap/uri.test.lua
+++ b/test/app-tap/uri.test.lua
@@ -2,6 +2,8 @@
 
 local tap = require('tap')
 local uri = require('uri')
+local ffi = require('ffi')
+local static_alloc = require('buffer').static_alloc
 
 local function test_parse(test)
     -- Tests for uri.parse() Lua bindings.
@@ -64,8 +66,22 @@ local function test_format(test)
     test:is(uri.format(u, true), "user:password@localhost", "password kept")
 end
 
+local function test_static_alloc(test)
+    -- gh-4779 uri.format returns junk output
+    local buffer_size = 12288
+    local buf = static_alloc('char', buffer_size)
+    ffi.fill(buf, 512, string.byte('x'))
+
+    local iterations = 10
+    test:plan(iterations)
+    for _ = 1, iterations do
+        test:is(uri.format({}), '')
+    end
+end
+
 tap.test("uri", function(test)
-    test:plan(2)
+    test:plan(3)
     test:test("parse", test_parse)
     test:test("format", test_format)
+    test:test("static_alloc", test_static_alloc)
 end)
-- 
2.23.0

Re: [Tarantool-patches] [PATCH] lua: handle uri.format empty input properly

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 15/02/2020 10:31, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> After 7fd6c8096343ca254fec10c2ce8a8233ebe47028
> (buffer: port static allocator to Lua) uri started to use
> static_allocator - cyclic buffer that also is used in
> several modules.

Yeah, my fault, sorry.

> However situation then uri.format output is zero-length

then -> when.

> string was not handled properly and ffi.string could
> return data that was previously written in static buffer
> because use as string terminator the first zero byte.
> 
> To prevent such situation let's pass result length explicitly.
> 
> Closes #4779
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4779
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/4779-uri-format-junk-output
> diff --git a/test/app-tap/uri.test.lua b/test/app-tap/uri.test.lua
> index 5405b0873..27efe6bc6 100755
> --- a/test/app-tap/uri.test.lua
> +++ b/test/app-tap/uri.test.lua
> @@ -64,8 +66,22 @@ local function test_format(test)
>      test:is(uri.format(u, true), "user:password@localhost", "password kept")
>  end
>  
> +local function test_static_alloc(test)
> +    -- gh-4779 uri.format returns junk output

Lets add a dot to the end of the sentence, and elaborate
a little about why do we call static_alloc() before this
test and fill it with something. You and me are in the context,
but for someone, who doesn't know the issue and looks at this
test it may look strange.

Otherwise the patch is good.

> +    local buffer_size = 12288
> +    local buf = static_alloc('char', buffer_size)
> +    ffi.fill(buf, 512, string.byte('x'))
> +
> +    local iterations = 10
> +    test:plan(iterations)
> +    for _ = 1, iterations do
> +        test:is(uri.format({}), '')
> +    end
> +end
> +
>  tap.test("uri", function(test)
> -    test:plan(2)
> +    test:plan(3)
>      test:test("parse", test_parse)
>      test:test("format", test_format)
> +    test:test("static_alloc", test_static_alloc)
>  end)
> 

Re: [Tarantool-patches] [PATCH] lua: handle uri.format empty input properly

[Oleg Babin]

Hi! Thanks for your review!

On 15/02/2020 19:15, Vladislav Shpilevoy wrote:
>> However situation then uri.format output is zero-length
> 
> then -> when.
>

Fixed. Thanks. I've updated commit message.

> Lets add a dot to the end of the sentence, and elaborate
> a little about why do we call static_alloc() before this
> test and fill it with something. You and me are in the context,
> but for someone, who doesn't know the issue and looks at this
> test it may look strange.

Yes. I've added following comment:
     -- As static allocator is also used in several places
     -- we should properly handle situation when output
     -- is zero-length string.
     -- Here we allocate the whole buffer,
     -- fill it with some "junk" bytes and
     -- check that result doesn't contain any of them.

I've force-pushed updates to my branch:
https://github.com/tarantool/tarantool/tree/olegrok/4779-uri-format-junk-output

Re: [Tarantool-patches] [PATCH] lua: handle uri.format empty input properly

[Vladislav Shpilevoy]

LGTM.

On 15/02/2020 18:37, Oleg Babin wrote:
> Hi! Thanks for your review!
> 
> On 15/02/2020 19:15, Vladislav Shpilevoy wrote:
>>> However situation then uri.format output is zero-length
>>
>> then -> when.
>>
> 
> Fixed. Thanks. I've updated commit message.
> 
>> Lets add a dot to the end of the sentence, and elaborate
>> a little about why do we call static_alloc() before this
>> test and fill it with something. You and me are in the context,
>> but for someone, who doesn't know the issue and looks at this
>> test it may look strange.
> 
> Yes. I've added following comment:
>     -- As static allocator is also used in several places
>     -- we should properly handle situation when output
>     -- is zero-length string.
>     -- Here we allocate the whole buffer,
>     -- fill it with some "junk" bytes and
>     -- check that result doesn't contain any of them.
> 
> I've force-pushed updates to my branch:
> https://github.com/tarantool/tarantool/tree/olegrok/4779-uri-format-junk-output

Re: [Tarantool-patches] [PATCH] lua: handle uri.format empty input properly

[Kirill Yukhin]

Hello,

On 15 фев 12:31, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> After 7fd6c8096343ca254fec10c2ce8a8233ebe47028
> (buffer: port static allocator to Lua) uri started to use
> static_allocator - cyclic buffer that also is used in
> several modules.
> 
> However situation then uri.format output is zero-length
> string was not handled properly and ffi.string could
> return data that was previously written in static buffer
> because use as string terminator the first zero byte.
> 
> To prevent such situation let's pass result length explicitly.
> 
> Closes #4779

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

Please, in future accompany your patch w/ a release notes
entry.

Also, please set your e-mail address in git settings to
@tarantool.

--
Regards, Kirill Yukhin