On Oct 29, 2018, at 16:01, Alexander Turenko
<alexander.turenko@xxxxxxxxxxxxx> wrote:
The patch is okay except one note re test case.
WBR, Alexander Turenko.
--- EVIDENCE-OF: R-39414-35489 The infix GLOB operator is implemented by
--- calling the function glob(Y,X) and can be modified by overriding that
--- function.
This test case was removed, while we have not a similar one for LIKE.
if(((size_t)(sourceLimit-s)>(size_t)0x7fffffff && sourceLimit>s)) {
*err=U_ILLEGAL_ARGUMENT_ERROR;
return 0xffff;
4. I’m not sure if string data can be this long in our context.
(string length > (size_t) 0x7ffffffff)
Note: not 0x7ffffffff, but 0x7fffffff.
This limit seems to be some weird internal thing related to using
ucnv_getNextUChar inside libicu.
I propose to lie libicu about the buffer size in case when it exceeds
this limit. A UTF-8 encoded symbol is 4 bytes long at max, so we can
pass the following instead of pattern_end:
((size_t) (pattern_end - pattern) > (size_t) 0x7fffffff ? pattern +
0x7fffffff : pattern_end
I think this trick need to be covered with a unit test (because it is
unclear
how to create a string of size >1GiB from lua). Don't sure whether it is
okay to allocate such amount of memory in the test, though...
Please, don't do that within this patch, because it is about the another
bug.
File an issue with all needed information instead (you can provide a link to
this message for example).
Ok, thank you for advice. I think that’s a good idea, but there’s one thing
I’m getting concerned about: it will cause a lot of operations especially
in case we’re using LIKE for scanning a lot of data (). I guess even if it’s
relevant it’s a discussion inside of an issue that’s going to be filed.
Filed https://github.com/tarantool/tarantool/issues/3773
} else if(U_SUCCESS(*err) && c>=0) {
return c;
6. Returns symbol (can also be 0xfffd, as it is not treated as an actual
error).
So if I’m not mistaken we will get results in our function either from
‘return’ number 5 or number 6 and the following code will not be executed.
It is not so. We'll fall through in case of U_ILLEGAL_CHAR_FOUND or
U_TRUNCATED_CHAR_FOUND error.
To be honest I don't want to continue. It seems we should not lean on
the fact that 0xffff always means end of the buffer, because it does not
guaranteed by the API and is not clear from the code.
AFAIR, the problem was to choose appropriate symbol to mark end of the
buffer situation and distinguish it from a real error. It seems we have
not one. So we should fairly (and always) check for the buffer before a
call to ucnv_getNextUChar() or check the status it provide after the
call. I would prefer to check it in our code. It seems that it is how
the API works.
I propose to use the same code pattern for all Utf8Read calls, e.g.:
if (pattern < pattern_end)
c = Utf8Read(pattern, pattern_end)
else
return SQL_...;
if (c == SQL_INVALID_UTF8_SYMBOL)
return SQL_...;
assert(U_SUCCESS(status));
Note: I have added the assert, because it is not clear what we can do
with, say, U_INVALID_TABLE_FORMAT (improper libicu build /
installation). Hope Nikita P. suggests right way, but now I think we
should at least assert on that.
It seems the code above can be even wrapped into a macro that will get
two pointers (pattern and pattern_end / string and string_end) and two
SQL_... error code to handle two possible errors. Yep, it is generally
discouraged to return from a macro, but if it'll greatly improves the
code readability, so it is appropriate, I think. Just define the macro
right before the function and undefne it after to show a reader it is
some pure internal thing.
Note: If you will going that way, don't wrap Utf8Read macro into another
macro. Use one with ucnv_getNextUChar call.
It is refactoring of the code and our of the scope of your issue.
Please, file an issue and link this message into it (but please ask
Nikita P. opinion before).
It is not good IMHO, but it seems now it worth to leave the code with
assumption 0xffff is the end of buffer. This is kind of splitting the
problem into parts and allow us to proceed with this patch re parsing
bug.
Filed https://github.com/tarantool/tarantool/issues/3774
