From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 84E8929439
	for <tarantool-patches@freelists.org>; Fri, 17 Aug 2018 07:17:28 -0400 (EDT)
Received: from turing.freelists.org ([127.0.0.1])
	by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id 43vd88A2AteO for <tarantool-patches@freelists.org>;
	Fri, 17 Aug 2018 07:17:28 -0400 (EDT)
Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 401AD21FFA
	for <tarantool-patches@freelists.org>; Fri, 17 Aug 2018 07:17:28 -0400 (EDT)
Date: Fri, 17 Aug 2018 14:17:27 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 1/2] sql: LIKE & GLOB pattern
 comparison issue
Message-ID: <20180817111727.y6nsbblpm5nh4n3g@tkn_work_nb>
References: <cover.1534436835.git.n.tatunov@tarantool.org>
 <43febf82af3702fadfea135db978ffb6426eb00d.1534436836.git.n.tatunov@tarantool.org>
 <d11b496b-1d77-c1ef-27dd-874835bee1b9@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <d11b496b-1d77-c1ef-27dd-874835bee1b9@tarantool.org>
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-help: <mailto:ecartis@freelists.org?Subject=help>
List-unsubscribe: <tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-subscribe: <tarantool-patches-request@freelists.org?Subject=subscribe>
List-owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-archive: <http://www.freelists.org/archives/tarantool-patches>
To: Alex Khatskevich <avkhatskevich@tarantool.org>
Cc: "N.Tatunov" <n.tatunov@tarantool.org>, tarantool-patches@freelists.org, "N.Tatunov" <hollow653@gmail.com>

Hi!

I have one note here.

Please, cite only relavant parts of a message you reply, because some
clients (say, mutt) don't wrap long cites automatically.

WBR, Alexander Turenko.

On Fri, Aug 17, 2018 at 12:23:16PM +0300, Alex Khatskevich wrote:
> The code seems relatively normal, except for one thing below.
> 
> 
> On 16.08.2018 20:00, N.Tatunov wrote:
> > From: "N.Tatunov" <hollow653@gmail.com>
> > 
> > +#define SQL_END_OF_STRING        0xffff
> I have a look at icu code and It seems like 0xffff is an error, and it is
> more similar to
> invalid symbol that to "end of string". Check it, and fix the code, so that
> it is treated as
> an error.

0xffff is the result of 'end of a string' check as well as internal buffer
overflow error. I have the relevant code pasted in the first review of
the patch (July, 18).

// source/common/ucnv.c::ucnv_getNextUChar
1860     s=*source;
1861     if(sourceLimit<s) {
1862         *err=U_ILLEGAL_ARGUMENT_ERROR;
1863         return 0xffff;
1864     }

We should not handle the buffer overflow case as an invalid symbol. Of
course we should not handle it as the 'end of the string' situation.
Ideally we should perform pointer myself and raise an error in case of
0xffff. I had thought that a buffer overflow error is unlikely to meet,
but you are right: we should differentiate these situations.

In one of the previous version of a patch we perform this check like so:

#define Utf8Read(s, e) (((s) < (e)) ?\
	ucnv_getNextUChar(pUtf8conv, &s, e, &status) : 0)

Don't sure why it was changed. Maybe it is try to correctly handle '\0'
symbol (it is valid unicode character)?

So I see two ways to proceed:

1. Lean on icu's check and ignore possibility of the buffer overflow.
2. Use our own check and possibly meet '\0' problems.
3. Check for U_ILLEGAL_ARGUMENT_ERROR to treat as end of a string, raise
   the error for other 0xffff.

Alex, what do you suggests here?