maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Mar 22, Oleksandr Byelkin wrote:
> revision-id: 4b142536a24 (mariadb-10.2.31-56-g4b142536a24)
> parent(s): ed21202a14e
> author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> timestamp: 2020-03-18 08:25:13 +0100
> message:
> 
> MDEV-20604: Duplicate key value is silently truncated to 64 characters in print_keydup_error
> 
> Added indication of truncated string with special width/precission suffix ':'.

The comment looks wrong :)

> diff --git a/client/mysql_upgrade.c b/client/mysql_upgrade.c
> index 0bbb8a149ff..003701514cf 100644
> --- a/client/mysql_upgrade.c
> +++ b/client/mysql_upgrade.c
> @@ -501,10 +499,10 @@ static void find_tool(char *tool_executable_name, const char *tool_name,
>        last_fn_libchar -= 6;
>      }
>  
> -    len= (int)(last_fn_libchar - self_name);
> -
> -    my_snprintf(tool_executable_name, FN_REFLEN, "%.*s%c%s",
> -                len, self_name, FN_LIBCHAR, tool_name);
> +    last_fn_libchar[0]= 0;
> +    my_snprintf(tool_executable_name, FN_REFLEN, "%s%c%s",
> +                self_name, FN_LIBCHAR, tool_name);
> +    last_fn_libchar[0]= FN_LIBCHAR;

Okay, but why not to use %b here?

>    }
>  
>    if (opt_verbose)
> diff --git a/client/mysqltest.cc b/client/mysqltest.cc
> index 60a203ccedd..045ab566fdb 100644
> --- a/client/mysqltest.cc
> +++ b/client/mysqltest.cc
> @@ -9534,6 +9533,7 @@ int main(int argc, char **argv)
>        case Q_LET: do_let(command); break;
>        case Q_EVAL_RESULT:
>          die("'eval_result' command  is deprecated");
> +	break; // never called but keep compiler calm

wouldn't it be better to specify __attribute__ ((noreturn)) for die()?

>        case Q_EVAL:
>        case Q_EVALP:
>        case Q_QUERY_VERTICAL:
> diff --git a/strings/my_vsnprintf.c b/strings/my_vsnprintf.c
> index ad3517e4252..9ae10f337cc 100644
> --- a/strings/my_vsnprintf.c
> +++ b/strings/my_vsnprintf.c
> @@ -28,7 +28,9 @@
>  #define LENGTH_ARG     1
>  #define WIDTH_ARG      2
>  #define PREZERO_ARG    4
> -#define ESCAPED_ARG    8 
> +#define ESCAPED_ARG    8
> +// Show truncation of string argument
> +#define SH_TRUNC_ARG  16

you don't really need a flag, because process_str_arg should always
do it. By the way, in your code it doesn't. It doesn't print dots
for %M and for %1$s.

>  
>  typedef struct pos_arg_info ARGS_INFO;
>  typedef struct print_info PRINT_INFO;
> @@ -198,18 +199,42 @@ static char *process_str_arg(CHARSET_INFO *cs, char *to, const char *end,
>                               size_t width, char *par, uint print_type)
>  {
>    int well_formed_error;
> -  size_t plen, left_len= (size_t) (end - to) + 1;
> +  uint dots= 0;
> +  size_t plen, left_len= (size_t) (end - to) + 1, slen=0;
>    if (!par)
>      par = (char*) "(null)";
>  
> -  plen= strnlen(par, width);
> +  plen= slen= strnlen(par, width + 1);
> +  if (plen > width)
> +    plen= width;
>    if (left_len <= plen)
>      plen = left_len - 1;
> +  if ((slen > plen) && (print_type & SH_TRUNC_ARG))
> +  {
> +    if (plen < 3)
> +    {
> +      dots= plen;
> +      plen= 0;
> +    }
> +    else
> +    {
> +      plen-= 3;
> +      dots= 3;
> +    }
> +  }
> +
>    plen= my_well_formed_length(cs, par, par + plen, width, &well_formed_error);
>    if (print_type & ESCAPED_ARG)
>      to= backtick_string(cs, to, end, par, plen, '`');
>    else
>      to= strnmov(to,par,plen);
> +
> +  if (dots)
> +  {
> +    for (; dots; dots--)
> +      *(to++)= '.';
> +    *(to)= 0;
> +  }

This should look strange when truncating a quoted string. See the test
case below

>    return to;
>  }
>  
> diff --git a/unittest/mysys/my_vsnprintf-t.c b/unittest/mysys/my_vsnprintf-t.c
> index 6ba0a42cf7e..e8831f7c1f4 100644
> --- a/unittest/mysys/my_vsnprintf-t.c
> +++ b/unittest/mysys/my_vsnprintf-t.c
> @@ -99,12 +99,12 @@ int main(void)
>    test1("Width is ignored for strings <x> <y>",
>          "Width is ignored for strings <%04s> <%5s>", "x", "y");
>  
> -  test1("Precision works for strings <abcde>",
> +  test1("Precision works for strings <ab...>",
>          "Precision works for strings <%.5s>", "abcdef!");
>  
> -  test1("Flag '`' (backtick) works: `abcd` `op``q` (mysql extension)", This 
> -        "Flag '`' (backtick) works: %`s %`.4s (mysql extension)", This 
> -        "abcd", "op`qrst");
> +  test1("Flag '`' (backtick) works: `abcd` `op``q`... (mysql extension)",
> +        "Flag '`' (backtick) works: %`s %`.7s (mysql extension)",
> +        "abcd", "op`qrstuuuuuuuuu");

Here, the test case with a quoted string. First, you print 10 characters
instead of 7. Second, you put dots after the closing backtick, which
looks confusing, the truncation happens before the backtick.  Third, as
you can see, you cannot use strlen() for a string length because there
can be backticks inside the string.

perhaps it would be simpler to move ... inside backtick_string()

>  
>    test1("Length modifiers work: 1 * -1 * 2 * 3",
>          "Length modifiers work: %d * %ld * %lld * %zd", 1, -1L, 2LL, (size_t)3);
> @@ -125,7 +125,7 @@ int main(void)
>    test1("Asterisk '*' as a width works: <    4>",
>          "Asterisk '*' as a width works: <%*d>", 5, 4);
>  
> -  test1("Asterisk '*' as a precision works: <qwerty>",
> +  test1("Asterisk '*' as a precision works: <qwe...>",
>          "Asterisk '*' as a precision works: <%.*s>", 6, "qwertyuiop");

please add tests for ...truncation in %M and %1$s

>  
>    test1("Positional arguments for a width: <    4>",
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx