From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 68C91469719
 for <tarantool-patches@dev.tarantool.org>;
 Sat, 15 Feb 2020 19:15:12 +0300 (MSK)
References: <20200215093156.32021-1-olegrok@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <55bad826-7af6-cf0b-b917-743c07d94d62@tarantool.org>
Date: Sat, 15 Feb 2020 17:15:09 +0100
MIME-Version: 1.0
In-Reply-To: <20200215093156.32021-1-olegrok@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH] lua: handle uri.format empty input
	properly
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: olegrok@tarantool.org, tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org
Cc: Oleg Babin <babinoleg@mail.ru>

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)
>