Fixup warnings for safe_strcpy

[LinuxJedi]

This fixes warnings in safe_strcpy when compiling with mysql_release build config.

How can this PR be tested?

Compile with mysql_release config, many of the warnings in CONNECT engine go away. Then normal CI runs.

Basing the PR against the correct MariaDB version

• This is a new feature and the PR is based against the latest MariaDB development branch
• This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced

[dlenski]

This fixes warnings in $WHEREVER when compiling with mysql_release build config.

Perhaps consider adding -Werror to the release build CI, so that CI will fail until they're fixed?

(That option can also take a specific set of warnings to convert to errors, if it would be too abrupt to do that all at once 😅)

[grooverdan]

Why?

We see a need to do major changes to avoid sprintf, which had pretty good detection on the lengths (because it was calculable) and now we don't trust the compiler warning (here, now that we've made it harder for the compiler to detect array overflow, which it does less reliably it seems), and now we silence the compiler warnings (because humans are more reliable at specifying length?)?

At least some warnings appear to be valid because the wrong dst_size was passed to the safe_strcpy function. E.g:

[1655/2227] Building C object client/CMakeFiles/mysql_plugin.dir/mysql_plugin.c.o
In file included from /home/dan/repos/mariadb-server-10.4/client/mysql_plugin.c:19:
In function ‘safe_strcpy’,
    inlined from ‘load_plugin_data’ at /home/dan/repos/mariadb-server-10.4/client/mysql_plugin.c:689:11,
    inlined from ‘check_options’ at /home/dan/repos/mariadb-server-10.4/client/mysql_plugin.c:820:9,
    inlined from ‘process_options’ at /home/dan/repos/mariadb-server-10.4/client/mysql_plugin.c:909:15,
    inlined from ‘main’ at /home/dan/repos/mariadb-server-10.4/client/mysql_plugin.c:123:15:
/home/dan/repos/mariadb-server-10.4/include/m_string.h:257:39: warning: array subscript 1023 is outside array bounds of ‘const char[4]’ [-Warray-bounds=]
  257 |   if (dst[dst_size - 2] != '\0' && src[dst_size - 1] != '\0')
      |                                    ~~~^~~~~~~~~~~~~~

mysql_plugin.c:689 is:

        if (safe_strcpy(line + line_len - 1, sizeof(line), FN_SOEXT))

line is:

    char line[1024];

Obviously sizeof(line) is the incorrect size if line + line_len - 1 is the staring point.

So under the right conditions, we've created a vulnerability.

This error wouldn't have been seen by disabling warning. Thankfully its, for now, the only one. But if we hide the warnings, what hope of seeing this is there?

So we're actually relying on review alone to determine the correct size is passed on each of these changes.

Has a more reliable code base actually been created?

[grooverdan]

Is there a gcc bug report for the false positives of array-bounds?

[LinuxJedi]

@grooverdan so far the only more reliable way tried has been a strlen, which we want to avoid. I will close this then and try and think about it. Although since no one looks at these warnings, your case still wouldn't be caught until now.

I don't know if this specific bug has been filed. There are a lot of false positive bugs for array-bounds.

@dlenski at the moment we have -DMYSQL_MAINTAINER_MODE=ON for this. Unfortunately on certain platforms I think this is going to be very difficult to do.

[dlenski]

@grooverdan wrote:

Is there a gcc bug report for the false positives of array-bounds?

There probably should be… -Warray-bounds has been there since GCC 8, and is still giving this false positive in GCC 13.

On the other hand, this is a tricky function to statically analyze:

static inline int safe_strcpy(char *dst, size_t dst_size, const char *src)
{
  memset(dst, '\0', dst_size);
  strncpy(dst, src, dst_size - 1);
  /*
     If the first condition is true, we are guaranteed to have src length
     >= (dst_size - 1), hence safe to access src[dst_size - 1].
  */
  if (dst[dst_size - 2] != '\0' && src[dst_size - 1] != '\0')
    return 1; /* Truncation of src. */
  return 0;
}

It's very hard for the compiler to figure out when src[dst_size - 1] might be read, or might not be read.

In #2692, I propose a replacement that will avoid the -Warray-bounds false positives, while making the function do the same thing (and slightly more efficiently to boot, no need to memset).