From 2eebf239cb88a067f3956aed9e01f8f5534d4371 Mon Sep 17 00:00:00 2001
From: Daniel Lenski <dlenski@amazon.com>
Date: Mon, 10 Jul 2023 13:57:07 -0700
Subject: [PATCH] Replace safe_strcpy with a more efficient version that
 doesn't trigger GCC 8+ warnings

The `safe_strcpy()` function was added in
https://github.com/mariadb/server/commit/567b68129943#diff-23f88d0b52735bf79b7eb76e2ddbbebc96f3b1ca16e784a347525a9c43134d77

Unfortunately, its current implementation triggers many GCC 8+ string
truncation and array bounds warnings, particularly due to the potential
for a false positive `-Warray-bounds`.

For example, the line `safe_strcpy(delimiter, sizeof(delimiter), ";")` in
`client/mysqldump.c` causes the following warning:

    [1669/1914] Building C object client/CMakeFiles/mariadb-dump.dir/mysqldump.c.o
    In file included from /PATH/include/my_sys.h:20,
                     from /PATH/mysqldump.c:51:
    In function ?safe_strcpy?,
        inlined from ?dump_events_for_db.isra? at /PATH/client/mysqldump.c:2595:3:
    /PATH/include/m_string.h:258:39: warning: array subscript 1535 is outside array bounds of ?const char[2]? [-Warray-bounds=]
      258 |   if (dst[dst_size - 2] != '\0' && src[dst_size - 1] != '\0')
          |                                    ~~~^~~~~~~~~~~~~~

GCC is reporting that the `safe_strcpy` function *could* cause an
out-of-bounds read from the constant *source* string `";"`, however this
warning is unhelpful and confusing because it can only happen if the size of
the *destination* buffer is incorrectly specified, which is not the case
here.

In https://github.com/MariaDB/server/pull/2640, Andrew Hutchings proposed
fixing this by disabling the `-Warray-bounds` check in this function
(specifically in
https://github.com/MariaDB/server/pull/2640/commits/be382d01d08739d081f6cf40f350f7414f29b49d#diff-23f88d0b52735bf79b7eb76e2ddbbebc96f3b1ca16e784a347525a9c43134d77R255-R262).

However, this was rejected because it also disables the *helpful*
`-Warray-bounds` check on the destination buffer.

This commit should solve these problems:

1. It reimplements `safe_strcpy` a bit more efficiently, skipping the
   `memset(dst, 0, dst_size)`. This is unnecessary since `strncpy` already
   pads `dst` with 0 bytes.
2. It will not trigger the `-Warray-bounds` warning, because `src` is
   not read based on an offset determined from `dst_size`.
3. It does trigger the `-Wstringop-truncation` warning in `strncpy`
   (https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-truncation),
   so we need to disable that.  However, this is a less broad and
   generally-useful warning so there is no loss of static analysis value
   caused by disabling it.

All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
---
 include/m_string.h | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/include/m_string.h b/include/m_string.h
index 8edaf19bc0ada..e8ccf59d50b49 100644
--- a/include/m_string.h
+++ b/include/m_string.h
@@ -235,14 +235,31 @@ static inline void lex_string_set3(LEX_CSTRING *lex_str, const char *c_str,
 */
 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. */
+  /* 1) IF there is a 0 byte in the first dst_size bytes of src, strncpy will
+   *    0-terminate dst, and pad dst with additional 0 bytes out to dst_size.
+   *
+   * 2) IF there is no 0 byte in the first dst_size bytes of src, strncpy will
+   *    copy dst_size bytes, and the final byte won't be 0.
+   *
+   * In GCC 8+, the `-Wstringop-truncation` warning will object to strncpy()
+   * being used in this way, so we need to disable this warning for this
+   * single statement.
+   */
+
+#if defined(__GNUC__) && __GNUC__ >= 8
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstringop-truncation"
+#endif
+  strncpy(dst, src, dst_size);
+#if defined(__GNUC__) && __GNUC__ >= 8
+#pragma GCC diagnostic pop
+#endif
+
+  if (dst[dst_size - 1] != '\0') {
+    /* Only possible in case (2), meaning src was truncated. */
+    dst[dst_size - 1]= '\0';
+    return 1;
+  }
   return 0;
 }