[tarantool-patches] Re: [PATCH 1/2] sql: LIKE & GLOB pattern comparison issue

[Alex Khatskevich <avkhatskevich@tarantool.org>]

On 17.08.2018 14:17, Alexander Turenko wrote:
> 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)?
The define you have pasted can return 0xffff.
The reasons to change it back are described in the previous patchset.
In short:
1. It is equivalent to
    a. check s < e in a while loop
    b. read next character inside of where loop body.
2. In some usages of the code this check (s<e) was redundant (it was 
performed a couple lines above)
3. There is no reason to rewrite the old version of this function. (So, 
we decided to use old version of the function)
> 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?
As I understand, by now the 0xffff is used ONLY to handle the case of 
unexpectedly ended symbol.
E.g. some symbol consists of 2 characters, but the length of the input 
buffer is 1.
In my opinion this is the same as an invalid symbol.

I guess that internal buffer overflow cannot occur in the 
`ucnv_getNextChar` function.

I suppose that it is Nikitas duty to investigate this problem and 
explain it to us all. I just have noticed a strange usage.